Remove 'default-platform-properties' and reduce scopetpollard/custom-platform-properties

12 files changed, 118 insertions, 88 deletions

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index cfc97562c..457f6bf42 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -193,9 +193,9 @@ tests-remote-execution:
     REMOTE_EXECUTION_SERVICE: http://docker:50051
     SOURCE_CACHE_SERVICE: http://docker:50052
     PYTEST_ARGS: "--color=yes --remote-execution"
-    DEFAULT_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64" # Default properties, substitued in manifest
+    PLATFORM_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64" # Default properties, substitued in manifest
 
-tests-remote-execution-custom-property:
+tests-remote-execution-extra-property:
   <<: *tests
   <<: *remote-test # Spin up server stack
   variables:
@@ -205,10 +205,9 @@ tests-remote-execution-custom-property:
     REMOTE_EXECUTION_SERVICE: http://docker:50051
     SOURCE_CACHE_SERVICE: http://docker:50052
     PYTEST_ARGS: "--color=yes --remote-execution"
-    DEFAULT_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64"
-    CUSTOM_PROPERTIES: "--platform foo=bar" # Custom property, substituted in manifest. This extends defaults
+    PLATFORM_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64 --platform foo=bar" # Custom property 'foo' added
 
-tests-remote-execution-no-default-properties:
+tests-remote-execution-no-default-iso:
   <<: *tests
   <<: *remote-test # Spin up server stack
   variables:
@@ -218,7 +217,7 @@ tests-remote-execution-no-default-properties:
     REMOTE_EXECUTION_SERVICE: http://docker:50051
     SOURCE_CACHE_SERVICE: http://docker:50052
     PYTEST_ARGS: "--color=yes --remote-execution"
-    CUSTOM_PROPERTIES: "--platform OSFamily=linux --platform foo=bar" # No DEFAULT_PROPERTIES set, so these become the *only* properties (default-platform-properties: False)
+    PLATFORM_PROPERTIES: "--platform OSFamily=linux --platform foo=bar" # Default property ISA not set, so BST should disable (via ISA: [] set in test suite)
 
 tests-remote-cache:
   <<: *tests
diff --git a/.gitlab-ci/buildgrid-remote-execution.yml b/.gitlab-ci/buildgrid-remote-execution.yml
index 2bc76eb6e..bef75b57b 100644
--- a/.gitlab-ci/buildgrid-remote-execution.yml
+++ b/.gitlab-ci/buildgrid-remote-execution.yml
@@ -42,7 +42,7 @@ services:
   bot:
     image: registry.gitlab.com/buildgrid/buildgrid.hub.docker.com/buildbox:nightly
     command: [
-      "sh", "-c", "sleep 15 && ( buildbox-casd --cas-remote=http://controller:50051 /var/lib/buildgrid/cache & buildbox-worker --bots-remote=http://controller:50051 --cas-remote=unix:/var/lib/buildgrid/cache/casd.sock --buildbox-run=buildbox-run-bubblewrap --runner-arg=--use-localcas ${DEFAULT_PROPERTIES} ${CUSTOM_PROPERTIES} --verbose )"]
+      "sh", "-c", "sleep 15 && ( buildbox-casd --cas-remote=http://controller:50051 /var/lib/buildgrid/cache & buildbox-worker --bots-remote=http://controller:50051 --cas-remote=unix:/var/lib/buildgrid/cache/casd.sock --buildbox-run=buildbox-run-bubblewrap --runner-arg=--use-localcas ${PLATFORM_PROPERTIES} --verbose )"]
     privileged: true
     volumes:
       - type: volume
diff --git a/doc/source/format_project.rst b/doc/source/format_project.rst
index b0b6fc191..e11a396e1 100644
--- a/doc/source/format_project.rst
+++ b/doc/source/format_project.rst
@@ -327,9 +327,8 @@ using the `remote-execution` option:
     action-cache-service:
       url: http://bar.action.com:50052
       instance-name: development-emea-1
