Merge branch 'juerg/cache-buildtrees' into 'master'

Tweak cache-buildtrees option

See merge request BuildStream/buildstream!1208

13 files changed, 97 insertions, 62 deletions

diff --git a/NEWS b/NEWS
index 85bf27c2c..a19182c09 100644
--- a/NEWS
+++ b/NEWS
@@ -137,8 +137,8 @@ buildstream 1.3.1
     Element types without a build-root were already cached with an empty build tree
     directory, this can now be extended to all or successful artifacts to save on cache
     overheads. The cli main option '--cache-buildtrees' or the user configuration cache
-    group option 'cache-buildtrees' can be set as 'always', 'failure' or 'never', with
-    the default being always. Note, as the cache-key for the artifact is independant of
+    group option 'cache-buildtrees' can be set as 'always', 'auto' or 'never', with
+    the default being 'auto'. Note, as the cache-key for the artifact is independent of
     the cached build tree input it will remain unaltered, however the availbility of the
     build tree content may differ.
 
diff --git a/buildstream/_artifact.py b/buildstream/_artifact.py
index 0c1e71194..07c02f4f7 100644
--- a/buildstream/_artifact.py
+++ b/buildstream/_artifact.py
@@ -128,7 +128,6 @@ class Artifact():
         assemblevdir = CasBasedDirectory(cas_cache=self._artifacts.cas)
         logsvdir = assemblevdir.descend("logs", create=True)
         metavdir = assemblevdir.descend("meta", create=True)
-        buildtreevdir = assemblevdir.descend("buildtree", create=True)
 
         # Create artifact directory structure
         assembledir = os.path.join(rootdir, 'artifact')
@@ -142,14 +141,8 @@ class Artifact():
             filesvdir = assemblevdir.descend("files", create=True)
             filesvdir.import_files(collectvdir)
 
-        # cache_buildtrees defaults to 'always', as such the
-        # default behaviour is to attempt to cache them. If only
-        # caching failed artifact buildtrees, then query the build
-        # result. Element types without a build-root dir will be cached
-        # with an empty buildtreedir regardless of this configuration as
-        # there will not be an applicable sandbox_build_dir.
-
         if sandbox_build_dir:
+            buildtreevdir = assemblevdir.descend("buildtree", create=True)
             buildtreevdir.import_files(sandbox_build_dir)
 
         # Write some logs out to normal directories: logsdir and metadir
@@ -227,6 +220,22 @@ class Artifact():
 
         return True
 
+    # buildtree_exists()
+    #
+    # Check if artifact was created with a buildtree. This does not check
+    # whether the buildtree is present in the local cache.
+    #
+    # Returns:
+    #     (bool): True if artifact was created with buildtree
+    #
+    def buildtree_exists(self):
+
+        if not self._element._cached():
+            return False
+
+        artifact_vdir, _ = self._get_directory()
+        return artifact_vdir._exists('buildtree')
+
     # load_public_data():
     #
     # Loads the public data from the cached artifact
diff --git a/buildstream/_cas/cascache.py b/buildstream/_cas/cascache.py
index 2be16067b..e07b4d4b4 100644
--- a/buildstream/_cas/cascache.py
+++ b/buildstream/_cas/cascache.py
@@ -128,12 +128,14 @@ class CASCache():
     def contains_subdir_artifact(self, ref, subdir):
         tree = self.resolve_ref(ref)
 
-        # This assumes that the subdir digest is present in the element tree
-        subdirdigest = self._get_subdir(tree, subdir)
-        objpath = self.objpath(subdirdigest)
+        try:
+            subdirdigest = self._get_subdir(tree, subdir)
+            objpath = self.objpath(subdirdigest)
 
-        # True if subdir content is cached or if empty as expected
-        return os.path.exists(objpath)
+            # True if subdir content is cached or if empty as expected
+            return os.path.exists(objpath)
+        except CASCacheError:
+            return False
 
     # checkout():
     #
