This is an automated email from the ASF dual-hosted git repository. tvb pushed a commit to branch tristan/change-remote-config in repository https://gitbox.apache.org/repos/asf/buildstream.git
commit 7898680d6a38341ed580ee3b2e302df0bf756b93 Author: Tristan van Berkom <[email protected]> AuthorDate: Sun Jan 17 20:19:00 2021 +0900 Remote execution can only be configured in user configuration As outlined on the mailing list[0], remote execution requires write access and certificates, as such there is no point for the project to make recommendations on this user configuration. Summary of changes: * _project.py: Removed RemoteExecutionSpec parsing * _context.py: Adjusted parsing of RemoteExecutionSpec First parse it initially, and then search for an override once the toplevel project is loaded, only the toplevel project override is significant, remote execution is only supported on a single build cluster for a given session. * element.py: Check context for whether remote execution is enabled at build time, instead of the element's project. * _stream.py: Check context for whether remote execution is enabled at initialization time, instead of checking all projects. * sandbox/_sandboxremote.py: Removed one keyword argument from the constructor, no need to specify the RemoteExecutionSpec in the constructor since the sandbox can just get that from the context object. * tests/sandboxes/remote-exec-config.py: Test with buildstream.conf This test was testing invalid configurations of remote-execution on project.conf files, which is no longer supported with this commit. This also removes one test which tests for an error with an empty configuration, this failure does not trigger when tested via project.conf due to the allowance of "pull-artifact-files" to sit alone within that configuration block. [0]: https://lists.apache.org/thread.html/rf2da9830e2fa918357f99a6021e55fc43df876f0b19d43f68802f083%40%3Cdev.buildstream.apache.org%3E --- src/buildstream/_context.py | 43 ++++++++++++++++------ src/buildstream/_project.py | 23 +----------- src/buildstream/_stream.py | 4 +- src/buildstream/element.py | 3 +- src/buildstream/sandbox/_sandboxremote.py | 3 +- tests/sandboxes/remote-exec-config.py | 35 ++++-------------- .../remote-exec-config/missing-certs/project.conf | 2 + 7 files changed, 47 insertions(+), 66 deletions(-) diff --git a/src/buildstream/_context.py b/src/buildstream/_context.py index 7a81d10..bae97ab 100644 --- a/src/buildstream/_context.py +++ b/src/buildstream/_context.py @@ -150,7 +150,7 @@ class Context: self.pull_buildtrees = None # Whether to pull the files of an artifact when doing remote execution - self.pull_artifact_files = None + self.pull_artifact_files = True # Whether or not to cache build trees on artifact creation self.cache_buildtrees = None @@ -333,19 +333,10 @@ class Context: # Load source cache config self.source_cache_specs = SourceCache.specs_from_config_node(defaults) - # Load remote execution config getting pull-artifact-files from it + # Load the global remote execution config including pull-artifact-files setting remote_execution = defaults.get_mapping("remote-execution", default=None) if remote_execution: - self.pull_artifact_files = remote_execution.get_bool("pull-artifact-files", default=True) - # This stops it being used in the remote service set up - remote_execution.safe_del("pull-artifact-files") - - # Don't pass the remote execution settings if that was the only option - if remote_execution.keys(): - self.remote_execution_specs = RemoteExecutionSpec.new_from_node(remote_execution) - else: - self.pull_artifact_files = True - self.remote_execution_specs = None + self.pull_artifact_files, self.remote_execution_specs = self._load_remote_execution(remote_execution) # Load pull build trees configuration self.pull_buildtrees = cache.get_bool("pull-buildtrees") @@ -448,6 +439,19 @@ class Context: def add_project(self, project): if not self._projects: self._workspaces = Workspaces(project, self._workspace_project_cache) + + # + # While loading the first, toplevel project, we can adjust some + # global settings which can be overridden on a per toplevel project basis. + # + override_node = self.get_overrides(project.name) + if override_node: + remote_execution = override_node.get_mapping("remote-execution", default=None) + if remote_execution: + self.pull_artifact_files, self.remote_execution_specs = self._load_remote_execution( + remote_execution + ) + self._projects.append(project) # get_projects(): @@ -563,3 +567,18 @@ class Context: log_directory=self.logdir, ) return self._cascache + + def _load_remote_execution(self, node): + # The pull_artifact_files attribute is special, it is allowed to + # be set to False even if there is no remote execution service configured. + # + pull_artifact_files = node.get_bool("pull-artifact-files", default=True) + node.safe_del("pull-artifact-files") + + # Don't pass the remote execution settings if that was the only option + if node.keys(): + remote_execution_specs = RemoteExecutionSpec.new_from_node(node) + else: + remote_execution_specs = None + + return pull_artifact_files, remote_execution_specs diff --git a/src/buildstream/_project.py b/src/buildstream/_project.py index 8e9c9ed..f1d1fa6 100644 --- a/src/buildstream/_project.py +++ b/src/buildstream/_project.py @@ -43,7 +43,7 @@ from ._loader import Loader, LoadContext from .element import Element from ._includes import Includes from ._workspaces import WORKSPACE_PROJECT_FILE -from ._remotespec import RemoteSpec, RemoteExecutionSpec +from ._remotespec import RemoteSpec if TYPE_CHECKING: @@ -141,7 +141,6 @@ class Project: # Remote specs for communicating with remote services self.artifact_cache_specs: List[RemoteSpec] = [] # Artifact caches self.source_cache_specs: List[RemoteSpec] = [] # Source caches - self.remote_execution_specs: Optional[RemoteExecutionSpec] = None # Remote execution services self.element_factory: Optional[ElementFactory] = None # ElementFactory for loading elements self.source_factory: Optional[SourceFactory] = None # SourceFactory for loading sources @@ -881,26 +880,6 @@ class Project: # Load source caches with pull/push config self.source_cache_specs = SourceCache.specs_from_config_node(config, self.directory) - # Load remote-execution configuration for this project - project_re_specs = None - project_re_node = config.get_mapping("remote-execution", default=None) - if project_re_node: - project_re_specs = RemoteExecutionSpec.new_from_node(project_re_node, self.directory) - - override_re_specs = None - override_node = self._context.get_overrides(self.name) - if override_node: - override_re_node = override_node.get_mapping("remote-execution", default=None) - if override_re_node: - override_re_specs = RemoteExecutionSpec.new_from_node(override_re_node, self.directory) - - if override_re_specs is not None: - self.remote_execution_specs = override_re_specs - elif project_re_specs is not None: - self.remote_execution_specs = project_re_specs - else: - self.remote_execution_specs = self._context.remote_execution_specs - # Load sandbox environment variables self.base_environment = config.get_mapping("environment") self.base_env_nocache = config.get_str_list("environment-nocache") diff --git a/src/buildstream/_stream.py b/src/buildstream/_stream.py index 6eb25e8..fa2421e 100644 --- a/src/buildstream/_stream.py +++ b/src/buildstream/_stream.py @@ -286,8 +286,8 @@ class Stream: # Assert that the elements are consistent _pipeline.assert_consistent(self._context, elements) - if all(project.remote_execution_specs for project in self._context.get_projects()): - # Remote execution is configured for all projects. + if self._context.remote_execution_specs: + # Remote execution is configured. # Require artifact files only for target elements and their runtime dependencies. self._context.set_artifact_files_optional() diff --git a/src/buildstream/element.py b/src/buildstream/element.py index 92f9e88..503361e 100644 --- a/src/buildstream/element.py +++ b/src/buildstream/element.py @@ -2772,7 +2772,7 @@ class Element(Plugin): else: output_node_properties = None - if directory is not None and allow_remote and project.remote_execution_specs: + if directory is not None and allow_remote and context.remote_execution_specs: self.info("Using a remote sandbox for artifact {} with directory '{}'".format(self.name, directory)) @@ -2786,7 +2786,6 @@ class Element(Plugin): stdout=stdout, stderr=stderr, config=config, - specs=project.remote_execution_specs, output_files_required=output_files_required, output_node_properties=output_node_properties, ) diff --git a/src/buildstream/sandbox/_sandboxremote.py b/src/buildstream/sandbox/_sandboxremote.py index 6ffe56b..af50051 100644 --- a/src/buildstream/sandbox/_sandboxremote.py +++ b/src/buildstream/sandbox/_sandboxremote.py @@ -43,7 +43,8 @@ class SandboxRemote(SandboxREAPI): self._output_files_required = kwargs.get("output_files_required", True) - specs = kwargs["specs"] # This should be a RemoteExecutionSpec + context = self._get_context() + specs = context.remote_execution_specs if specs is None: return diff --git a/tests/sandboxes/remote-exec-config.py b/tests/sandboxes/remote-exec-config.py index 2f5a2cf..b840b51 100644 --- a/tests/sandboxes/remote-exec-config.py +++ b/tests/sandboxes/remote-exec-config.py @@ -5,7 +5,6 @@ import os import pytest -from buildstream import _yaml from buildstream.exceptions import ErrorDomain, LoadErrorReason from buildstream.testing.runcli import cli # pylint: disable=unused-import @@ -20,35 +19,17 @@ DATA_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), "remote-exe def test_missing_certs(cli, datafiles, config_key, config_value): project = os.path.join(datafiles.dirname, datafiles.basename, "missing-certs") - project_conf = { - "name": "test", - "min-version": "2.0", - "remote-execution": { - "execution-service": {"url": "http://localhost:8088"}, - "storage-service": {"url": "http://charactron:11001";, config_key: config_value,}, - }, - } - project_conf_file = os.path.join(project, "project.conf") - _yaml.roundtrip_dump(project_conf, project_conf_file) + cli.configure( + { + "remote-execution": { + "execution-service": {"url": "http://localhost:8088"}, + "storage-service": {"url": "http://charactron:11001";, config_key: config_value,}, + }, + } + ) # Use `pull` here to ensure we try to initialize the remotes, triggering the error # # This does not happen for a simple `bst show`. result = cli.run(project=project, args=["show", "element.bst"]) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA, "Your config is missing") - - -# Assert that if incomplete information is supplied we get a sensible error message. [email protected](DATA_DIR) -def test_empty_config(cli, datafiles): - project = os.path.join(datafiles.dirname, datafiles.basename, "missing-certs") - - project_conf = {"name": "test", "min-version": "2.0", "remote-execution": {}} - project_conf_file = os.path.join(project, "project.conf") - _yaml.roundtrip_dump(project_conf, project_conf_file) - - # Use `pull` here to ensure we try to initialize the remotes, triggering the error - # - # This does not happen for a simple `bst show`. - result = cli.run(project=project, args=["artifact", "pull", "element.bst"]) - result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA, "specify one") diff --git a/tests/sandboxes/remote-exec-config/missing-certs/project.conf b/tests/sandboxes/remote-exec-config/missing-certs/project.conf new file mode 100644 index 0000000..20636c4 --- /dev/null +++ b/tests/sandboxes/remote-exec-config/missing-certs/project.conf @@ -0,0 +1,2 @@ +name: test +min-version: 2.0
