Merge branch 'juerg/platform' into 'master'

Improve sandbox configuration handling

See merge request BuildStream/buildstream!1845

13 files changed, 66 insertions, 66 deletions

diff --git a/src/buildstream/_platform/darwin.py b/src/buildstream/_platform/darwin.py
index 2e244557e..e880ea6f6 100644
--- a/src/buildstream/_platform/darwin.py
+++ b/src/buildstream/_platform/darwin.py
@@ -47,7 +47,7 @@ class Darwin(Platform):
 
     @staticmethod
     def _check_dummy_sandbox_config(config):
-        return True
+        pass
 
     @staticmethod
     def _create_dummy_sandbox(*args, **kwargs):
diff --git a/src/buildstream/_platform/fallback.py b/src/buildstream/_platform/fallback.py
index b9e9f520d..d80ac8fde 100644
--- a/src/buildstream/_platform/fallback.py
+++ b/src/buildstream/_platform/fallback.py
@@ -21,7 +21,7 @@ from .platform import Platform
 
 class Fallback(Platform):
     def _check_dummy_sandbox_config(self, config):
-        return True
+        pass
 
     def _create_dummy_sandbox(self, *args, **kwargs):
         kwargs["dummy_reason"] = (
diff --git a/src/buildstream/_platform/linux.py b/src/buildstream/_platform/linux.py
index c5192c86d..670cfc6b9 100644
--- a/src/buildstream/_platform/linux.py
+++ b/src/buildstream/_platform/linux.py
@@ -86,7 +86,7 @@ class Linux(Platform):
     def _check_sandbox_config_bwrap(self, config):
         from ..sandbox._sandboxbwrap import SandboxBwrap
 
-        return SandboxBwrap.check_sandbox_config(self, config)
+        SandboxBwrap.check_sandbox_config(self, config)
 
     def _create_bwrap_sandbox(self, *args, **kwargs):
         from ..sandbox._sandboxbwrap import SandboxBwrap
diff --git a/src/buildstream/_platform/platform.py b/src/buildstream/_platform/platform.py
index e0a0cf7ce..dddb52bd4 100644
--- a/src/buildstream/_platform/platform.py
+++ b/src/buildstream/_platform/platform.py
@@ -266,18 +266,18 @@ class Platform:
 
     # Buildbox run sandbox methods
     def _check_sandbox_config_buildboxrun(self, config):
-        from ..sandbox._sandboxbuildboxrun import SandboxBuildBoxRun
+        from ..sandbox._sandboxbuildboxrun import SandboxBuildBoxRun  # pylint: disable=cyclic-import
 
-        return SandboxBuildBoxRun.check_sandbox_config(self, config)
+        SandboxBuildBoxRun.check_sandbox_config(self, config)
 
     @staticmethod
     def _create_buildboxrun_sandbox(*args, **kwargs):
-        from ..sandbox._sandboxbuildboxrun import SandboxBuildBoxRun
+        from ..sandbox._sandboxbuildboxrun import SandboxBuildBoxRun  # pylint: disable=cyclic-import
 
         return SandboxBuildBoxRun(*args, **kwargs)
 
     def setup_buildboxrun_sandbox(self):
-        from ..sandbox._sandboxbuildboxrun import SandboxBuildBoxRun
+        from ..sandbox._sandboxbuildboxrun import SandboxBuildBoxRun  # pylint: disable=cyclic-import
 
         self._check_sandbox(SandboxBuildBoxRun)
         self.check_sandbox_config = self._check_sandbox_config_buildboxrun
diff --git a/src/buildstream/_platform/win32.py b/src/buildstream/_platform/win32.py
index a2529d8f6..33645e030 100644
--- a/src/buildstream/_platform/win32.py
+++ b/src/buildstream/_platform/win32.py
@@ -45,7 +45,7 @@ class Win32(Platform):
 
     @staticmethod
     def _check_dummy_sandbox_config(config):
-        return True
+        pass
 
     @staticmethod
     def _create_dummy_sandbox(*args, **kwargs):
diff --git a/src/buildstream/data/projectconfig.yaml b/src/buildstream/data/projectconfig.yaml
index d84edbf92..a2dc4ad9b 100644
--- a/src/buildstream/data/projectconfig.yaml
+++ b/src/buildstream/data/projectconfig.yaml
@@ -74,12 +74,8 @@ environment:
 environment-nocache: []
 
 # Configuration for the sandbox other than environment variables
-# should go in 'sandbox'. This just contains the UID and GID that
-# the user in the sandbox will have. Not all sandboxes will support
-# changing the values.
-sandbox:
-  build-uid: 0
-  build-gid: 0
+# should go in 'sandbox'.
+sandbox: {}
 
 # Defaults for the 'split-rules' public data found on elements
 # in the 'bst' domain.
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index f270bd8cc..06581b652 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -328,14 +328,6 @@ class Element(Plugin):
         # Extract Sandbox config
         self.__sandbox_config = self.__extract_sandbox_config(context, project, meta)
 
-        self.__sandbox_config_supported = True
-        if not self.__use_remote_execution():
-            platform = context.platform
-            if not platform.check_sandbox_config(self.__sandbox_config):
-                # Local sandbox does not fully support specified sandbox config.
-                # This will taint the artifact, disable pushing.
-                self.__sandbox_config_supported = False
-
     def __lt__(self, other):
         return self.name < other.name
 
@@ -1544,14 +1536,6 @@ class Element(Plugin):
         context = self._get_context()
         with self._output_file() as output_file:
 
-            if not self.__sandbox_config_supported:
-                self.warn(
-                    "Sandbox configuration is not supported by the platform.",
-                    detail="Falling back to UID {} GID {}. Artifact will not be pushed.".format(
-                        self.__sandbox_config.build_uid, self.__sandbox_config.build_gid
-                    ),
-                )
-
             # Explicitly clean it up, keep the build dir around if exceptions are raised
             os.makedirs(context.builddir, exist_ok=True)
 
@@ -2454,7 +2438,7 @@ class Element(Plugin):
             workspaced_dependencies = self.__artifact.get_metadata_workspaced_dependencies()
 
             # Other conditions should be or-ed
-            self.__tainted = workspaced or workspaced_dependencies or not self.__sandbox_config_supported
+            self.__tainted = workspaced or workspaced_dependencies
 
         return self.__tainted
 
@@ -2526,6 +2510,8 @@ class Element(Plugin):
             yield sandbox
 
         elif directory is not None and os.path.exists(directory):
+            platform = context.platform
+            platform.check_sandbox_config(config)
 
             sandbox = platform.create_sandbox(
                 context,
@@ -2701,7 +2687,7 @@ class Element(Plugin):
     @classmethod
     def __extract_sandbox_config(cls, context, project, meta):
         if meta.is_junction:
-            sandbox_config = Node.from_dict({"build-uid": 0, "build-gid": 0})
+            sandbox_config = Node.from_dict({})
         else:
             sandbox_config = project._sandbox.clone()
 
@@ -2734,7 +2720,7 @@ class Element(Plugin):
             build_arch = host_arch
 
         return SandboxConfig(
-            sandbox_config.get_int("build-uid"), sandbox_config.get_int("build-gid"), build_os, build_arch,
+            sandbox_config.get_int("build-uid", None), sandbox_config.get_int("build-gid", None), build_os, build_arch,
         )
 
     # This makes a special exception for the split rules, which
diff --git a/src/buildstream/node.pxd b/src/buildstream/node.pxd
index 47f46bbdf..c26d78a41 100644
--- a/src/buildstream/node.pxd
+++ b/src/buildstream/node.pxd
@@ -47,7 +47,7 @@ cdef class MappingNode(Node):
     # Public Methods
     cpdef bint get_bool(self, str key, default=*) except *
     cpdef object get_enum(self, str key, object constraint, object default=*)
-    cpdef int get_int(self, str key, default=*) except *
+    cpdef object get_int(self, str key, default=*)
     cpdef MappingNode get_mapping(self, str key, default=*)
     cpdef Node get_node(self, str key, list allowed_types=*, bint allow_none=*)
     cpdef ScalarNode get_scalar(self, str key, default=*)
diff --git a/src/buildstream/node.pyx b/src/buildstream/node.pyx
index adc8aa04e..e33a11753 100644
--- a/src/buildstream/node.pyx
+++ b/src/buildstream/node.pyx
@@ -593,7 +593,7 @@ cdef class MappingNode(Node):
 
         return (<ScalarNode> value).as_enum(constraint)
 
-    cpdef int get_int(self, str key, object default=_sentinel) except *:
+    cpdef object get_int(self, str key, object default=_sentinel):
         """get_int(key, default=sentinel)
 
         Get the value of the node for `key` as an integer.
@@ -602,7 +602,7 @@ cdef class MappingNode(Node):
 
         Args:
             key (str): key for which to get the value
-            default (int): default value to return if `key` is not in the mapping
+            default (int, None): default value to return if `key` is not in the mapping
 
         Raises:
            :class:`buildstream._exceptions.LoadError`: if the value at `key` is not a
@@ -610,9 +610,11 @@ cdef class MappingNode(Node):
                                                        valid `integer`
 
         Returns:
-            :class:`int`: the value at `key` or the default
+            :class:`int` or :class:`None`: the value at `key` or the default
         """
         cdef ScalarNode scalar = self.get_scalar(key, default)
+        if default is None and scalar.is_none():
+            return None
         return scalar.as_int()
 
     cpdef MappingNode get_mapping(self, str key, object default=_sentinel):
diff --git a/src/buildstream/sandbox/_config.py b/src/buildstream/sandbox/_config.py
index 614f22063..7a71e7d50 100644
--- a/src/buildstream/sandbox/_config.py
+++ b/src/buildstream/sandbox/_config.py
@@ -39,21 +39,12 @@ class SandboxConfig:
     #
     def get_unique_key(self):
 
-        # Currently operating system and machine architecture
-        # are not configurable and we have no sandbox implementation
-        # which can conform to such configurations.
-        #
-        # However this should be the right place to support
-        # such configurations in the future.
-        #
         unique_key = {"os": self.build_os, "arch": self.build_arch}
 
-        # Avoid breaking cache key calculation with
-        # the addition of configurabuild build uid/gid
-        if self.build_uid != 0:
+        if self.build_uid is not None:
             unique_key["build-uid"] = self.build_uid
 
-        if self.build_gid != 0:
+        if self.build_gid is not None:
             unique_key["build-gid"] = self.build_gid
 
         return unique_key
diff --git a/src/buildstream/sandbox/_sandboxbuildboxrun.py b/src/buildstream/sandbox/_sandboxbuildboxrun.py
index f6ecbeaa0..246fdd450 100644
--- a/src/buildstream/sandbox/_sandboxbuildboxrun.py
+++ b/src/buildstream/sandbox/_sandboxbuildboxrun.py
@@ -26,6 +26,7 @@ from .. import utils, _signals
 from . import SandboxFlags
 from .._exceptions import SandboxError
 from .._message import Message, MessageType
+from .._platform import Platform
 from .._protos.build.bazel.remote.execution.v2 import remote_execution_pb2
 from ._sandboxreapi import SandboxREAPI
 
@@ -59,15 +60,35 @@ class SandboxBuildBoxRun(SandboxREAPI):
             cls._dummy_reasons += ["buildbox-run: {}".format(output)]
             raise SandboxError(" and ".join(cls._dummy_reasons), reason="unavailable-local-sandbox")
 
+        osfamily_prefix = "platform:OSFamily="
+        cls._osfamilies = {cap[len(osfamily_prefix) :] for cap in cls._capabilities if cap.startswith(osfamily_prefix)}
+        if not cls._osfamilies:
+            # buildbox-run is too old to list supported OS families,
+            # limit support to native building on the host OS.
+            cls._osfamilies.add(Platform.get_host_os())
+
+        isa_prefix = "platform:ISA="
+        cls._isas = {cap[len(isa_prefix) :] for cap in cls._capabilities if cap.startswith(isa_prefix)}
+        if not cls._isas:
+            # buildbox-run is too old to list supported ISAs,
+            # limit support to native building on the host ISA.
+            cls._isas.add(Platform.get_host_arch())
+
     @classmethod
     def check_sandbox_config(cls, platform, config):
-        # Check host os and architecture match
-        if config.build_os != platform.get_host_os():
-            raise SandboxError("Configured and host OS don't match.")
-        if config.build_arch != platform.get_host_arch():
-            raise SandboxError("Configured and host architecture don't match.")
-
-        return True
+        if platform.does_multiprocessing_start_require_pickling():
+            # Reinitialize class as class data is not pickled.
+            cls.check_available()
+
+        if config.build_os not in cls._osfamilies:
+            raise SandboxError("OS '{}' is not supported by buildbox-run.".format(config.build_os))
+        if config.build_arch not in cls._isas:
+            raise SandboxError("ISA '{}' is not supported by buildbox-run.".format(config.build_arch))
+
+        if config.build_uid is not None and "platform:unixUID" not in cls._capabilities:
+            raise SandboxError("Configuring sandbox UID is not supported by buildbox-run.")
+        if config.build_gid is not None and "platform:unixGID" not in cls._capabilities:
+            raise SandboxError("Configuring sandbox GID is not supported by buildbox-run.")
 
     def _execute_action(self, action, flags):
         stdout, stderr = self._get_output()
diff --git a/src/buildstream/sandbox/_sandboxbwrap.py b/src/buildstream/sandbox/_sandboxbwrap.py
index 433b0f754..216145938 100644
--- a/src/buildstream/sandbox/_sandboxbwrap.py
+++ b/src/buildstream/sandbox/_sandboxbwrap.py
@@ -107,13 +107,17 @@ class SandboxBwrap(Sandbox):
 
     @classmethod
     def check_sandbox_config(cls, local_platform, config):
-        if cls.user_ns_available:
-            # User namespace support allows arbitrary build UID/GID settings.
-            pass
-        elif config.build_uid != local_platform._uid or config.build_gid != local_platform._gid:
+        if local_platform.does_multiprocessing_start_require_pickling():
+            # Reinitialize class as class data is not pickled.
+            cls.check_available()
+
+        if not cls.user_ns_available:
             # Without user namespace support, the UID/GID in the sandbox
             # will match the host UID/GID.
-            return False
+            if config.build_uid is not None and config.build_uid != local_platform._uid:
+                raise SandboxError("Configured and host UID don't match and user namespace is not supported.")
+            if config.build_gid is not None and config.build_gid != local_platform._gid:
+                raise SandboxError("Configured and host UID don't match and user namespace is not supported.")
 
         host_os = local_platform.get_host_os()
         host_arch = local_platform.get_host_arch()
@@ -122,8 +126,6 @@ class SandboxBwrap(Sandbox):
         if config.build_arch != host_arch and not local_platform.can_crossbuild(config):
             raise SandboxError("Configured architecture and host architecture don't match.")
 
-        return True
-
     def _run(self, command, flags, *, cwd, env):
         stdout, stderr = self._get_output()
 
@@ -230,8 +232,8 @@ class SandboxBwrap(Sandbox):
         if self.user_ns_available:
             bwrap_command += ["--unshare-user"]
             if not flags & SandboxFlags.INHERIT_UID:
-                uid = self._get_config().build_uid
-                gid = self._get_config().build_gid
+                uid = self._get_config().build_uid or 0
+                gid = self._get_config().build_gid or 0
                 bwrap_command += ["--uid", str(uid), "--gid", str(gid)]
 
         with ExitStack() as stack:
diff --git a/src/buildstream/sandbox/_sandboxreapi.py b/src/buildstream/sandbox/_sandboxreapi.py
index 9d8c22f0b..43d00a357 100644
--- a/src/buildstream/sandbox/_sandboxreapi.py
+++ b/src/buildstream/sandbox/_sandboxreapi.py
@@ -136,8 +136,10 @@ class SandboxREAPI(Sandbox):
         else:
             uid = config.build_uid
             gid = config.build_gid
-        platform_dict["unixUID"] = str(uid)
-        platform_dict["unixGID"] = str(gid)
+        if uid is not None:
+            platform_dict["unixUID"] = str(uid)
+        if gid is not None:
+            platform_dict["unixGID"] = str(gid)
 
         if flags & SandboxFlags.NETWORK_ENABLED:
             platform_dict["network"] = "on"