diff --git a/buildstream/_context.py b/buildstream/_context.py
index 286e2d223..476032f39 100644
--- a/buildstream/_context.py
+++ b/buildstream/_context.py
@@ -269,7 +269,7 @@ class Context():
 
         # Load cache build trees configuration
         self.cache_buildtrees = _node_get_option_str(
-            cache, 'cache-buildtrees', ['always', 'failure', 'never'])
+            cache, 'cache-buildtrees', ['always', 'auto', 'never'])
 
         # Load logging config
         logging = _yaml.node_get(defaults, Mapping, 'logging')
diff --git a/buildstream/_frontend/app.py b/buildstream/_frontend/app.py
index 329f9a2c6..c1a5e57b7 100644
--- a/buildstream/_frontend/app.py
+++ b/buildstream/_frontend/app.py
@@ -602,7 +602,7 @@ class App():
                     click.echo("\nDropping into an interactive shell in the failed build sandbox\n", err=True)
                     try:
                         prompt = self.shell_prompt(element)
-                        self.stream.shell(element, Scope.BUILD, prompt, isolate=True, usebuildtree=True)
+                        self.stream.shell(element, Scope.BUILD, prompt, isolate=True, usebuildtree='always')
                     except BstError as e:
                         click.echo("Error while attempting to create interactive shell: {}".format(e), err=True)
                 elif choice == 'log':
diff --git a/buildstream/_frontend/cli.py b/buildstream/_frontend/cli.py
index 74a529934..520624f59 100644
--- a/buildstream/_frontend/cli.py
+++ b/buildstream/_frontend/cli.py
@@ -255,7 +255,7 @@ def print_version(ctx, param, value):
 @click.option('--pull-buildtrees', is_flag=True, default=None,
               help="Include an element's build tree when pulling remote element artifacts")
 @click.option('--cache-buildtrees', default=None,
-              type=click.Choice(['always', 'failure', 'never']),
+              type=click.Choice(['always', 'auto', 'never']),
               help="Cache artifact build tree content on creation")
 @click.pass_context
 def cli(context, **kwargs):
@@ -555,9 +555,10 @@ def shell(app, element, sysroot, mount, isolate, build_, cli_buildtree, command)
         ]
 
         cached = element._cached_buildtree()
+        buildtree_exists = element._buildtree_exists()
         if cli_buildtree in ("always", "try"):
             use_buildtree = cli_buildtree
-            if not cached and use_buildtree == "always":
+            if not cached and buildtree_exists and use_buildtree == "always":
                 click.echo("WARNING: buildtree is not cached locally, will attempt to pull from available remotes",
                            err=True)
         else:
@@ -566,7 +567,7 @@ def shell(app, element, sysroot, mount, isolate, build_, cli_buildtree, command)
             if app.interactive and cli_buildtree == "ask":
                 if cached and bool(click.confirm('Do you want to use the cached buildtree?')):
                     use_buildtree = "always"
