Remove 'default-platform-properties' and reduce scope
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index cfc9756..457f6bf 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -193,9 +193,9 @@
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 @@
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 @@
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 2bc76eb..bef75b5 100644
--- a/.gitlab-ci/buildgrid-remote-execution.yml
+++ b/.gitlab-ci/buildgrid-remote-execution.yml
@@ -42,7 +42,7 @@
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 b0b6fc1..e11a396 100644
--- a/doc/source/format_project.rst
+++ b/doc/source/format_project.rst
@@ -327,9 +327,8 @@
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 @@
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 919b051..fb6673a 100644
--- a/doc/source/using_config.rst
+++ b/doc/source/using_config.rst
@@ -210,10 +210,9 @@
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 dce4cc6..4b129ab 100644
--- a/src/buildstream/data/projectconfig.yaml
+++ b/src/buildstream/data/projectconfig.yaml
@@ -72,7 +72,7 @@
# 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 3d71b74..594a45f 100644
--- a/src/buildstream/sandbox/_sandboxbuildboxrun.py
+++ b/src/buildstream/sandbox/_sandboxbuildboxrun.py
@@ -70,6 +70,7 @@
# 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 fd63ec9..1785db6 100644
--- a/src/buildstream/sandbox/_sandboxreapi.py
+++ b/src/buildstream/sandbox/_sandboxreapi.py
@@ -119,36 +119,49 @@
# 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()
+ # Get the SandoxConfig
+ config = self._get_config()
+ default_dict = {}
+ default_dict["OSFamily"] = config.build_os
+ default_dict["ISA"] = config.build_arch
- # 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()
+ 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.INHERIT_UID:
- uid = os.geteuid()
- gid = os.getegid()
+ 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 @@
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 4439dc4..a882a19 100644
--- a/src/buildstream/sandbox/_sandboxremote.py
+++ b/src/buildstream/sandbox/_sandboxremote.py
@@ -41,7 +41,7 @@
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 @@
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 @@
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 @@
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 @@
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 be72f6a..689a5c6 100644
--- a/src/buildstream/testing/runcli.py
+++ b/src/buildstream/testing/runcli.py
@@ -805,14 +805,24 @@
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 a0fc112..759a060 100755
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -103,8 +103,7 @@
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 @@
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 37f4dca..3c1dfea 100644
--- a/tests/remoteexecution/buildfail.py
+++ b/tests/remoteexecution/buildfail.py
@@ -58,3 +58,31 @@
# 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 31a999d..7f94dc8 100644
--- a/tox.ini
+++ b/tox.ini
@@ -64,8 +64,7 @@
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
#
