Merge branch 'juerg/shell-sysroot' into 'master'

Remove bst shell --sysroot

See merge request BuildStream/buildstream!1882

7 files changed, 49 insertions, 130 deletions

diff --git a/NEWS b/NEWS
index 84a7a6bf7..4b453b0e9 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,13 @@ Core
   o BREAKING CHANGE: "format-version" is removed and replaced with "min-version",
     which is now required to be specified in project.conf.
 
+CLI
+---
+
+  o BREAKING CHANGE: `bst shell --sysroot` has been removed. This is no longer
+    needed now that we support caching buildtrees in CAS.
+
+
 ==================
 buildstream 1.93.1
 ==================
diff --git a/man/bst-shell.1 b/man/bst-shell.1
index 7acfc4616..ff6179a52 100644
--- a/man/bst-shell.1
+++ b/man/bst-shell.1
@@ -23,10 +23,6 @@ otherwise bst may respond to them instead. e.g.
 Use the --build option to create a temporary sysroot for
 building the element instead.
 .PP
-Use the --sysroot option with an existing failed build
-directory or with a checkout of the given target, in order
-to use a specific sysroot.
-.PP
 If no COMMAND is specified, the default is to attempt
 to run an interactive shell.
 .SH OPTIONS
@@ -34,9 +30,6 @@ to run an interactive shell.
 \fB\-b,\fP \-\-build
 Stage dependencies and sources to build
 .TP
-\fB\-s,\fP \-\-sysroot DIRECTORY
-An existing sysroot
-.TP
 \fB\-\-mount\fP HOSTPATH PATH
 Mount a file or directory into the sandbox
 .TP