-                elif not cached:
+                elif buildtree_exists:
                     try:
                         choice = click.prompt("Do you want to pull & use a cached buildtree?",
                                               type=click.Choice(['try', 'always', 'never']),
diff --git a/buildstream/_stream.py b/buildstream/_stream.py
index f1600a8e9..4a21f5002 100644
--- a/buildstream/_stream.py
+++ b/buildstream/_stream.py
@@ -180,12 +180,17 @@ class Stream():
                     # Now check if the buildtree was successfully fetched
                     if element._cached_buildtree():
                         buildtree = True
+
                 if not buildtree:
+                    if element._buildtree_exists():
+                        message = "Buildtree is not cached locally or in available remotes"
+                    else:
+                        message = "Artifact was created without buildtree"
+
                     if usebuildtree == "always":
-                        raise StreamError("Buildtree is not cached locally or in available remotes")
+                        raise StreamError(message)
                     else:
-                        self._message(MessageType.INFO, """Buildtree is not cached locally or in available remotes,
-                                                        shell will be loaded without it""")
+                        self._message(MessageType.INFO, message + ", shell will be loaded without it")
             else:
                 buildtree = True
 
@@ -1420,7 +1425,7 @@ class Stream():
         for element in elements:
             # Check if element is partially cached without its buildtree, as the element
             # artifact may not be cached at all
-            if element._cached() and not element._cached_buildtree():
+            if element._cached() and not element._cached_buildtree() and element._buildtree_exists():
                 required_list.append(element)
 
         return required_list
diff --git a/buildstream/data/userconfig.yaml b/buildstream/data/userconfig.yaml
index d27e56ef2..34fd300d1 100644
--- a/buildstream/data/userconfig.yaml
+++ b/buildstream/data/userconfig.yaml
@@ -40,11 +40,14 @@ cache:
 
   # Whether to cache build trees on artifact creation:
   #
-  #  always - Always cache artifact build tree content
-  #  failure - Only cache build trees of failed builds
-  #  never - Don't cache artifact build tree content
+  #  always  - Always cache artifact build tree content
+  #  auto    - Only cache build trees when necessary, e.g., for failed builds
+  #  never   - Never cache artifact build tree content. This is not recommended
+  #            for normal users as this breaks core functionality such as
+  #            debugging failed builds and may break additional functionality
+  #            in future versions.
   #
-  cache-buildtrees: always
+  cache-buildtrees: auto
 
 
 #
diff --git a/buildstream/element.py b/buildstream/element.py
index 9ec65edaa..e5bc0792e 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -1724,7 +1724,13 @@ class Element(Plugin):
         cache_buildtrees = context.cache_buildtrees
         build_success = buildresult[0]
 
-        if cache_buildtrees == 'always' or (cache_buildtrees == 'failure' and not build_success):
+        # cache_buildtrees defaults to 'auto', only caching buildtrees
+        # when necessary, which includes failed builds.
+        # If only caching failed artifact buildtrees, then query the build
+        # result. Element types without a build-root dir will be cached
+        # with an empty buildtreedir regardless of this configuration.
+
+        if cache_buildtrees == 'always' or (cache_buildtrees == 'auto' and not build_success):
             try:
                 sandbox_build_dir = sandbox_vroot.descend(
                     *self.get_variable('build-root').lstrip(os.sep).split(os.sep))
@@ -1845,7 +1851,7 @@ class Element(Plugin):
 
         # Do not push elements that aren't cached, or that are cached with a dangling buildtree
         # ref unless element type is expected to have an an empty buildtree directory
-        if not self._cached_buildtree():
+        if not self._cached_buildtree() and self._buildtree_exists():
             return True
 
         # Do not push tainted artifact
@@ -2056,6 +2062,17 @@ class Element(Plugin):
     def _cached_buildtree(self):
         return self.__artifact.cached_buildtree()
 
+    # _buildtree_exists()
+    #
+    # Check if artifact was created with a buildtree. This does not check
+    # whether the buildtree is present in the local cache.
+    #
+    # Returns:
+    #     (bool): True if artifact was created with buildtree
+    #
+    def _buildtree_exists(self):
+        return self.__artifact.buildtree_exists()
+
     # _fetch()
     #
     # Fetch the element's sources.
diff --git a/tests/frontend/completions.py b/tests/frontend/completions.py
index 7810a06d5..6c737924d 100644
--- a/tests/frontend/completions.py
+++ b/tests/frontend/completions.py
@@ -158,7 +158,7 @@ def test_options(cli, cmd, word_idx, expected):
 
 @pytest.mark.parametrize("cmd,word_idx,expected", [
     ('bst --on-error ', 2, ['continue ', 'quit ', 'terminate ']),
-    ('bst --cache-buildtrees ', 2, ['always ', 'failure ', 'never ']),
+    ('bst --cache-buildtrees ', 2, ['always ', 'auto ', 'never ']),
     ('bst show --deps ', 3, ['all ', 'build ', 'none ', 'plan ', 'run ']),
     ('bst show --deps=', 2, ['all ', 'build ', 'none ', 'plan ', 'run ']),
     ('bst show --deps b', 3, ['build ']),
diff --git a/tests/integration/artifact.py b/tests/integration/artifact.py
index a375ee253..1f6028e4b 100644
--- a/tests/integration/artifact.py
+++ b/tests/integration/artifact.py
@@ -68,32 +68,29 @@ def test_cache_buildtrees(cli, tmpdir, datafiles):
             finally:
                 utils._force_rmtree(extractdir)
 
-        # Build autotools element with cache-buildtrees set via the
-        # cli. The artifact should be successfully pushed to the share1 remote
+        # Build autotools element with the default behavior of caching buildtrees
+        # only when necessary. The artifact should be successfully pushed to the share1 remote
         # and cached locally with an 'empty' buildtree digest, as it's not a
         # dangling ref
-        result = cli.run(project=project, args=['--cache-buildtrees', 'never', 'build', element_name])
+        result = cli.run(project=project, args=['build', element_name])
         assert result.exit_code == 0
         assert cli.get_element_state(project, element_name) == 'cached'
         assert share1.has_artifact('test', element_name, cli.get_element_key(project, element_name))
 
-        # The extracted buildtree dir should be empty, as we set the config
-        # to not cache buildtrees
+        # The buildtree dir should not exist, as we set the config to not cache buildtrees.
         cache_key = cli.get_element_key(project, element_name)
         elementdigest = share1.has_artifact('test', element_name, cache_key)
         with cas_extract_buildtree(elementdigest) as buildtreedir:
-            assert os.path.isdir(buildtreedir)
-            assert not os.listdir(buildtreedir)
+            assert not os.path.isdir(buildtreedir)
 
         # Delete the local cached artifacts, and assert the when pulled with --pull-buildtrees
-        # that is was cached in share1 as expected with an empty buildtree dir
+        # that is was cached in share1 as expected without a buildtree dir
         shutil.rmtree(os.path.join(str(tmpdir), 'cas'))
         assert cli.get_element_state(project, element_name) != 'cached'
         result = cli.run(project=project, args=['--pull-buildtrees', 'artifact', 'pull', element_name])
         assert element_name in result.get_pulled_elements()
         with cas_extract_buildtree(elementdigest) as buildtreedir:
-            assert os.path.isdir(buildtreedir)
-            assert not os.listdir(buildtreedir)
+            assert not os.path.isdir(buildtreedir)
         shutil.rmtree(os.path.join(str(tmpdir), 'cas'))
 
         # Assert that the default behaviour of pull to not include buildtrees on the artifact
@@ -106,13 +103,13 @@ def test_cache_buildtrees(cli, tmpdir, datafiles):
             assert not os.path.isdir(buildtreedir)
         shutil.rmtree(os.path.join(str(tmpdir), 'cas'))
 
-        # Repeat building the artifacts, this time with the default behaviour of caching buildtrees,
-        # as such the buildtree dir should not be empty
+        # Repeat building the artifacts, this time with cache-buildtrees set to
+        # 'always' via the cli, as such the buildtree dir should not be empty
         cli.configure({
             'artifacts': {'url': share2.repo, 'push': True},
             'cachedir': str(tmpdir)
         })
-        result = cli.run(project=project, args=['build', element_name])
+        result = cli.run(project=project, args=['--cache-buildtrees', 'always', 'build', element_name])
         assert result.exit_code == 0
         assert cli.get_element_state(project, element_name) == 'cached'
         assert share2.has_artifact('test', element_name, cli.get_element_key(project, element_name))
@@ -121,7 +118,7 @@ def test_cache_buildtrees(cli, tmpdir, datafiles):
         elementdigest = share2.has_artifact('test', element_name, cache_key)
         with cas_extract_buildtree(elementdigest) as buildtreedir:
             assert os.path.isdir(buildtreedir)
-            assert os.listdir(buildtreedir) is not None
+            assert os.listdir(buildtreedir)
 
         # Delete the local cached artifacts, and assert that when pulled with --pull-buildtrees
         # that it was cached in share2 as expected with a populated buildtree dir
@@ -131,7 +128,7 @@ def test_cache_buildtrees(cli, tmpdir, datafiles):
         assert element_name in result.get_pulled_elements()
         with cas_extract_buildtree(elementdigest) as buildtreedir:
             assert os.path.isdir(buildtreedir)
-            assert os.listdir(buildtreedir) is not None
+            assert os.listdir(buildtreedir)
         shutil.rmtree(os.path.join(str(tmpdir), 'cas'))
 
         # Clarify that the user config option for cache-buildtrees works as the cli
@@ -140,7 +137,7 @@ def test_cache_buildtrees(cli, tmpdir, datafiles):
         cli.configure({
             'artifacts': {'url': share3.repo, 'push': True},
             'cachedir': str(tmpdir),
-            'cache': {'cache-buildtrees': 'never'}
+            'cache': {'cache-buildtrees': 'always'}
         })
         result = cli.run(project=project, args=['build', element_name])
         assert result.exit_code == 0
@@ -149,4 +146,4 @@ def test_cache_buildtrees(cli, tmpdir, datafiles):
         elementdigest = share3.has_artifact('test', element_name, cache_key)
         with cas_extract_buildtree(elementdigest) as buildtreedir:
             assert os.path.isdir(buildtreedir)
-            assert not os.listdir(buildtreedir)
+            assert os.listdir(buildtreedir)
diff --git a/tests/integration/pullbuildtrees.py b/tests/integration/pullbuildtrees.py
index efcf9cf87..6a3b92723 100644
--- a/tests/integration/pullbuildtrees.py
+++ b/tests/integration/pullbuildtrees.py
@@ -48,6 +48,7 @@ def test_pullbuildtrees(cli2, tmpdir, datafiles):
         cli2.configure({
             'artifacts': {'url': share1.repo, 'push': True},
             'cachedir': str(tmpdir),
+            'cache': {'cache-buildtrees': 'always'},
         })
 
         @contextmanager
diff --git a/tests/integration/shellbuildtrees.py b/tests/integration/shellbuildtrees.py
index 13d21548e..fed3c5167 100644
--- a/tests/integration/shellbuildtrees.py
+++ b/tests/integration/shellbuildtrees.py
@@ -26,7 +26,7 @@ def test_buildtree_staged(cli_integration, datafiles):
     project = os.path.join(datafiles.dirname, datafiles.basename)
     element_name = 'build-shell/buildtree.bst'
 
-    res = cli_integration.run(project=project, args=['build', element_name])
+    res = cli_integration.run(project=project, args=['--cache-buildtrees', 'always', 'build', element_name])
     res.assert_success()
 
     res = cli_integration.run(project=project, args=[
@@ -42,7 +42,7 @@ def test_buildtree_staged_forced_true(cli_integration, datafiles):
     project = os.path.join(datafiles.dirname, datafiles.basename)
     element_name = 'build-shell/buildtree.bst'
 
-    res = cli_integration.run(project=project, args=['build', element_name])
+    res = cli_integration.run(project=project, args=['--cache-buildtrees', 'always', 'build', element_name])
     res.assert_success()
 
     res = cli_integration.run(project=project, args=[
@@ -65,14 +65,14 @@ def test_buildtree_staged_warn_empty_cached(cli_integration, tmpdir, datafiles):
         'cachedir': str(tmpdir)
     })
 
-    res = cli_integration.run(project=project, args=['--cache-buildtrees', 'never', 'build', element_name])
+    res = cli_integration.run(project=project, args=['build', element_name])
     res.assert_success()
 
     res = cli_integration.run(project=project, args=[
         'shell', '--build', '--use-buildtree', 'always', element_name, '--', 'cat', 'test'
     ])
-    res.assert_shell_error()
-    assert "Artifact contains an empty buildtree" in res.stderr
+    res.assert_main_error(ErrorDomain.APP, None)
+    assert "Artifact was created without buildtree" in res.stderr
 
 
 @pytest.mark.datafiles(DATA_DIR)
@@ -82,7 +82,7 @@ def test_buildtree_staged_if_available(cli_integration, datafiles):
     project = os.path.join(datafiles.dirname, datafiles.basename)
     element_name = 'build-shell/buildtree.bst'
 
-    res = cli_integration.run(project=project, args=['build', element_name])
+    res = cli_integration.run(project=project, args=['--cache-buildtrees', 'always', 'build', element_name])
     res.assert_success()
 
     res = cli_integration.run(project=project, args=[
@@ -99,7 +99,7 @@ def test_buildtree_staged_forced_false(cli_integration, datafiles):
     project = os.path.join(datafiles.dirname, datafiles.basename)
     element_name = 'build-shell/buildtree.bst'
 
-    res = cli_integration.run(project=project, args=['build', element_name])
+    res = cli_integration.run(project=project, args=['--cache-buildtrees', 'always', 'build', element_name])
     res.assert_success()
 
     res = cli_integration.run(project=project, args=[
@@ -148,25 +148,25 @@ def test_buildtree_from_failure_option_never(cli_integration, tmpdir, datafiles)
     res = cli_integration.run(project=project, args=[
         'shell', '--build', element_name, '--use-buildtree', 'always', '--', 'cat', 'test'
     ])
-    res.assert_shell_error()
-    assert "Artifact contains an empty buildtree" in res.stderr
+    res.assert_main_error(ErrorDomain.APP, None)
+    assert "Artifact was created without buildtree" in res.stderr
 
 
 @pytest.mark.datafiles(DATA_DIR)
 @pytest.mark.skipif(not HAVE_SANDBOX, reason='Only available with a functioning sandbox')
-def test_buildtree_from_failure_option_failure(cli_integration, tmpdir, datafiles):
+def test_buildtree_from_failure_option_always(cli_integration, tmpdir, datafiles):
 
     project = os.path.join(datafiles.dirname, datafiles.basename)
     element_name = 'build-shell/buildtree-fail.bst'
 
-    # build with  --cache-buildtrees set to 'failure', behaviour should match
+    # build with  --cache-buildtrees set to 'always', behaviour should match
     # default behaviour (which is always) as the buildtree will explicitly have been
     # cached with content.
     cli_integration.configure({
         'cachedir': str(tmpdir)
     })
 
-    res = cli_integration.run(project=project, args=['--cache-buildtrees', 'failure', 'build', element_name])
+    res = cli_integration.run(project=project, args=['--cache-buildtrees', 'always', 'build', element_name])
     res.assert_main_error(ErrorDomain.STREAM, None)
 
     res = cli_integration.run(project=project, args=[
@@ -190,7 +190,7 @@ def test_buildtree_pulled(cli, tmpdir, datafiles):
         cli.configure({
             'artifacts': {'url': share.repo, 'push': True}
         })
-        result = cli.run(project=project, args=['build', element_name])
+        result = cli.run(project=project, args=['--cache-buildtrees', 'always', 'build', element_name])
         result.assert_success()
         assert cli.get_element_state(project, element_name) == 'cached'
 
@@ -222,7 +222,7 @@ def test_buildtree_options(cli, tmpdir, datafiles):
         cli.configure({
             'artifacts': {'url': share.repo, 'push': True}
         })
-        result = cli.run(project=project, args=['build', element_name])
+        result = cli.run(project=project, args=['--cache-buildtrees', 'always', 'build', element_name])
         result.assert_success()
         assert cli.get_element_state(project, element_name) == 'cached'
         assert share.has_artifact('test', element_name, cli.get_element_key(project, element_name))