-    custom-platform-properties:
+    platform-properties:
       docker: docker://marketplace.gcr.io/google/rbe-ubuntu16-04
-    default-platform-properties: True
 
 
 storage-service specifies a remote CAS store and the parameters are the
@@ -351,15 +350,12 @@ name should be given to you by the service provider of each
 service. Not all remote execution and storage services support
 instance names.
 
-The custom-platform-properties is optional, properties can be be provided as
-key: value pairs and are included with the default properties. Pre-emptive
-compatability filtering isn't applied, and default values take precedence
-unless explicitly disabled.
-
-default-platform-properties is an optional bool & specifies if BuildStream
-should determine the platform properties & values (which can be set in 'sandbox' config)
-to be added to the remote sandbox commands. This behaviour defaults to True if
-not specified in the configuration.
+platform-properties is optional, additional properties specific to the Remote Execution
+ennvironment can be be provided as key:value pairs and are included with the default
+properties of the sandbox (the values of which are derived from the local sandox enviroment,
+unless set in `sandbox` config). Pre-emptive compatability filtering isn't applied and default
+property values (such as OSFamily, ISA) cannot be overriden here (configurable in `sandbox` config)
+however they can can be explicitly disabled by setting the key value to [].
 
 The Remote Execution API can be found via https://github.com/bazelbuild/remote-apis.
 
diff --git a/doc/source/using_config.rst b/doc/source/using_config.rst
index 919b051f9..fb6673a8e 100644
--- a/doc/source/using_config.rst
+++ b/doc/source/using_config.rst
@@ -210,10 +210,9 @@ configuration will be used as fallback.
         action-cache-service:
           url: http://cache.some_project.example.com:50052
           instance-name: main
-        custom-platform-properties:
+        platform-properties:
           docker-image: docker://marketplace.gcr.io/google/rbe-ubuntu16-04
-          ISA: arm-a64
-        default-platform-properties: False
+          ISA: []
 
 
 .. _user_config_strict_mode:
diff --git a/src/buildstream/data/projectconfig.yaml b/src/buildstream/data/projectconfig.yaml
index dce4cc6aa..4b129ab75 100644
--- a/src/buildstream/data/projectconfig.yaml
+++ b/src/buildstream/data/projectconfig.yaml
@@ -72,7 +72,7 @@ environment-nocache: []
 
 # Configuration for the sandbox other than environment variables
 # should go in 'sandbox'. Custom platform properties for Remote
-# Execution commands can be set via 'remote-execution' config.
+# Execution services can be set via 'remote-execution' config.
 sandbox: {}
 
 # Defaults for the 'split-rules' public data found on elements
diff --git a/src/buildstream/sandbox/_sandboxbuildboxrun.py b/src/buildstream/sandbox/_sandboxbuildboxrun.py
index 3d71b7440..594a45fef 100644
--- a/src/buildstream/sandbox/_sandboxbuildboxrun.py
+++ b/src/buildstream/sandbox/_sandboxbuildboxrun.py
@@ -70,6 +70,7 @@ class SandboxBuildBoxRun(SandboxREAPI):
             # limit support to native building on the host ISA.
             cls._isas.add(Platform.get_host_arch())
 
+    # Only called when lauching a local sandbox, as we can't pre-empt the remote environment capabilities
     @classmethod
     def check_sandbox_config(cls, platform, config):
         if config.build_os not in cls._osfamilies:
diff --git a/src/buildstream/sandbox/_sandboxreapi.py b/src/buildstream/sandbox/_sandboxreapi.py
index fd63ec9cb..1785db662 100644
--- a/src/buildstream/sandbox/_sandboxreapi.py
+++ b/src/buildstream/sandbox/_sandboxreapi.py
@@ -119,36 +119,49 @@ class SandboxREAPI(Sandbox):
         # Request read-write directories as output
         output_directories = [os.path.relpath(dir, start=working_directory) for dir in read_write_directories]
 
-        # Get the custom platform properties, if specified. These are not filtered
-        platform_dict = self._get_custom_platform_properties()
-
-        # Unless explicitly disabled, generate default platform properties
-        if self._use_default_platform_properties():
-            config = self._get_config()
-            default_dict = {}
-            default_dict["OSFamily"] = config.build_os
-            default_dict["ISA"] = config.build_arch
-
-            if flags & SandboxFlags.INHERIT_UID:
-                uid = os.geteuid()
-                gid = os.getegid()
+        # Get the SandoxConfig
+        config = self._get_config()
+        default_dict = {}
+        default_dict["OSFamily"] = config.build_os
+        default_dict["ISA"] = config.build_arch
+
+        if flags & SandboxFlags.INHERIT_UID:
+            uid = os.geteuid()
+            gid = os.getegid()
+        else:
+            uid = config.build_uid
+            gid = config.build_gid
+        if uid is not None:
+            default_dict["unixUID"] = str(uid)
+        if gid is not None:
+            default_dict["unixGID"] = str(gid)
+
+        if flags & SandboxFlags.NETWORK_ENABLED:
+            default_dict["network"] = "on"
+        # Remove unsupported platform properties from the default dict, this filter is derived from the
+        # local sandbox capabilities
+        supported_properties = self._supported_platform_properties()
+        platform_dict = {key: value for (key, value) in default_dict.items() if key in supported_properties}
+
+        # Get the platform properties dict, if specified. These are not filtered as they are specific
+        # to the remote server
+        platform_properties = self._get_platform_properties()
+
+        # Apply the properties to the default_dict. k:v pairs in the default_dict
+        # can be disabled if given a explicit value of `[]` in platform properties
+        # with a matching key.
+        for platform_property, value in platform_properties.items():
+            if platform_property in platform_dict:
+                if value != []:
+                    raise SandboxError(
+                        "Platform Property {}:{} should be configured in sandbox config, not remote-execution.".format(
+                            platform_property, value
+                        ),
+                        reason="invalid-platform-property",
+                    )
+                del platform_dict[platform_property]
             else:
-                uid = config.build_uid
-                gid = config.build_gid
-            if uid is not None:
-                default_dict["unixUID"] = str(uid)
-            if gid is not None:
-                default_dict["unixGID"] = str(gid)
-
-            if flags & SandboxFlags.NETWORK_ENABLED:
-                default_dict["network"] = "on"
-            # Remove unsupported platform properties from the default dict
-            supported_properties = self._supported_platform_properties()
-            default_dict = {key: value for (key, value) in default_dict.items() if key in supported_properties}
-
-            # Apply the defaults to the platform_dict. Default values take precedence
-            # on key collisions
-            platform_dict = {**platform_dict, **default_dict}
+                platform_dict[platform_property] = value
 
         # Create Platform message with properties sorted by name in code point order
         platform = remote_execution_pb2.Platform()
@@ -208,12 +221,9 @@ class SandboxREAPI(Sandbox):
     def _supported_platform_properties(self):
         return {"OSFamily", "ISA"}
 
-    def _get_custom_platform_properties(self):
+    def _get_platform_properties(self):
         return {}
 
-    def _use_default_platform_properties(self):
-        return True
-
 
 # _SandboxREAPIBatch()
 #
diff --git a/src/buildstream/sandbox/_sandboxremote.py b/src/buildstream/sandbox/_sandboxremote.py
index 4439dc42c..a882a19cd 100644
--- a/src/buildstream/sandbox/_sandboxremote.py
+++ b/src/buildstream/sandbox/_sandboxremote.py
@@ -41,7 +41,7 @@ from .._remote import RemoteSpec
 
 
 class RemoteExecutionSpec(
-    namedtuple("RemoteExecutionSpec", "exec_service storage_service action_service custom_properties use_defaults")
+    namedtuple("RemoteExecutionSpec", "exec_service storage_service action_service platform_properties")
 ):
     pass
 
