_frontend/cli.py: Make show() --use-buildtree respect pull semanticstpollard/shellbuildtree

Ensure that if a buildtree isn't cached locally, it's only fetched
if --pull & pull-buildtrees config are set. Also, only attempt to
fetch if it's plausible that it could be pulled, with appropriate
messaging based on local cached state.

2 files changed, 89 insertions, 30 deletions

diff --git a/src/buildstream/_frontend/cli.py b/src/buildstream/_frontend/cli.py
index fc501f09c..52848ca2e 100644
--- a/src/buildstream/_frontend/cli.py
+++ b/src/buildstream/_frontend/cli.py
@@ -633,7 +633,12 @@ def show(app, elements, deps, except_, order, format_):
     type=click.Choice(["ask", "try", "always", "never"]),
     default="ask",
     show_default=True,
-    help=("Use a buildtree. If `always` is set, will always fail to " "build if a buildtree is not available."),
+    help=(
+        "Stage a buildtree. If `always` is set, will always fail to "
+        "build if a buildtree is not available."
+        " --pull and pull-buildtrees configuration is needed "
+        "if trying to query for remotely cached buildtrees."
+    ),
 )
 @click.option("--pull", "pull_", is_flag=True, help="Attempt to pull missing or incomplete artifacts")
 @click.argument("element", required=False, type=click.Path(readable=False))
