_frontend/cli.py: Tweak 'try' & 'always' bst shell buildtree handling

If the cached Artifact wasn't originally generated with a buildtree,
then there's no need to attempt to find it in remotes by entering
Stream(). In interactive mode we only present the user the option to
attempt the pull with the above assumption on buildtree_exists, so
apply the same to applicable non interactive choices.

This also includes some tweaks in integration/shellbuildtrees.py
to reflect changes and cover cases that were missing.

3 files changed, 29 insertions, 17 deletions

diff --git a/src/buildstream/_frontend/cli.py b/src/buildstream/_frontend/cli.py
index cc8cd5e54..2301fcb78 100644
--- a/src/buildstream/_frontend/cli.py
+++ b/src/buildstream/_frontend/cli.py
@@ -559,11 +559,20 @@ 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 buildtree_exists and use_buildtree == "always":
-                click.echo("WARNING: buildtree is not cached locally, will attempt to pull from available remotes",
-                           err=True)
+            if buildtree_exists:
+                use_buildtree = cli_buildtree
+                if not cached and use_buildtree == "always":
+                    click.echo("WARNING: buildtree is not cached locally, will attempt to pull from available remotes",
+                               err=True)
+            else:
+                if cli_buildtree == "always":
+                    # Exit early if it won't be possible to even fetch a buildtree with always option
+                    raise AppError("Artifact was created without buildtree, unable to launch shell with it")
+                else:
+                    click.echo("WARNING: Artifact created without buildtree, shell will be loaded without it",
+                               err=True)
         else:
             # If the value has defaulted to ask and in non interactive mode, don't consider the buildtree, this
             # being the default behaviour of the command
diff --git a/src/buildstream/_stream.py b/src/buildstream/_stream.py
index 76a6ba1c0..0606c906a 100644
--- a/src/buildstream/_stream.py
+++ b/src/buildstream/_stream.py
@@ -180,11 +180,7 @@ class Stream():
                         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"
-
+                    message = "Buildtree is not cached locally or in available remotes"
                     if usebuildtree == "always":
                         raise StreamError(message)
                     else:
diff --git a/tests/integration/shellbuildtrees.py b/tests/integration/shellbuildtrees.py
index e53003757..b48f4afe7 100644
--- a/tests/integration/shellbuildtrees.py
+++ b/tests/integration/shellbuildtrees.py
@@ -59,13 +59,13 @@ def test_buildtree_staged_forced_true(cli_integration, datafiles):
 
 @pytest.mark.datafiles(DATA_DIR)
 @pytest.mark.skipif(not HAVE_SANDBOX, reason='Only available with a functioning sandbox')
-def test_buildtree_staged_warn_empty_cached(cli_integration, tmpdir, datafiles):
-    # Test that if we stage a cached and empty buildtree, we warn the user.
+def test_buildtree_staged_warn_non_cached(cli_integration, tmpdir, datafiles):
+    # Test that if we attempt to stage a buildtree that was never cached, we warn the user.
     project = str(datafiles)
     element_name = 'build-shell/buildtree.bst'
 
     # Switch to a temp artifact cache dir to ensure the artifact is rebuilt,
-    # caching an empty buildtree
+    # without caching a buildtree which is the default bst behaviour
     cli_integration.configure({
         'cachedir': str(tmpdir)
     })
@@ -77,7 +77,15 @@ def test_buildtree_staged_warn_empty_cached(cli_integration, tmpdir, datafiles):
         'shell', '--build', '--use-buildtree', 'always', element_name, '--', 'cat', 'test'
     ])
     res.assert_main_error(ErrorDomain.APP, None)
-    assert "Artifact was created without buildtree" in res.stderr
+    assert "Artifact was created without buildtree, unable to launch shell with it" in res.stderr
+
+    # Now attempt the same with the try option, this should not attempt to find a buildtree
+    # and just launch the shell, however the cat should still fail.
+    res = cli_integration.run(project=project, args=[
+        'shell', '--build', '--use-buildtree', 'try', element_name, '--', 'cat', 'test'
+    ])
+    assert "Artifact created without buildtree, shell will be loaded without it" in res.stderr
+    assert 'Hi' not in res.output
 
 
 @pytest.mark.datafiles(DATA_DIR)
@@ -142,7 +150,7 @@ def test_buildtree_from_failure_option_never(cli_integration, tmpdir, datafiles)
     element_name = 'build-shell/buildtree-fail.bst'
 
     # Switch to a temp artifact cache dir to ensure the artifact is rebuilt,
-    # caching an empty buildtree
+    # without caching a buildtree explicitly
     cli_integration.configure({
         'cachedir': str(tmpdir)
     })
@@ -154,7 +162,7 @@ def test_buildtree_from_failure_option_never(cli_integration, tmpdir, datafiles)
         'shell', '--build', element_name, '--use-buildtree', 'always', '--', 'cat', 'test'
     ])
     res.assert_main_error(ErrorDomain.APP, None)
-    assert "Artifact was created without buildtree" in res.stderr
+    assert "Artifact was created without buildtree, unable to launch shell with it" in res.stderr
 
 
 @pytest.mark.datafiles(DATA_DIR)
@@ -264,8 +272,6 @@ def test_buildtree_options(cli, tmpdir, datafiles):
         ])
         assert 'Hi' not in res.output
         assert 'Attempting to fetch missing artifact buildtrees' not in res.stderr
-        assert """Buildtree is not cached locally or in available remotes,
-                shell will be loaded without it"""
 
         # Check correctly handling the lack of buildtree, with 'try' attempting and succeeding
         # to pull the buildtree as the user context allow the pulling of buildtrees and it is
@@ -297,4 +303,5 @@ def test_buildtree_options(cli, tmpdir, datafiles):
             '--pull-buildtrees', 'shell', '--build', element_name, '--use-buildtree', 'always', '--', 'cat', 'test'
         ])
         assert 'Hi' in res.output
+        assert "buildtree is not cached locally, will attempt to pull from available remotes" in res.stderr
         assert 'Attempting to fetch missing artifact buildtree' in res.stderr