Merge branch 'tpollard/shellbuildtree' into 'master'

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

See merge request BuildStream/buildstream!1392

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