[Notes] [Git][BuildStream/buildstream][tlater/message-lines] 9 commits:

Tristan Van Berkom pushed to branch tlater/message-lines at BuildStream / buildstream

Commits:

951a8df1
by Angelos Evripiotis at 2019-01-18T18:02:22Z
```
cli.py: add a hint about '--' to 'bst shell' help
```

9911023f

by Angelos Evripiotis at 2019-01-18T18:56:21Z

Merge branch 'aevri/shell_separator_hint' into 'master'

cli.py: add a hint about '--' to 'bst shell' help

See merge request BuildStream/buildstream!1078

c9ce89d2

by Tristan Van Berkom at 2019-01-18T19:36:26Z

_cas/cascache.py: Cleanup directories when removing refs

With out this, empty directories in the refs/heads directory just
grow unconditionally.

c536ab6a

by Tristan Van Berkom at 2019-01-18T19:36:26Z

tests/artifactcache/expiry.py: Test refs directory cleanup

Enhance the test which checks removal of the extract directories
with an additional check that the ref directories are cleaned up
when removing artifacts.

99699ffc

by Tristan Van Berkom at 2019-01-18T19:36:26Z

utils.py: Added _tempnamedfile()

When used in a child process in BuildStream, this should be
used instead of tempfile.NamedTemporaryFile() directly, otherwise
we fail to cleanup the file on SIGTERM.

8ce483d4

by Tristan Van Berkom at 2019-01-18T19:36:26Z

_cas/cascache.py: Use utils._tempdir() and utils._tempnamedfile()

The direct usage of tempfile.TemporaryDirectory() and
tempfile.NamedTemporaryFile() here causes leakage of any temporary
data when the process operating on temporary data is terminated
with SIGTERM.

Using the utilities ensures that trash is not left behind in
~/.cache/buildstream/artifacts/tmp when the user terminates
BuildStream with ^C.

73c7252d

by Tristan Van Berkom at 2019-01-18T20:57:42Z

Merge branch 'tristan/cas-cleanup-improve' into 'master'

CASCache cleanup improvements

See merge request BuildStream/buildstream!1087

32735414

by Tristan Maat at 2019-01-18T21:29:52Z

testutils/runcli.py: Allow removing artifacts from arbitrary dirs

`remove_artifact_from_cache` used a hard-coded path to remove
artifacts, which wasn't sufficient for integration tests.

cc4fb4bd

by Tristan Maat at 2019-01-18T21:29:52Z

widget.py: Avoid "showing 0 lines" messages when there are no lines

This happened when bst is invoked with --message-lines 0 or
--error-lines 0, and was arguably a little too verbose (the user
explicitly asked us not to show them any lines, after all).

Fixes #779

Changes:

buildstream/_cas/cascache.py

@@ -21,7 +21,7 @@ import hashlib
  import itertools
  import os
  import stat
 -import tempfile
 +import errno
  import uuid
  import contextlib
@@ -129,7 +129,7 @@ class CASCache():
              else:
                  return dest
 -        with tempfile.TemporaryDirectory(prefix='tmp', dir=self.tmpdir) as tmpdir:
 +        with utils._tempdir(prefix='tmp', dir=self.tmpdir) as tmpdir:
              checkoutdir = os.path.join(tmpdir, ref)
              self._checkout(checkoutdir, tree)