diff --git a/src/buildstream/_frontend/cli.py b/src/buildstream/_frontend/cli.py
index 522f15115..e8e2112aa 100644
--- a/src/buildstream/_frontend/cli.py
+++ b/src/buildstream/_frontend/cli.py
@@ -618,13 +618,6 @@ def show(app, elements, deps, except_, order, format_):
 @cli.command(short_help="Shell into an element's sandbox environment")
 @click.option("--build", "-b", "build_", is_flag=True, help="Stage dependencies and sources to build")
 @click.option(
-    "--sysroot",
-    "-s",
-    default=None,
-    type=click.Path(exists=True, file_okay=False, readable=True),
-    help="An existing sysroot",
-)
-@click.option(
     "--mount",
     type=click.Tuple([click.Path(exists=True), str]),
     multiple=True,
@@ -650,7 +643,7 @@ def show(app, elements, deps, except_, order, format_):
 @click.argument("element", required=False, type=click.Path(readable=False))
 @click.argument("command", type=click.STRING, nargs=-1)
 @click.pass_obj
-def shell(app, element, sysroot, mount, isolate, build_, cli_buildtree, pull_, command):
+def shell(app, element, mount, isolate, build_, cli_buildtree, pull_, command):
     """Run a command in the target element's sandbox environment
 
     When this command is executed from a workspace directory, the default
@@ -669,10 +662,6 @@ def shell(app, element, sysroot, mount, isolate, build_, cli_buildtree, pull_, c
     Use the --build option to create a temporary sysroot for
     building the element instead.
 
-    Use the --sysroot option with an existing failed build
-    directory or with a checkout of the given target, in order
-    to use a specific sysroot.
-
     If no COMMAND is specified, the default is to attempt
     to run an interactive shell.
     """
@@ -784,7 +773,6 @@ def shell(app, element, sysroot, mount, isolate, build_, cli_buildtree, pull_, c
                 element,
                 scope,
                 prompt,
-                directory=sysroot,
                 mounts=mounts,
                 isolate=isolate,
                 command=command,
diff --git a/src/buildstream/_stream.py b/src/buildstream/_stream.py
index 92b9f5113..09e6dfb17 100644
--- a/src/buildstream/_stream.py
+++ b/src/buildstream/_stream.py
@@ -177,7 +177,6 @@ class Stream:
     #    element (Element): An Element object to run the shell for
     #    scope (Scope): The scope for the shell (Scope.BUILD or Scope.RUN)
     #    prompt (str): The prompt to display in the shell
-    #    directory (str): A directory where an existing prestaged sysroot is expected, or None
     #    mounts (list of HostMount): Additional directories to mount into the sandbox
     #    isolate (bool): Whether to isolate the environment like we do in builds
     #    command (list): An argv to launch in the sandbox, or None
@@ -194,7 +193,6 @@ class Stream:
         scope,
         prompt,
         *,
-        directory=None,
         mounts=None,
         isolate=False,
         command=None,
@@ -207,30 +205,26 @@ class Stream:
         if unique_id and element is None:
             element = Plugin._lookup(unique_id)
 
-        # Assert we have everything we need built, unless the directory is specified
-        # in which case we just blindly trust the directory, using the element
+        # Assert we have everything we need built, using the element
         # definitions to control the execution environment only.
-        if directory is None:
+        if scope == Scope.BUILD and not element._has_all_sources_in_source_cache():
+            raise StreamError(
+                "Sources for element {} are not cached." "Element must be fetched.".format(element._get_full_name())
+            )
 
-            if not element._has_all_sources_in_source_cache():
+        missing_deps = [dep for dep in self._pipeline.dependencies([element], scope) if not dep._cached()]
+        if missing_deps:
+            if not pull_dependencies:
                 raise StreamError(
-                    "Sources for element {} are not cached."
-                    "Element must be fetched.".format(element._get_full_name())
+                    "Elements need to be built or downloaded before staging a shell environment",
+                    detail="\n".join(list(map(lambda x: x._get_full_name(), missing_deps))),
                 )
-
-            missing_deps = [dep for dep in self._pipeline.dependencies([element], scope) if not dep._cached()]
-            if missing_deps:
-                if not pull_dependencies:
-                    raise StreamError(
-                        "Elements need to be built or downloaded before staging a shell environment",
-                        detail="\n".join(list(map(lambda x: x._get_full_name(), missing_deps))),
-                    )
-                self._message(MessageType.INFO, "Attempting to fetch missing or incomplete artifacts")
-                self._scheduler.clear_queues()
-                self._add_queue(PullQueue(self._scheduler))
-                plan = self._pipeline.add_elements([element], missing_deps)
-                self._enqueue_plan(plan)
-                self._run()
+            self._message(MessageType.INFO, "Attempting to fetch missing or incomplete artifacts")
+            self._scheduler.clear_queues()
+            self._add_queue(PullQueue(self._scheduler))
+            plan = self._pipeline.add_elements([element], missing_deps)
+            self._enqueue_plan(plan)
+            self._run()
 
         buildtree = False
         # Check if we require a pull queue attempt, with given artifact state and context
@@ -258,7 +252,7 @@ class Stream:
                 buildtree = True
 
         return element._shell(
-            scope, directory, mounts=mounts, isolate=isolate, prompt=prompt, command=command, usebuildtree=buildtree
+            scope, mounts=mounts, isolate=isolate, prompt=prompt, command=command, usebuildtree=buildtree
         )
 
     # build()
@@ -560,7 +554,7 @@ class Stream:
                 _PipelineSelection.NONE: Scope.NONE,
                 _PipelineSelection.ALL: Scope.ALL,
             }
-            with target._prepare_sandbox(scope=scope[selection], directory=None, integrate=integrate) as sandbox:
+            with target._prepare_sandbox(scope=scope[selection], integrate=integrate) as sandbox:
                 # Copy or move the sandbox to the target directory
                 virdir = sandbox.get_virtual_directory()
                 self._export_artifact(tar, location, compression, target, hardlinks, virdir)
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 6cce9b93f..8563eee3c 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -1265,32 +1265,28 @@ class Element(Plugin):
     # is used to stage things by the `bst artifact checkout` codepath
     #
     @contextmanager
-    def _prepare_sandbox(self, scope, directory, shell=False, integrate=True, usebuildtree=False):
+    def _prepare_sandbox(self, scope, shell=False, integrate=True, usebuildtree=False):
         # bst shell and bst artifact checkout require a local sandbox.
-        bare_directory = bool(directory)
-        with self.__sandbox(
-            directory, config=self.__sandbox_config, allow_remote=False, bare_directory=bare_directory
-        ) as sandbox:
+        with self.__sandbox(None, config=self.__sandbox_config, allow_remote=False) as sandbox:
             sandbox._usebuildtree = usebuildtree
 
             # Configure always comes first, and we need it.
             self.__configure_sandbox(sandbox)
 
-            # Stage something if we need it
-            if not directory:
-                if shell and scope == Scope.BUILD:
-                    self.stage(sandbox)
-                else:
-                    # Stage deps in the sandbox root
-                    with self.timed_activity("Staging dependencies", silent_nested=True):
-                        self.stage_dependency_artifacts(sandbox, scope)
+            # Stage what we need
+            if shell and scope == Scope.BUILD:
+                self.stage(sandbox)
+            else:
+                # Stage deps in the sandbox root
+                with self.timed_activity("Staging dependencies", silent_nested=True):
+                    self.stage_dependency_artifacts(sandbox, scope)
 
-                    # Run any integration commands provided by the dependencies
-                    # once they are all staged and ready
-                    if integrate:
-                        with self.timed_activity("Integrating sandbox"):
-                            for dep in self.dependencies(scope):
-                                dep.integrate(sandbox)
+                # Run any integration commands provided by the dependencies
+                # once they are all staged and ready
+                if integrate:
+                    with self.timed_activity("Integrating sandbox"):
+                        for dep in self.dependencies(scope):
+                            dep.integrate(sandbox)
 
             yield sandbox
 
@@ -1808,7 +1804,6 @@ class Element(Plugin):
     #
     # Args:
     #    scope (Scope): Either BUILD or RUN scopes are valid, or None
-    #    directory (str): A directory to an existing sandbox, or None
     #    mounts (list): A list of (str, str) tuples, representing host/target paths to mount
     #    isolate (bool): Whether to isolate the environment like we do in builds
     #    prompt (str): A suitable prompt string for PS1
@@ -1816,13 +1811,9 @@ class Element(Plugin):
     #    usebuildtree (bool): Use the buildtree as its source
     #
     # Returns: Exit code
-    #
-    # If directory is not specified, one will be staged using scope
-    def _shell(
-        self, scope=None, directory=None, *, mounts=None, isolate=False, prompt=None, command=None, usebuildtree=False
-    ):
+    def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command=None, usebuildtree=False):
 