@@ -693,42 +698,76 @@ def shell(app, element, sysroot, mount, isolate, build_, cli_buildtree, pull_, c
         prompt = app.shell_prompt(element_name, element_key)
         mounts = [HostMount(path, host_path) for host_path, path in mount]
 
-        cached = element._cached_buildtree()
+        artifact_is_cached = element._cached()
+        buildtree_is_cached = element._cached_buildtree()
         buildtree_exists = element._buildtree_exists()
+        can_attempt_pull = app.context.pull_buildtrees and pull_
 
         if cli_buildtree in ("always", "try"):
-            if buildtree_exists or pull_:
+            if buildtree_is_cached:
                 use_buildtree = cli_buildtree
-                if not cached and use_buildtree == "always":
+            # If element is already cached, we can check the proto to see if the buildtree existed
+            elif artifact_is_cached:
+                if not buildtree_exists:
+                    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")
+                    click.echo(
+                        "WARNING: Artifact created without buildtree, shell will be loaded without it", err=True
+                    )
+                elif can_attempt_pull:
+                    use_buildtree = cli_buildtree
                     click.echo(
-                        "WARNING: buildtree is not cached locally, will attempt to pull from available remotes",
+                        "WARNING: buildtree is not cached locally but did exist, will attempt to pull from available remotes",
+                        err=True,
+                    )
+                else:
+                    if cli_buildtree == "always":
+                        # Exit early if it won't be possible to perform a fetch as pull semantics aren't present
+                        raise AppError(
+                            "Artifact has a buildtree but it isn't cached. Can be retried with --pull and pull-buildtrees configured"
+                        )
+                    click.echo("WARNING: buildtree is not cached locally, shell will be loaded without it", err=True)
+            # If element isn't cached at all, we can't check the proto to see if it existed so can't exit early
+            elif can_attempt_pull:
+                use_buildtree = cli_buildtree
+                if use_buildtree == "always":
+                    click.echo(
+                        "WARNING: Element is not cached so buildtree status unknown, 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")
-                click.echo("WARNING: Artifact created without buildtree, shell will be loaded without it", err=True)
+                    # Exit early as there is no buildtree locally & can_attempt_pull is False
+                    raise AppError(
+                        "Artifact not cached locally. Can be retried with --pull and pull-buildtrees configured"
+                    )
+                click.echo("WARNING: buildtree is not cached locally, 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
             if app.interactive and cli_buildtree == "ask":
-                if cached and bool(click.confirm("Do you want to use the cached buildtree?")):
+                if buildtree_is_cached and bool(click.confirm("Do you want to use the cached buildtree?")):
                     use_buildtree = "always"
-                elif buildtree_exists:
-                    try:
-                        choice = click.prompt(
-                            "Do you want to pull & use a cached buildtree?",
-                            type=click.Choice(["try", "always", "never"]),
-                            err=True,
-                            show_choices=True,
-                        )
-                    except click.Abort:
-                        click.echo("Aborting", err=True)
-                        sys.exit(-1)
-
-                    if choice != "never":
-                        use_buildtree = choice
+                elif can_attempt_pull:
+                    # If buildtree not cached, check if it's worth presenting the user a dialogue
+                    message = None
+                    if artifact_is_cached:
+                        if buildtree_exists:
+                            message = "Buildtree not cached but can be pulled if in available remotes, do you want to use it?"
+                    else:
+                        message = "Element is not cached so buildtree status unknown, do you want to pull and use it?"
+                    if message:
+                        try:
+                            choice = click.prompt(
+                                message, type=click.Choice(["try", "always", "never"]), err=True, show_choices=True,
+                            )
+                        except click.Abort:
+                            click.echo("Aborting", err=True)
+                            sys.exit(-1)
+
+                        if choice != "never":
+                            use_buildtree = choice
 
         # Raise warning if the element is cached in a failed state
         if use_buildtree and element._cached_failure():
diff --git a/tests/integration/shellbuildtrees.py b/tests/integration/shellbuildtrees.py
index d3191870a..a3e7da1c2 100644
--- a/tests/integration/shellbuildtrees.py
+++ b/tests/integration/shellbuildtrees.py
@@ -251,15 +251,17 @@ def test_buildtree_options(cli, tmpdir, datafiles):
 
         # Check correctly handling the lack of buildtree, with 'try' not attempting to
         # pull the buildtree as the user context is by default set to not pull them
+        # and --pull not given
         res = cli.run(
             project=project, args=["shell", "--build", element_name, "--use-buildtree", "try", "--", "cat", "test"]
         )
         assert "Hi" not in res.output
         assert "Attempting to fetch missing artifact buildtrees" not in res.stderr
+        assert "WARNING: buildtree is not cached locally, shell will be loaded without it" in res.stderr
 
         # 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
-        # available in the remote
+        # available in the remote and --pull given
         res = cli.run(
             project=project,
             args=[
@@ -267,6 +269,7 @@ def test_buildtree_options(cli, tmpdir, datafiles):
                 "shell",
                 "--build",
                 element_name,
+                "--pull",
                 "--use-buildtree",
                 "try",
                 "--",
@@ -281,19 +284,22 @@ def test_buildtree_options(cli, tmpdir, datafiles):
         assert cli.get_element_state(project, element_name) != "cached"
 
         # Check it's not loading the shell at all with always set for the buildtree, when the
-        # user context does not allow for buildtree pulling
+        # user context does not allow for buildtree pulling and --pull is not given
         result = cli.run(project=project, args=["artifact", "pull", "--deps", "all", element_name])
         result.assert_success()
         res = cli.run(
             project=project, args=["shell", "--build", element_name, "--use-buildtree", "always", "--", "cat", "test"]
         )
         res.assert_main_error(ErrorDomain.APP, None)
-        assert "Buildtree is not cached locally or in available remotes" in res.stderr
+        assert (
+            "Artifact has a buildtree but it isn't cached. Can be retried with --pull and pull-buildtrees configured"
+            in res.stderr
+        )
         assert "Hi" not in res.output
         assert "Attempting to fetch missing artifact buildtree" not in res.stderr
 
         # Check that when user context is set to pull buildtrees and a remote has the buildtree,
-        # 'always' will attempt and succeed at pulling the missing buildtree.
+        # 'always' will attempt and succeed at pulling the missing buildtree with --pull set.
         res = cli.run(
             project=project,
             args=[
@@ -301,6 +307,7 @@ def test_buildtree_options(cli, tmpdir, datafiles):
                 "shell",
                 "--build",
                 element_name,
+                "--pull",
                 "--use-buildtree",
                 "always",
                 "--",
@@ -309,7 +316,9 @@ def test_buildtree_options(cli, tmpdir, datafiles):
             ],
         )
         assert "Hi" in res.output
-        assert "buildtree is not cached locally, will attempt to pull from available remotes" in res.stderr
+        assert (
+            "buildtree is not cached locally but did exist, will attempt to pull from available remotes" in res.stderr
+        )
         assert "Attempting to fetch missing artifact buildtree" in res.stderr
 
 
@@ -332,7 +341,17 @@ def test_pull_buildtree_pulled(cli, tmpdir, datafiles):
         shutil.rmtree(str(os.path.join(str(tmpdir), "cache", "artifacts")))
         assert cli.get_element_state(project, element_name) != "cached"
 
-        # Check it's using the cached build tree
+        # Check it's not using the cached build tree, because --pull
+        # and pull-buildtrees were not both set
+        res = cli.run(
+            project=project,
+            args=["shell", "--build", element_name, "--pull", "--use-buildtree", "always", "--", "cat", "test",],
+        )
+        res.assert_main_error(ErrorDomain.APP, None)
+        assert "Artifact not cached locally. Can be retried with --pull and pull-buildtrees configured" in res.stderr
+
+        # Check it's using the cached build tree, because --pull
+        # and pull-buildtrees were both set
         res = cli.run(
             project=project,
             args=[
@@ -348,4 +367,5 @@ def test_pull_buildtree_pulled(cli, tmpdir, datafiles):
                 "test",
             ],
         )
-        res.assert_success()
+        result.assert_success()
+        assert "Hi" in res.output