_artifact.py: Do not create empty buildtree directories

Creating an empty buildtree directory when buildtree caching is disabled
means cached_buildtree() can return True even if no buildtree is
available, which breaks logic in the frontend and in Stream.

This adds the buildtree_exists() method to indicate whether an artifact
was created with a buildtree, independent of whether the buildtree is
available in the local cache.

6 files changed, 50 insertions, 21 deletions

diff --git a/buildstream/_artifact.py b/buildstream/_artifact.py
index 0c1e71194..a2600b995 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')
@@ -150,6 +149,7 @@ class Artifact():
         # 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 +227,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/_frontend/cli.py b/buildstream/_frontend/cli.py
index 74a529934..4974e2bfc 100644
--- a/buildstream/_frontend/cli.py
+++ b/buildstream/_frontend/cli.py
@@ -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 4a475d83f..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/element.py b/buildstream/element.py
index 9ec65edaa..43bbd6939 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -1845,7 +1845,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 +2056,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/integration/artifact.py b/tests/integration/artifact.py
index 35e7f56ae..98c6ce13f 100644
--- a/tests/integration/artifact.py
+++ b/tests/integration/artifact.py
@@ -77,23 +77,20 @@ def test_cache_buildtrees(cli, tmpdir, datafiles):
         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
@@ -148,5 +145,4 @@ def test_cache_buildtrees(cli, tmpdir, datafiles):
         cache_key = cli.get_element_key(project, element_name)
         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 not os.path.isdir(buildtreedir)
diff --git a/tests/integration/shellbuildtrees.py b/tests/integration/shellbuildtrees.py
index 13d21548e..31af8a0e4 100644
--- a/tests/integration/shellbuildtrees.py
+++ b/tests/integration/shellbuildtrees.py
@@ -71,8 +71,8 @@ def test_buildtree_staged_warn_empty_cached(cli_integration, tmpdir, datafiles):
     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)
@@ -148,8 +148,8 @@ 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)