-        with self._prepare_sandbox(scope, directory, shell=True, usebuildtree=usebuildtree) as sandbox:
+        with self._prepare_sandbox(scope, shell=True, usebuildtree=usebuildtree) as sandbox:
             environment = self.get_environment()
             environment = copy.copy(environment)
             flags = SandboxFlags.INTERACTIVE | SandboxFlags.ROOT_READ_ONLY
@@ -2448,14 +2439,12 @@ class Element(Plugin):
     #    stderr (fileobject): The stream for stderr for the sandbox
     #    config (SandboxConfig): The SandboxConfig object
     #    allow_remote (bool): Whether the sandbox is allowed to be remote
-    #    bare_directory (bool): Whether the directory is bare i.e. doesn't have
-    #                           a separate 'root' subdir
     #
     # Yields:
     #    (Sandbox): A usable sandbox
     #
     @contextmanager
-    def __sandbox(self, directory, stdout=None, stderr=None, config=None, allow_remote=True, bare_directory=False):
+    def __sandbox(self, directory, stdout=None, stderr=None, config=None, allow_remote=True):
         context = self._get_context()
         project = self._get_project()
         platform = context.platform
@@ -2488,7 +2477,6 @@ class Element(Plugin):
                 stderr=stderr,
                 config=config,
                 specs=self.__remote_execution_specs,
-                bare_directory=bare_directory,
                 allow_real_directory=False,
                 output_files_required=output_files_required,
                 output_node_properties=output_node_properties,
@@ -2507,7 +2495,6 @@ class Element(Plugin):
                 stdout=stdout,
                 stderr=stderr,
                 config=config,
-                bare_directory=bare_directory,
                 allow_real_directory=not self.BST_VIRTUAL_DIRECTORY,
                 output_node_properties=output_node_properties,
             )
