[buildstream] 01/03: sandbox.py: Clarifications to the utility of Sandbox._disable_run()

[tv...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

tvb pushed a commit to branch tristan/light-cache-keys-without-sandbox-run
in repository https://gitbox.apache.org/repos/asf/buildstream.git

commit 86cd1496a7b24e47b307416cc48f785b8e334c25
Author: Tristan van Berkom <tr...@codethink.co.uk>
AuthorDate: Thu Aug 4 15:20:26 2022 +0900

    sandbox.py: Clarifications to the utility of Sandbox._disable_run()
    
    In the past, BuildStream had internal sandbox implementations which
    would require staging files on the local filesystem in order to run,
    but now we use buildbox-run exclusively and always use CasBasedDirectory.
    
    The _use_cas_based_directory() method was removed in the commit
    b487a11f6ee529c9cba505106d054c92c16e864c, and this was actually the
    only reason why we had the Element.BST_RUN_COMMANDS flag at the time.
    
    We still want BST_RUN_COMMANDS for other reasons, and it is useful
    to keep the Sandbox._disable_run() function, but it's only purpose
    now is to ensure the invariant that plugins who set BST_RUN_COMMANDS
    to False do not actually run commands.
---
 src/buildstream/element.py         |  9 +++++----
 src/buildstream/sandbox/sandbox.py | 17 +++++++++++++----
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 8f47a5b13..9a2b1bc2f 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -1677,11 +1677,12 @@ class Element(Plugin):
                 rootdir, output_file, output_file, self.__sandbox_config
             ) as sandbox:  # noqa
 
+                # Ensure that the plugin does not run commands if it said that it wouldn't
+                #
+                # We only disable commands here in _assemble() instead of __sandbox() because
+                # the user might still run a shell on an element which itself does not run commands.
+                #
                 if not self.BST_RUN_COMMANDS:
-                    # Element doesn't need to run any commands in the sandbox.
-                    #
-                    # Disable Sandbox.run() to allow CasBasedDirectory for all
-                    # sandboxes.
                     sandbox._disable_run()
 
                 # By default, the dynamic public data is the same as the static public data.
diff --git a/src/buildstream/sandbox/sandbox.py b/src/buildstream/sandbox/sandbox.py
index e30414bc1..a7cf9c777 100644
--- a/src/buildstream/sandbox/sandbox.py
+++ b/src/buildstream/sandbox/sandbox.py
@@ -64,6 +64,13 @@ class SandboxCommandError(SandboxError):
         self.collect = collect
 
 
+# An internal exception which can be used to explicitly trigger a bug / exception
+# which will be reported with a stack trace instead of reporting a user facing error
+#
+class _SandboxBug(Exception):
+    pass
+
+
 class Sandbox:
     """Sandbox()
 
@@ -345,7 +352,7 @@ class Sandbox:
         label: str = None
     ) -> Optional[int]:
         if not self.__allow_run:
-            raise SandboxError("Sandbox.run() has been disabled")
+            raise _SandboxBug("Element specified BST_RUN_COMMANDS as False but called Sandbox.run()")
 
         # Fallback to the sandbox default settings for
         # the cwd and env.
@@ -553,9 +560,11 @@ class Sandbox:
 
     # _disable_run()
     #
-    # Raise exception if `Sandbox.run()` is called. This enables use of
-    # CasBasedDirectory for faster staging when command execution is not
-    # required.
+    # Raise exception if `Sandbox.run()` is called.
+    #
+    # This enforces an invariant by raising an exception if an element
+    # plugin ever set BST_RUN_COMMANDS to False but then proceeded to
+    # attempt to run the sandbox at assemble time.
     #
     def _disable_run(self):
         self.__allow_run = False