@@ -374,7 +374,7 @@ class CASCache():
                      for chunk in iter(lambda: tmp.read(4096), b""):
                          h.update(chunk)
                  else:
 -                    tmp = stack.enter_context(tempfile.NamedTemporaryFile(dir=self.tmpdir))
 +                    tmp = stack.enter_context(utils._tempnamedfile(dir=self.tmpdir))
                      # Set mode bits to 0644
                      os.chmod(tmp.name, stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
@@ -545,11 +545,7 @@ class CASCache():
      def remove(self, ref, *, defer_prune=False):
          # Remove cache ref
 -        refpath = self._refpath(ref)
 -        if not os.path.exists(refpath):
 -            raise CASCacheError("Could not find ref '{}'".format(ref))
+-
 -        os.unlink(refpath)
 +        self._remove_ref(ref)
          if not defer_prune:
              pruned = self.prune()
@@ -626,6 +622,55 @@ class CASCache():
      def _refpath(self, ref):
          return os.path.join(self.casdir, 'refs', 'heads', ref)
 +    # _remove_ref()
 +    #
 +    # Removes a ref.
 +    #
 +    # This also takes care of pruning away directories which can
 +    # be removed after having removed the given ref.
 +    #
 +    # Args:
 +    #    ref (str): The ref to remove
 +    #
 +    # Raises:
 +    #    (CASCacheError): If the ref didnt exist, or a system error
 +    #                     occurred while removing it
 +    #
 +    def _remove_ref(self, ref):
++
 +        # Remove the ref itself
 +        refpath = self._refpath(ref)
 +        try:
 +            os.unlink(refpath)
 +        except FileNotFoundError as e:
 +            raise CASCacheError("Could not find ref '{}'".format(ref)) from e
++
 +        # Now remove any leading directories
 +        basedir = os.path.join(self.casdir, 'refs', 'heads')
 +        components = list(os.path.split(ref))
 +        while components:
 +            components.pop()
 +            refdir = os.path.join(basedir, *components)
++
 +            # Break out once we reach the base
 +            if refdir == basedir:
 +                break
++
 +            try:
 +                os.rmdir(refdir)
 +            except FileNotFoundError:
 +                # The parent directory did not exist, but it's
 +                # parent directory might still be ready to prune
 +                pass
 +            except OSError as e:
 +                if e.errno == errno.ENOTEMPTY:
 +                    # The parent directory was not empty, so we
 +                    # cannot prune directories beyond this point
 +                    break
++
 +                # Something went wrong here
 +                raise CASCacheError("System error while removing ref '{}': {}".format(ref, e)) from e
++
      # _commit_directory():
+     #
      # Adds local directory to content addressable store.
@@ -797,7 +842,7 @@ class CASCache():
              # already in local repository
              return objpath
 -        with tempfile.NamedTemporaryFile(dir=self.tmpdir) as f:
 +        with utils._tempnamedfile(dir=self.tmpdir) as f:
              remote._fetch_blob(digest, f)
              added_digest = self.add_object(path=f.name, link_directly=True)
@@ -807,7 +852,7 @@ class CASCache():
      def _batch_download_complete(self, batch):
          for digest, data in batch.send():
 -            with tempfile.NamedTemporaryFile(dir=self.tmpdir) as f:
 +            with utils._tempnamedfile(dir=self.tmpdir) as f:
                  f.write(data)
                  f.flush()
@@ -904,7 +949,7 @@ class CASCache():
      def _fetch_tree(self, remote, digest):
          # download but do not store the Tree object
 -        with tempfile.NamedTemporaryFile(dir=self.tmpdir) as out:
 +        with utils._tempnamedfile(dir=self.tmpdir) as out:
              remote._fetch_blob(digest, out)
              tree = remote_execution_pb2.Tree()

buildstream/_frontend/cli.py

@@ -554,6 +554,12 @@ def shell(app, element, sysroot, mount, isolate, build_, cli_buildtree, command)
      element, assuming it has already been built and all required
      artifacts are in the local cache.
 +    Use '--' to separate a command from the options to bst,
 +    otherwise bst may respond to them instead. e.g.
++
 +    \b
 +        bst shell example.bst -- df -h
++
      Use the --build option to create a temporary sysroot for
      building the element instead.

buildstream/_frontend/widget.py

@@ -647,8 +647,9 @@ class LogLine(Widget):
              abbrev = False
              if message.message_type not in ERROR_MESSAGES \
                 and not frontend_message and n_lines > self._message_lines:
 -                abbrev = True
                  lines = lines[0:self._message_lines]
 +                if self._message_lines > 0:
 +                    abbrev = True
              else:
                  lines[n_lines - 1] = lines[n_lines - 1].rstrip('\n')
@@ -674,7 +675,7 @@ class LogLine(Widget):
              if self.context is not None and not self.context.log_verbose:
                  text += self._indent + self._err_profile.fmt("Log file: ")
                  text += self._indent + self._logfile_widget.render(message) + '\n'
 -            else:
 +            elif self._log_lines > 0:
                  text += self._indent + self._err_profile.fmt("Printing the last {} lines from log file:"
                                                               .format(self._log_lines)) + '\n'
                  text += self._indent + self._logfile_widget.render(message, abbrev=False) + '\n'

buildstream/utils.py

@@ -1032,6 +1032,36 @@ def _tempdir(suffix="", prefix="tmp", dir=None):  # pylint: disable=redefined-bu
          cleanup_tempdir()
 +# _tempnamedfile()
 +#
 +# A context manager for doing work on an open temporary file
 +# which is guaranteed to be named and have an entry in the filesystem.
 +#
 +# Args:
 +#    dir (str): A path to a parent directory for the temporary file
 +#    suffix (str): A suffix for the temproary file name
 +#    prefix (str): A prefix for the temporary file name
 +#
 +# Yields:
 +#    (str): The temporary file handle
 +#
 +# Do not use tempfile.NamedTemporaryFile() directly, as this will
 +# leak files on the filesystem when BuildStream exits a process
 +# on SIGTERM.
 +#
 +@contextmanager
 +def _tempnamedfile(suffix="", prefix="tmp", dir=None):  # pylint: disable=redefined-builtin
 +    temp = None
++
 +    def close_tempfile():
 +        if temp is not None:
 +            temp.close()
++
 +    with _signals.terminator(close_tempfile), \
 +        tempfile.NamedTemporaryFile(suffix=suffix, prefix=prefix, dir=dir) as temp:
 +        yield temp
++
++
  # _kill_process_tree()
+ #
  # Brutally murder a process and all of its children

tests/artifactcache/expiry.py

@@ -382,6 +382,7 @@ def test_extract_expiry(cli, datafiles, tmpdir):
      res = cli.run(project=project, args=['checkout', 'target.bst', os.path.join(str(tmpdir), 'checkout')])
      res.assert_success()
 +    # Get a snapshot of the extracts in advance
      extractdir = os.path.join(project, 'cache', 'artifacts', 'extract', 'test', 'target')
      extracts = os.listdir(extractdir)
      assert(len(extracts) == 1)
@@ -395,3 +396,16 @@ def test_extract_expiry(cli, datafiles, tmpdir):
      # Now the extract should be removed.
      assert not os.path.exists(extract)
++
 +    # As an added bonus, let's ensure that no directories have been left behind
 +    #
 +    # Now we should have a directory for the cached target2.bst, which
 +    # replaced target.bst in the cache, we should not have a directory
 +    # for the target.bst
 +    refsdir = os.path.join(project, 'cache', 'artifacts', 'cas', 'refs', 'heads')
 +    refsdirtest = os.path.join(refsdir, 'test')
 +    refsdirtarget = os.path.join(refsdirtest, 'target')
 +    refsdirtarget2 = os.path.join(refsdirtest, 'target2')
++
 +    assert os.path.isdir(refsdirtarget2)
 +    assert not os.path.exists(refsdirtarget)

tests/integration/messages.py

 +#
 +#  Copyright (C) 2018 Codethink Limited
 +#
 +#  This program is free software; you can redistribute it and/or
 +#  modify it under the terms of the GNU Lesser General Public
 +#  License as published by the Free Software Foundation; either
 +#  version 2 of the License, or (at your option) any later version.
 +#
 +#  This library is distributed in the hope that it will be useful,
 +#  but WITHOUT ANY WARRANTY; without even the implied warranty of
 +#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 +#  Lesser General Public License for more details.
 +#
 +#  You should have received a copy of the GNU Lesser General Public
 +#  License along with this library. If not, see <http://www.gnu.org/licenses/>.
 +#
 +#  Authors: Tristan Maat <tristan maat codethink co uk>
 +#
++
 +import os
 +import pytest
++
 +from buildstream import _yaml
 +from buildstream._exceptions import ErrorDomain
++
 +from tests.testutils import cli_integration as cli
 +from tests.testutils.site import HAVE_BWRAP, IS_LINUX
++
++
 +pytestmark = pytest.mark.integration
++
++
 +# Project directory
 +DATA_DIR = os.path.join(
 +    os.path.dirname(os.path.realpath(__file__)),
 +    "project",
 +)
++
++
 +@pytest.mark.integration
 +@pytest.mark.datafiles(DATA_DIR)
 +@pytest.mark.skipif(IS_LINUX and not HAVE_BWRAP, reason='Only available with bubblewrap on Linux')
 +def test_disable_message_lines(cli, tmpdir, datafiles):
 +    project = os.path.join(datafiles.dirname, datafiles.basename)
 +    element_path = os.path.join(project, 'elements')
 +    element_name = 'message.bst'
++
 +    element = {
 +        'kind': 'manual',
 +        'depends': [{
 +            'filename': 'base.bst'
 +        }],
 +        'config': {
 +            'build-commands':
 +            ['echo "Silly message"'],
 +            'strip-commands': []
 +        }
 +    }
++
 +    os.makedirs(os.path.dirname(os.path.join(element_path, element_name)), exist_ok=True)
 +    _yaml.dump(element, os.path.join(element_path, element_name))
++
 +    # First we check that we get the "Silly message"
 +    result = cli.run(project=project, args=["build", element_name])
 +    result.assert_success()
 +    assert 'echo "Silly message"' in result.stderr
++
 +    # Let's now build it again, but with --message-lines 0
 +    cli.remove_artifact_from_cache(project, element_name)
 +    result = cli.run(project=project, args=["--message-lines", "0",
 +                                            "build", element_name])
 +    result.assert_success()
 +    assert "Message contains " not in result.stderr
++
++
 +@pytest.mark.integration
 +@pytest.mark.datafiles(DATA_DIR)
 +@pytest.mark.skipif(IS_LINUX and not HAVE_BWRAP, reason='Only available with bubblewrap on Linux')
 +def test_disable_error_lines(cli, tmpdir, datafiles):
 +    project = os.path.join(datafiles.dirname, datafiles.basename)
 +    element_path = os.path.join(project, 'elements')
 +    element_name = 'message.bst'
++
 +    element = {
 +        'kind': 'manual',
 +        'depends': [{
 +            'filename': 'base.bst'
 +        }],
 +        'config': {
 +            'build-commands':
 +            ['This is a syntax error > >'],
 +            'strip-commands': []
 +        }
 +    }
++
 +    os.makedirs(os.path.dirname(os.path.join(element_path, element_name)), exist_ok=True)
 +    _yaml.dump(element, os.path.join(element_path, element_name))
++
 +    # First we check that we get the syntax error
 +    result = cli.run(project=project, args=["--error-lines", "0",
 +                                            "build", element_name])
 +    result.assert_main_error(ErrorDomain.STREAM, None)
 +    assert "This is a syntax error" in result.stderr
++
 +    # Let's now build it again, but with --error-lines 0
 +    cli.remove_artifact_from_cache(project, element_name)
 +    result = cli.run(project=project, args=["--error-lines", "0",
 +                                            "build", element_name])
 +    result.assert_main_error(ErrorDomain.STREAM, None)
 +    assert "Printing the last" not in result.stderr

tests/testutils/runcli.py

@@ -245,8 +245,14 @@ class Cli():
      def remove_artifact_from_cache(self, project, element_name,
                                     *, cache_dir=None):
 +        # Read configuration to figure out where artifacts are stored
          if not cache_dir:
 -            cache_dir = os.path.join(project, 'cache', 'artifacts')
 +            default = os.path.join(project, 'cache', 'artifacts')
++
 +            if self.config is not None:
 +                cache_dir = self.config.get('artifactdir', default)
 +            else:
 +                cache_dir = default
          cache_dir = os.path.join(cache_dir, 'cas', 'refs', 'heads')

[Notes] [Git][BuildStream/buildstream][tlater/message-lines] 9 commits: cli.py: add a hint about '--' to 'bst shell' help

Tristan Van Berkom pushed to branch tlater/message-lines at BuildStream / buildstream

Commits:

7 changed files:

Changes: