[Notes] [Git][BuildStream/buildstream][tristan/error-message-regression]

Tristan Van Berkom pushed to branch tristan/error-message-regression at BuildStream / buildstream

Commits:

19adc9e5

by Tristan Van Berkom at 2019-01-16T20:48:10Z

element.py: Cleanup temporary staging directories on termination

Use utils._tempdir() which in turn uses _signals.terminator() for
this purpose.

This also changes utils._tempdir() to use utils._force_rmtree()
so that it is safe to use for cleaning up the staging directory.

This patch fixes orphaned temporary directories being left behind
in the artifact cache directory at forceful termination time.

5e9598c5

by Tristan Van Berkom at 2019-01-16T21:17:26Z

Merge branch 'tristan/cleanup-staging-tempdir' into 'master'

element.py: Cleanup temporary staging directories on termination

See merge request BuildStream/buildstream!1080

0fffea91

by Tristan Van Berkom at 2019-01-16T23:26:26Z

sandbox/sandbox.py: Display failed commands in the detail string

We should only display commands in detail strings, not in the
message texts.

This also updates tests/integration/sandbox-bwrap.py to expect
the new message string which only contains the command exit status
and not the whole command itself, this does not alter the validity
of the text case which is checking that we can obtain the expected
return value.

Changes:

buildstream/_exceptions.py

@@ -262,8 +262,8 @@ class PlatformError(BstError):
  # Raised when errors are encountered by the sandbox implementation
+ #
  class SandboxError(BstError):
 -    def __init__(self, message, reason=None):
 -        super().__init__(message, domain=ErrorDomain.SANDBOX, reason=reason)
 +    def __init__(self, message, detail=None, reason=None):
 +        super().__init__(message, detail=detail, domain=ErrorDomain.SANDBOX, reason=reason)
  # ArtifactError

buildstream/element.py

@@ -1398,43 +1398,40 @@ class Element(Plugin):
      #     usebuildtree (bool): use a the elements build tree as its source.
+     #
      def _stage_sources_at(self, vdirectory, mount_workspaces=True, usebuildtree=False):
 -        with self.timed_activity("Staging sources", silent_nested=True):
 +        context = self._get_context()
++
 +        # It's advantageous to have this temporary directory on
 +        # the same file system as the rest of our cache.
 +        with self.timed_activity("Staging sources", silent_nested=True), \
 +            utils._tempdir(dir=context.artifactdir, prefix='staging-temp') as temp_staging_directory:
++
 +            import_dir = temp_staging_directory
++
              if not isinstance(vdirectory, Directory):
                  vdirectory = FileBasedDirectory(vdirectory)
              if not vdirectory.is_empty():
                  raise ElementError("Staging directory '{}' is not empty".format(vdirectory))
 -            # It's advantageous to have this temporary directory on
 -            # the same filing system as the rest of our cache.
 -            temp_staging_location = os.path.join(self._get_context().artifactdir, "staging_temp")
 -            temp_staging_directory = tempfile.mkdtemp(prefix=temp_staging_location)
 -            import_dir = temp_staging_directory
+-
 -            try:
 -                workspace = self._get_workspace()
 -                if workspace:
 -                    # If mount_workspaces is set and we're doing incremental builds,
 -                    # the workspace is already mounted into the sandbox.
 -                    if not (mount_workspaces and self.__can_build_incrementally()):
 -                        with self.timed_activity("Staging local files at {}"
 -                                                 .format(workspace.get_absolute_path())):
 -                            workspace.stage(temp_staging_directory)
 -                # Check if we have a cached buildtree to use
 -                elif usebuildtree:
 -                    artifact_base, _ = self.__extract()
 -                    import_dir = os.path.join(artifact_base, 'buildtree')
 -                else:
 -                    # No workspace or cached buildtree, stage source directly
 -                    for source in self.sources():
 -                        source._stage(temp_staging_directory)
 +            workspace = self._get_workspace()
 +            if workspace:
 +                # If mount_workspaces is set and we're doing incremental builds,
 +                # the workspace is already mounted into the sandbox.
 +                if not (mount_workspaces and self.__can_build_incrementally()):
 +                    with self.timed_activity("Staging local files at {}"
 +                                             .format(workspace.get_absolute_path())):
 +                        workspace.stage(temp_staging_directory)
++
 +            # Check if we have a cached buildtree to use
 +            elif usebuildtree:
 +                artifact_base, _ = self.__extract()
 +                import_dir = os.path.join(artifact_base, 'buildtree')
 +            else:
 +                # No workspace or cached buildtree, stage source directly
 +                for source in self.sources():
 +                    source._stage(temp_staging_directory)
 -                vdirectory.import_files(import_dir)
 +            vdirectory.import_files(import_dir)
 -            finally:
 -                # Staging may produce directories with less than 'rwx' permissions
 -                # for the owner, which breaks tempfile. _force_rmtree will deal
 -                # with these.
 -                utils._force_rmtree(temp_staging_directory)
          # Ensure deterministic mtime of sources at build time
          vdirectory.set_deterministic_mtime()
          # Ensure deterministic owners of sources at build time

buildstream/sandbox/sandbox.py

@@ -86,10 +86,11 @@ class SandboxCommandError(SandboxError):
      Args:
         message (str): The error message to report to the user
 +       detail (str): The detailed error string
         collect (str): An optional directory containing partial install contents
      """
 -    def __init__(self, message, *, collect=None):
 -        super().__init__(message, reason='command-failed')
 +    def __init__(self, message, *, detail=None, collect=None):
 +        super().__init__(message, detail=detail, reason='command-failed')
          self.collect = collect
@@ -599,8 +600,8 @@ class _SandboxBatch():
          if exitcode != 0:
              cmdline = ' '.join(shlex.quote(cmd) for cmd in command.command)
              label = command.label or cmdline
 -            raise SandboxCommandError("Command '{}' failed with exitcode {}".format(label, exitcode),
 -                                      collect=self.collect)
 +            raise SandboxCommandError("Command failed with exitcode {}".format(exitcode),
 +                                      detail=label, collect=self.collect)
      def execute_call(self, call):
          call.callback()

buildstream/utils.py

@@ -1023,7 +1023,7 @@ def _tempdir(suffix="", prefix="tmp", dir=None):  # pylint: disable=redefined-bu
      def cleanup_tempdir():
          if os.path.isdir(tempdir):
 -            shutil.rmtree(tempdir)
 +            _force_rmtree(tempdir)
      try:
          with _signals.terminator(cleanup_tempdir):

tests/integration/sandbox-bwrap.py

@@ -59,4 +59,4 @@ def test_sandbox_bwrap_return_subprocess(cli, tmpdir, datafiles):
      result = cli.run(project=project, args=['build', element_name])
      result.assert_task_error(error_domain=ErrorDomain.SANDBOX, error_reason="command-failed")
 -    assert "sandbox-bwrap/command-exit-42.bst|Command 'exit 42' failed with exitcode 42" in result.stderr
 +    assert "sandbox-bwrap/command-exit-42.bst|Command failed with exitcode 42" in result.stderr

[Notes] [Git][BuildStream/buildstream][tristan/error-message-regression] 3 commits: element.py: Cleanup temporary staging directories on termination

Tristan Van Berkom pushed to branch tristan/error-message-regression at BuildStream / buildstream

Commits:

5 changed files:

Changes: