Make public data immutable
As a part of sealing up the sandbox[0], public data needs to be made
immutable for a number of reasons:
- Manipulation of public data with python code adds a significant
risk that public data is not deterministic.
- Various plugins have been reading public data at initialization time,
in advance of the build phase where they are allowed to read it off of
dependencies, this means that currently we have plugins reading invalid
(pre-mutation) public data from dependencies and thus creating
unpredictable results.
Summary of changes:
o element.py:
- Remove the `__dynamic_public` member, and use only the `__public`
member for storing public data.
- Continue to support loading public data from the artifact for the
purpose of ArtifactElement (ability to observe public data when
loading the artifacts in `bst artifact` commands).
- Make `Element.set_public_data()` private/internal (we still need to
use it elsewhere).
o filter.py (element plugin):
- Refactor this plugin to use the new `configure_dependencies()` in order
to parse the filter element, hold on to the filter element throughout
the plugin lifetime in order to simplify the plugin.
- Change how we support the `pass-integration` configuration.
Public data is now immutable publicly but we still need to automatically
mutate the filter element's intergration commands at load time.
Instead of waiting for the build phase like before, now we do it
immediately at load time.
This is done early because reverse dependencies are allowed to read
the public data immediately at load time, so we must provide stable
public data immediately, before reverse dependencies ever get a chance
to observe it.
o tests/elements/filter.py: Removed a test which was asserting the ability
of plugins to mutate public data, this is no longer allowed and the API
is now removed.
[0]: https://lists.apache.org/thread.html/r3eb2dce3561cb46ac80c859b14aafb2471aaf3e319a39f70475fe22a%40%3Cdev.buildstream.apache.org%3E
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 503361e..a28e055 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -300,7 +300,6 @@
self.__build_result = None # The result of assembling this Element (success, description, detail)
# Artifact class for direct artifact composite interaction
self.__artifact = None # type: Optional[Artifact]
- self.__dynamic_public = None
self.__sandbox_config = None # type: Optional[SandboxConfig]
self.__batch_prepare_assemble = False # Whether batching across prepare()/assemble() is configured
@@ -317,6 +316,7 @@
self.__environment: Dict[str, str] = {}
self.__variables: Optional[Variables] = None
+ self.__public: "MappingNode" = Node.from_dict({})
if artifact:
self.__initialize_from_artifact(artifact)
@@ -761,43 +761,11 @@
domain: A public domain name to fetch data for
Returns:
-
- .. note::
-
- This can only be called the abstract methods which are
- called as a part of the :ref:`build phase <core_element_build_phase>`
- and never before.
+ The public data for the requested *domain*, if *domain* does not exist
+ for this element, then an empty node will be returned.
"""
- if self.__dynamic_public is None:
- self.__load_public_data()
-
- # Disable type-checking since we can't easily tell mypy that
- # `self.__dynamic_public` can't be None here.
- data = self.__dynamic_public.get_mapping(domain, default=None) # type: ignore
- if data is not None:
- data = data.clone()
-
- return data
-
- def set_public_data(self, domain: str, data: "MappingNode[Node]") -> None:
- """Set public data on this element
-
- Args:
- domain: A public domain name to fetch data for
- data: The public data dictionary for the given domain
-
- This allows an element to dynamically mutate public data of
- elements or add new domains as the result of success completion
- of the :func:`Element.assemble() <buildstream.element.Element.assemble>`
- method.
- """
- if self.__dynamic_public is None:
- self.__load_public_data()
-
- if data is not None:
- data = data.clone()
-
- self.__dynamic_public[domain] = data # type: ignore
+ data = self.__public.get_mapping(domain, default={})
+ return data.clone()
def get_environment(self) -> Dict[str, str]:
"""Fetch the environment suitable for running in the sandbox
@@ -1222,6 +1190,21 @@
def __set_build_result(self, success, description, detail=None):
self.__build_result = (success, description, detail)
+ # _set_public_data():
+ #
+ # Set public data on this element
+ #
+ # Args:
+ # domain: A public domain name to fetch data for
+ # data: The public data dictionary for the given domain
+ #
+ # This internal function allows the core and core components to doctor
+ # the public data after being loaded, as the filter element does for instance
+ # in order to propagate some public data from it's dependency forward to itself.
+ #
+ def _set_public_data(self, domain: str, data: "MappingNode[Node]") -> None:
+ self.__public[domain] = data.clone() # type: ignore
+
# _cached_success():
#
# Returns:
@@ -1739,10 +1722,6 @@
# sandboxes.
sandbox._disable_run()
- # By default, the dynamic public data is the same as the static public data.
- # The plugin's assemble() method may modify this, though.
- self.__dynamic_public = self.__public.clone()
-
# Call the abstract plugin methods
# Step 1 - Configure
@@ -1779,7 +1758,7 @@
context = self._get_context()
buildresult = self.__build_result
- publicdata = self.__dynamic_public
+ publicdata = self.__public
sandbox_vroot = sandbox.get_virtual_directory()
collectvdir = None
sandbox_build_dir = None
@@ -2438,6 +2417,7 @@
self.__environment = artifact.load_environment()
self.__sandbox_config = artifact.load_sandbox_config()
self.__variables = artifact.load_variables()
+ self.__public = self.__artifact.load_public_data()
self.__cache_key = artifact.strong_key
self.__strict_cache_key = artifact.strict_key
@@ -3153,16 +3133,6 @@
if filter_func(filename):
yield filename
- # __load_public_data():
- #
- # Loads the public data from the cached artifact
- #
- def __load_public_data(self):
- self.__assert_cached()
- assert self.__dynamic_public is None
-
- self.__dynamic_public = self.__artifact.load_public_data()
-
def __load_build_result(self):
self.__assert_cached()
assert self.__build_result is None
diff --git a/src/buildstream/plugins/elements/filter.py b/src/buildstream/plugins/elements/filter.py
index 5560f7b..65cbac3 100644
--- a/src/buildstream/plugins/elements/filter.py
+++ b/src/buildstream/plugins/elements/filter.py
@@ -167,25 +167,59 @@
self.exclude = self.exclude_node.as_str_list()
self.include_orphans = node.get_bool("include-orphans")
self.pass_integration = node.get_bool("pass-integration", False)
+ self.filter_element = None
- def preflight(self):
+ def configure_dependencies(self, dependencies):
+
# Exactly one build-depend is permitted
- build_deps = list(self._dependencies(_Scope.BUILD, recurse=False))
- if len(build_deps) != 1:
- detail = "Full list of build-depends:\n"
- deps_list = " \n".join([x.name for x in build_deps])
+ if len(dependencies) != 1:
+ detail = "Full list of build dependencies:\n"
+ deps_list = " \n".join([x.path for x in dependencies])
detail += deps_list
raise ElementError(
- "{}: {} element must have exactly 1 build-dependency, actually have {}".format(
- self, type(self).__name__, len(build_deps)
+ "{}: {} element must have exactly 1 build dependency, actually have {}".format(
+ self, type(self).__name__, len(dependencies)
),
detail=detail,
reason="filter-bdepend-wrong-count",
)
- # That build-depend must not also be a runtime-depend
+ # Hold on to the element being filtered for later (this is actually an ElementProxy)
+ #
+ self.filter_element = dependencies[0].element
+
+ # If the filter element does not produce an artifact, fail and inform user that the dependency
+ # must produce artifacts
+ if not self.filter_element.BST_ELEMENT_HAS_ARTIFACT:
+ detail = "{} does not produce an artifact, so there is nothing to filter".format(dependencies[0].path)
+ raise ElementError(
+ "{}: {} element's build dependency must produce an artifact".format(self, type(self).__name__),
+ detail=detail,
+ reason="filter-bdepend-no-artifact",
+ )
+
+ # Optionally inherit the integration public data
+ if self.pass_integration:
+
+ # Integration commands of the build dependency
+ pub_data = self.filter_element.get_public_data("bst")
+ integration_commands = pub_data.get_str_list("integration-commands", [])
+
+ # Integration commands of the filter element itself
+ filter_pub_data = self.get_public_data("bst")
+ filter_integration_commands = filter_pub_data.get_str_list("integration-commands", [])
+
+ # Concatenate the command lists
+ filter_pub_data["integration-commands"] = integration_commands + filter_integration_commands
+ self._set_public_data("bst", filter_pub_data)
+
+ def preflight(self):
+
+ # The filter element must not also be a runtime dependency
runtime_deps = list(self._dependencies(_Scope.RUN, recurse=False))
- if build_deps[0] in runtime_deps:
+
+ # The filter_element is an ElementProxy, check if the proxied element is in the runtime deps.
+ if self.filter_element._plugin in runtime_deps:
detail = "Full list of runtime depends:\n"
deps_list = " \n".join([x.name for x in runtime_deps])
detail += deps_list
@@ -197,16 +231,6 @@
reason="filter-bdepend-also-rdepend",
)
- # If a parent does not produce an artifact, fail and inform user that the dependency
- # must produce artifacts
- if not build_deps[0].BST_ELEMENT_HAS_ARTIFACT:
- detail = "{} does not produce an artifact, so there is nothing to filter".format(build_deps[0].name)
- raise ElementError(
- "{}: {} element's build dependency must produce an artifact".format(self, type(self).__name__),
- detail=detail,
- reason="filter-bdepend-no-artifact",
- )
-
def get_unique_key(self):
key = {
"include": sorted(self.include),
@@ -220,61 +244,49 @@
def stage(self, sandbox):
with self.timed_activity("Staging artifact", silent_nested=True):
- for dep in self.dependencies(recurse=False):
- # Check that all the included/excluded domains exist
- pub_data = dep.get_public_data("bst")
- split_rules = pub_data.get_mapping("split-rules", {})
- unfound_includes = []
- for domain in self.include:
- if domain not in split_rules:
- unfound_includes.append(domain)
- unfound_excludes = []
- for domain in self.exclude:
- if domain not in split_rules:
- unfound_excludes.append(domain)
- detail = []
- if unfound_includes:
- detail.append("Unknown domains were used in {}".format(self.include_node.get_provenance()))
- detail.extend([" - {}".format(domain) for domain in unfound_includes])
+ # Check that all the included/excluded domains exist
+ pub_data = self.filter_element.get_public_data("bst")
+ split_rules = pub_data.get_mapping("split-rules", {})
+ unfound_includes = []
+ for domain in self.include:
+ if domain not in split_rules:
+ unfound_includes.append(domain)
+ unfound_excludes = []
+ for domain in self.exclude:
+ if domain not in split_rules:
+ unfound_excludes.append(domain)
- if unfound_excludes:
- detail.append("Unknown domains were used in {}".format(self.exclude_node.get_provenance()))
- detail.extend([" - {}".format(domain) for domain in unfound_excludes])
+ detail = []
+ if unfound_includes:
+ detail.append("Unknown domains were used in {}".format(self.include_node.get_provenance()))
+ detail.extend([" - {}".format(domain) for domain in unfound_includes])
- if detail:
- detail = "\n".join(detail)
- raise ElementError("Unknown domains declared.", detail=detail)
+ if unfound_excludes:
+ detail.append("Unknown domains were used in {}".format(self.exclude_node.get_provenance()))
+ detail.extend([" - {}".format(domain) for domain in unfound_excludes])
- dep.stage_artifact(sandbox, include=self.include, exclude=self.exclude, orphans=self.include_orphans)
+ if detail:
+ detail = "\n".join(detail)
+ raise ElementError("Unknown domains declared.", detail=detail)
+
+ self.filter_element.stage_artifact(
+ sandbox, include=self.include, exclude=self.exclude, orphans=self.include_orphans
+ )
def assemble(self, sandbox):
- if self.pass_integration:
- build_deps = list(self.dependencies(recurse=False))
- assert len(build_deps) == 1
- dep = build_deps[0]
-
- # Integration commands of the build dependency
- pub_data = dep.get_public_data("bst")
- integration_commands = pub_data.get_str_list("integration-commands", [])
-
- # Integration commands of the filter element itself
- filter_pub_data = self.get_public_data("bst")
- filter_integration_commands = filter_pub_data.get_str_list("integration-commands", [])
-
- # Concatenate the command lists
- filter_pub_data["integration-commands"] = integration_commands + filter_integration_commands
- self.set_public_data("bst", filter_pub_data)
return ""
+ #
+ # Private abstract method which yields the element which provides sources, for
+ # the purpose of redirecting commands like `source checkout` or `workspace open`
+ # and such.
+ #
def _get_source_element(self):
# Filter elements act as proxies for their sole build-dependency
#
- build_deps = list(self._dependencies(_Scope.BUILD, recurse=False))
- assert len(build_deps) == 1
- output_elm = build_deps[0]._get_source_element()
- return output_elm
+ return self.filter_element._get_source_element()
def setup():
diff --git a/tests/elements/filter.py b/tests/elements/filter.py
index 443f64d..1e9953f 100644
--- a/tests/elements/filter.py
+++ b/tests/elements/filter.py
@@ -28,21 +28,6 @@
@pytest.mark.datafiles(os.path.join(DATA_DIR, "basic"))
-def test_filter_include_dynamic(datafiles, cli, tmpdir):
- project = str(datafiles)
- result = cli.run(project=project, args=["build", "output-dynamic-include.bst"])
- result.assert_success()
-
- checkout = os.path.join(tmpdir.dirname, tmpdir.basename, "checkout")
- result = cli.run(
- project=project, args=["artifact", "checkout", "output-dynamic-include.bst", "--directory", checkout]
- )
- result.assert_success()
- assert os.path.exists(os.path.join(checkout, "foo"))
- assert not os.path.exists(os.path.join(checkout, "bar"))
-
-
-@pytest.mark.datafiles(os.path.join(DATA_DIR, "basic"))
def test_filter_exclude(datafiles, cli, tmpdir):
project = str(datafiles)
result = cli.run(project=project, args=["build", "output-exclude.bst"])
diff --git a/tests/elements/filter/basic/element_plugins/dynamic.py b/tests/elements/filter/basic/element_plugins/dynamic.py
deleted file mode 100644
index 401c6b1..0000000
--- a/tests/elements/filter/basic/element_plugins/dynamic.py
+++ /dev/null
@@ -1,35 +0,0 @@
-from buildstream import Element
-
-
-# Copies files from the dependent element but inserts split-rules using dynamic data
-class DynamicElement(Element):
-
- BST_MIN_VERSION = "2.0"
-
- def configure(self, node):
- node.validate_keys(["split-rules"])
- self.split_rules = {key: value.as_str_list() for key, value in node.get_mapping("split-rules").items()}
-
- def preflight(self):
- pass
-
- def get_unique_key(self):
- return {"split-rules": self.split_rules}
-
- def configure_sandbox(self, sandbox):
- pass
-
- def stage(self, sandbox):
- with self.timed_activity("Staging artifact", silent_nested=True):
- self.stage_dependency_artifacts(sandbox)
-
- def assemble(self, sandbox):
- bstdata = self.get_public_data("bst")
- bstdata["split-rules"] = self.split_rules
- self.set_public_data("bst", bstdata)
-
- return ""
-
-
-def setup():
- return DynamicElement
diff --git a/tests/elements/filter/basic/project.conf b/tests/elements/filter/basic/project.conf
index 023943f..371d037 100644
--- a/tests/elements/filter/basic/project.conf
+++ b/tests/elements/filter/basic/project.conf
@@ -1,8 +1,3 @@
name: test
min-version: 2.0
element-path: elements
-plugins:
-- origin: local
- path: element_plugins
- elements:
- - dynamic