@@ -98,8 +98,7 @@ class SandboxRemote(SandboxREAPI):
             self.action_instance = None
             self.action_credentials = None
 
-        self.custom_properties = config.custom_properties
-        self.use_defaults = config.use_defaults
+        self.platform_properties = config.platform_properties
 
         self.exec_instance = config.exec_service.get("instance-name", None)
         self.storage_instance = config.storage_service.get("instance-name", None)
@@ -136,15 +135,12 @@ class SandboxRemote(SandboxREAPI):
 
         service_keys = ["execution-service", "storage-service", "action-cache-service"]
 
-        remote_config.validate_keys(
-            ["url", "custom-platform-properties", "default-platform-properties", *service_keys]
-        )
+        remote_config.validate_keys(["url", "platform-properties", *service_keys])
 
         exec_config = require_node(remote_config, "execution-service")
         storage_config = require_node(remote_config, "storage-service")
         action_config = remote_config.get_mapping("action-cache-service", default={})
-        custom_properties = remote_config.get_mapping("custom-platform-properties", default={})
-        use_defaults = remote_config.get_bool("default-platform-properties", default=True)
+        platform_properties = remote_config.get_mapping("platform-properties", default={})
 
         tls_keys = ["client-key", "client-cert", "server-cert"]
 
@@ -190,11 +186,11 @@ class SandboxRemote(SandboxREAPI):
                 if tls_key in config:
                     config[tls_key] = resolve_path(config.get_str(tls_key))
 
-        # Add in the custom platform properties config
-        service_configs.append(custom_properties)
+        # Add in the platform properties config
+        service_configs.append(platform_properties)
 
         # TODO: we should probably not be stripping node info and rather load files the safe way
-        return RemoteExecutionSpec(*[conf.strip_node_info() for conf in service_configs], use_defaults)
+        return RemoteExecutionSpec(*[conf.strip_node_info() for conf in service_configs])
 
     def run_remote_command(self, channel, action_digest):
         # Sends an execution request to the remote execution server.
@@ -442,13 +438,9 @@ class SandboxRemote(SandboxREAPI):
                 self.info("Action result found in action cache")
                 return result
 
-    def _get_custom_platform_properties(self):
-        # Dict containing custom platformn properties, if provided in config
-        return self.custom_properties
-
-    def _use_default_platform_properties(self):
-        # Bool, defaults to True unless overriden in RE Spec
-        return self.use_defaults
+    def _get_platform_properties(self):
+        # Dict platformn properties, if provided in config
+        return self.platform_properties
 
     @staticmethod
     def _extract_action_result(operation):
diff --git a/src/buildstream/testing/runcli.py b/src/buildstream/testing/runcli.py
index be72f6ae0..689a5c69c 100644
--- a/src/buildstream/testing/runcli.py
+++ b/src/buildstream/testing/runcli.py
@@ -805,14 +805,24 @@ def cli_remote_execution(tmpdir, remote_services):
         remote_execution["storage-service"] = {
             "url": remote_services.storage_service,
         }