@@ -2520,7 +2507,7 @@ class Element(Plugin):
             with utils._tempdir(
                 prefix="{}-".format(self.normal_name), dir=context.builddir
             ) as rootdir, self.__sandbox(
-                rootdir, stdout=stdout, stderr=stderr, config=config, allow_remote=allow_remote, bare_directory=False
+                rootdir, stdout=stdout, stderr=stderr, config=config, allow_remote=allow_remote
             ) as sandbox:
                 yield sandbox
 
diff --git a/src/buildstream/sandbox/sandbox.py b/src/buildstream/sandbox/sandbox.py
index bba051704..0a09788f4 100644
--- a/src/buildstream/sandbox/sandbox.py
+++ b/src/buildstream/sandbox/sandbox.py
@@ -139,21 +139,14 @@ class Sandbox:
         self.__config = kwargs["config"]
         self.__stdout = kwargs["stdout"]
         self.__stderr = kwargs["stderr"]
-        self.__bare_directory = kwargs["bare_directory"]
 
         # Setup the directories. Root and output_directory should be
         # available to subclasses, hence being single-underscore. The
         # others are private to this class.
-        # If the directory is bare, it probably doesn't need scratch
-        if self.__bare_directory:
-            self._root = directory
-            self.__scratch = None
-            os.makedirs(self._root, exist_ok=True)
-        else:
-            self._root = os.path.join(directory, "root")
-            self.__scratch = os.path.join(directory, "scratch")
-            for directory_ in [self._root, self.__scratch]:
-                os.makedirs(directory_, exist_ok=True)
+        self._root = os.path.join(directory, "root")
+        self.__scratch = os.path.join(directory, "scratch")
+        for directory_ in [self._root, self.__scratch]:
+            os.makedirs(directory_, exist_ok=True)
 
         self._output_directory = None  # type: Optional[str]
         self._build_directory = None
@@ -529,7 +522,6 @@ class Sandbox:
     # Returns:
     #    (str): The sandbox scratch directory
     def _get_scratch_directory(self):
-        assert not self.__bare_directory, "Scratch is not going to work with bare directories"
         return self.__scratch
 
     # _get_output()
diff --git a/tests/integration/shell.py b/tests/integration/shell.py
index 2aa386730..7fa742f41 100644
--- a/tests/integration/shell.py
+++ b/tests/integration/shell.py
@@ -307,48 +307,6 @@ def test_workspace_visible(cli, datafiles):
     assert result.output == workspace_hello
 
 
-# Test that '--sysroot' works
-@pytest.mark.datafiles(DATA_DIR)
-@pytest.mark.skipif(not HAVE_SANDBOX, reason="Only available with a functioning sandbox")
-@pytest.mark.xfail(HAVE_SANDBOX == "buildbox-run", reason="Not working with BuildBox")
-def test_sysroot(cli, tmpdir, datafiles):
-    project = str(datafiles)
-    base_element = "base/base-alpine.bst"
-    # test element only needs to be something lightweight for this test
-    test_element = "script/script.bst"
-    checkout_dir = os.path.join(str(tmpdir), "alpine-sysroot")
-    test_file = "hello"
-
-    # Build and check out a sysroot
-    res = cli.run(project=project, args=["build", base_element])
-    res.assert_success()
-    res = cli.run(project=project, args=["artifact", "checkout", base_element, "--directory", checkout_dir])
-    res.assert_success()
-
-    # Mutate the sysroot
-    test_path = os.path.join(checkout_dir, test_file)
-    with open(test_path, "w") as f:
-        f.write("hello\n")
-
-    # Shell into the sysroot and check the test file exists
-    res = cli.run(
-        project=project,
-        args=[
-            "shell",
-            "--build",
-            "--sysroot",
-            checkout_dir,
-            test_element,
-            "--",
-            "grep",
-            "-q",
-            "hello",
-            "/" + test_file,
-        ],
-    )
-    res.assert_success()
-
-
 # Test system integration commands can access devices in /dev
 @pytest.mark.datafiles(DATA_DIR)
 @pytest.mark.skipif(not HAVE_SANDBOX, reason="Only available with a functioning sandbox")