-    if remote_services.custom_properties:
-        # Expected string with substring pattern "--platform property=value"
-        remote_execution["custom-platform-properties"] = dict(
-            s.split("=") for s in remote_services.custom_properties.replace("--platform", "").split()
+    if remote_services.platform_properties:
+        default_properties = ["OSFamily", "ISA"]
+        # Strip cli argument, expected string with substring pattern "--platform property=value"
+        parsed_properties = dict(
+            s.split("=") for s in remote_services.platform_properties.replace("--platform", "").split()
         )
+        # If a default property has been set on the server, do not add it to remote-execution config
+        # as these should configured via sandbox config in bst if required. If a default hasn't been
+        # set on the server (e.g, no ISO) then this maps to it needing to be explicitly disabled in bst.
+        for default_property in default_properties:
+            if default_property in parsed_properties:
+                del parsed_properties[default_property]
+            else:
+                parsed_properties[default_property] = []
+
+        remote_execution["platform-properties"] = parsed_properties
+
     if remote_execution:
-        if not remote_services.use_defaults:
-            remote_execution["default-platform-properties"] = False
         fixture.configure({"remote-execution": remote_execution})
 
     if remote_services.source_service:
diff --git a/tests/conftest.py b/tests/conftest.py
index a0fc1121d..759a06005 100755
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -103,8 +103,7 @@ class RemoteServices:
         self.storage_service = kwargs.get("storage_service")
         self.artifact_index_service = kwargs.get("artifact_index_service")
         self.artifact_storage_service = kwargs.get("artifact_storage_service")
-        self.use_defaults = kwargs.get("default_properties")
-        self.custom_properties = kwargs.get("custom_properties")
+        self.platform_properties = kwargs.get("platform_properties")
 
 
 @pytest.fixture(scope="session")
@@ -128,11 +127,8 @@ def remote_services(request):
     if "SOURCE_CACHE_SERVICE" in os.environ:
         kwargs["source_service"] = os.environ.get("SOURCE_CACHE_SERVICE")
 
-    if "DEFAULT_PROPERTIES" in os.environ:
-        kwargs["default_properties"] = os.environ.get("DEFAULT_PROPERTIES")
-
-    if "CUSTOM_PROPERTIES" in os.environ:
-        kwargs["custom_properties"] = os.environ.get("CUSTOM_PROPERTIES")
+    if "PLATFORM_PROPERTIES" in os.environ:
+        kwargs["platform_properties"] = os.environ.get("PLATFORM_PROPERTIES")
 
     return RemoteServices(**kwargs)
 
diff --git a/tests/remoteexecution/buildfail.py b/tests/remoteexecution/buildfail.py
index 37f4dcafa..3c1dfea24 100644
--- a/tests/remoteexecution/buildfail.py
+++ b/tests/remoteexecution/buildfail.py
@@ -58,3 +58,31 @@ def test_build_remote_failure(cli, datafiles):
     # check that the file created before the failure exists
     filename = os.path.join(checkout_path, "foo")
     assert os.path.isfile(filename)
+
+
+# Assert that a SandboxError is given if an invalid Remote Execution platform property
+# is given which should be configured at a sandbox level, e.g. OSFamily
+@pytest.mark.datafiles(DATA_DIR)
+def test_default_platform_property_error(cli, datafiles):
+    project = str(datafiles)
+    element_path = os.path.join(project, "elements", "element.bst")
+
+    # Write out our test target
+    element = {
+        "kind": "script",
+        "depends": [{"filename": "base.bst", "type": "build",},],
+        "config": {"commands": ["touch %{install-root}/foo",],},
+    }
+    _yaml.roundtrip_dump(element, element_path)
+
+    services = cli.ensure_services()
+    assert set(services) == set(["action-cache", "execution", "storage"])
+
+    # Add invalid platform property to remote execution config, this will override any
+    # valid [] keys generated for any other testing config. Default properties in relation
+    # to the local sandbox (e.g, OSFamily & ISO) should only be configured via sandbox config.
+    cli.config["remote-execution"]["platform-properties"]["OSFamily"] = "macos"
+
+    # Try to build it, this should result in a Sanbox error when contructing the platform dict
+    result = cli.run(project=project, args=["build", "element.bst"])
+    result.assert_task_error(ErrorDomain.SANDBOX, "invalid-platform-property")
diff --git a/tox.ini b/tox.ini
index 31a999db3..7f94dc8e1 100644
--- a/tox.ini
+++ b/tox.ini
@@ -64,8 +64,7 @@ passenv =
     SOURCE_CACHE_SERVICE
     SSL_CERT_FILE
     BST_PLUGINS_EXPERIMENTAL_VERSION
-    DEFAULT_PROPERTIES
-    CUSTOM_PROPERTIES
+    PLATFORM_PROPERTIES
 #
 # These keys are not inherited by any other sections
 #