From 7a83caaf3069bf569d4442a6eee78db1c79ae6ac Mon Sep 17 00:00:00 2001 From: Tristan van Berkom Date: Sat, 26 Sep 2020 17:08:10 +0900 Subject: _loader/loadelement.pyx: Support multiple filenames in a dependency This allows declaring multiple dependencies with common properties in a mode convenient way, e.g.: depends: - type: build junction: subproject.bst filename: - foo.bst - bar.bst - baz.bst This was requested during this email thread: https://lists.apache.org/thread.html/r9d716f2734cae5a81731f9a46d73f944a7287ff168f6987e25597c6a%40%3Cdev.buildstream.apache.org%3E --- src/buildstream/_loader/loadelement.pyx | 170 +++++++++++++++++++++++++------- 1 file changed, 132 insertions(+), 38 deletions(-) diff --git a/src/buildstream/_loader/loadelement.pyx b/src/buildstream/_loader/loadelement.pyx index 80b96cd2c..aef17bf48 100644 --- a/src/buildstream/_loader/loadelement.pyx +++ b/src/buildstream/_loader/loadelement.pyx @@ -52,6 +52,17 @@ cpdef enum DependencyType: ALL = 0x003 +# Some forward declared lists, avoid creating these lists repeatedly +# +cdef list _filename_allowed_types=[ScalarNode, SequenceNode] +cdef list _valid_dependency_keys = [Symbol.FILENAME, Symbol.TYPE, Symbol.JUNCTION, Symbol.STRICT, Symbol.CONFIG] +cdef list _valid_typed_dependency_keys = [Symbol.FILENAME, Symbol.JUNCTION, Symbol.STRICT, Symbol.CONFIG] +cdef list _valid_element_keys = [ + 'kind', 'depends', 'sources', 'sandbox', 'variables', 'environment', 'environment-nocache', + 'config', 'public', 'description', 'build-depends', 'runtime-depends', +] + + # Dependency(): # # Early stage data model for dependencies objects, the LoadElement has @@ -122,31 +133,33 @@ cdef class Dependency: # load() # - # Load the dependency from a Node + # Load dependency attributes from a Node, and validate it # # Args: # dep (Node): A node to load the dependency from + # junction (str): The junction name, or None + # name (str): The element name # default_dep_type (DependencyType): The default dependency type # - cdef load(self, Node dep, int default_dep_type): + cdef load(self, Node dep, str junction, str name, int default_dep_type): cdef str parsed_type cdef MappingNode config_node + cdef ProvenanceInformation provenance - self._node = dep + self.junction = junction + self.name = name self.element = None + self._node = dep if type(dep) is ScalarNode: - self.name = dep.as_str() self.dep_type = default_dep_type or DependencyType.ALL - self.junction = None - self.strict = False elif type(dep) is MappingNode: if default_dep_type: - ( dep).validate_keys([Symbol.FILENAME, Symbol.JUNCTION, Symbol.STRICT, Symbol.CONFIG]) + ( dep).validate_keys(_valid_typed_dependency_keys) self.dep_type = default_dep_type else: - ( dep).validate_keys([Symbol.FILENAME, Symbol.TYPE, Symbol.JUNCTION, Symbol.STRICT, Symbol.CONFIG]) + ( dep).validate_keys(_valid_dependency_keys) # Resolve the DependencyType parsed_type = ( dep).get_str( Symbol.TYPE, None) @@ -161,8 +174,6 @@ cdef class Dependency: raise LoadError("{}: Dependency type '{}' is not 'build', 'runtime' or 'all'" .format(provenance, parsed_type), LoadErrorReason.INVALID_DATA) - self.name = ( dep).get_str( Symbol.FILENAME) - self.junction = ( dep).get_str( Symbol.JUNCTION, None) self.strict = ( dep).get_bool( Symbol.STRICT, False) config_node = ( dep).get_mapping( Symbol.CONFIG, None) @@ -196,18 +207,6 @@ cdef class Dependency: LoadErrorReason.INVALID_DATA, detail="Only dependencies required at build time may be declared `strict`.") - # `:` characters are not allowed in filename if a junction was - # explicitly specified - if self.junction and ':' in self.name: - raise LoadError("{}: Dependency {} contains `:` in its name. " - "`:` characters are not allowed in filename when " - "junction attribute is specified.".format(self.provenance, self.name), - LoadErrorReason.INVALID_DATA) - - # Attempt to split name if no junction was specified explicitly - if not self.junction and ':' in self.name: - self.junction, self.name = self.name.rsplit(':', maxsplit=1) - # merge() # # Merge the attributes of an existing dependency into this dependency @@ -282,12 +281,7 @@ cdef class LoadElement: self.dependencies = [] # Ensure the root node is valid - self.node.validate_keys([ - 'kind', 'depends', 'sources', 'sandbox', - 'variables', 'environment', 'environment-nocache', - 'config', 'public', 'description', - 'build-depends', 'runtime-depends', - ]) + self.node.validate_keys(_valid_element_keys) self.kind = node.get_str(Symbol.KIND, default=None) self.first_pass = self.kind in ("junction", "link") @@ -470,6 +464,96 @@ def sort_dependencies(LoadElement element, set visited): element.dependencies.sort(key=cmp_to_key(_dependency_cmp)) +# _parse_dependency_filename(): +# +# Parse the filename of a dependency with the already provided parsed junction +# name, if any. +# +# This will validate that the filename node does not contain `:` if +# the junction is already specified, and otherwise it will appropriately +# split the filename string and decompose it into a junction and filename. +# +# Args: +# node (ScalarNode): The ScalarNode of the filename +# junction (str): The already parsed junction, or None +# +# Returns: +# (str): The junction component of the dependency filename +# (str): The filename component of the dependency filename +# +cdef tuple _parse_dependency_filename(ScalarNode node, str junction): + cdef str name = node.as_str() + + if junction is not None: + if ':' in name: + raise LoadError( + "{}: Dependency {} contains `:` in its name. " + "`:` characters are not allowed in filename when " + "junction attribute is specified.".format(node.get_provenance(), name), + LoadErrorReason.INVALID_DATA) + elif ':' in name: + junction, name = name.rsplit(':', maxsplit=1) + + return junction, name + + +# _list_dependency_node_files(): +# +# List the filename, junction tuples associated with a dependency node, +# this supports the `filename` attribute being expressed as a list, so +# that multiple dependencies can be expressed with the common attributes. +# +# Args: +# node (Node): A YAML loaded dictionary +# +# Returns: +# (list): A list of filenames for `node` +# +cdef list _list_dependency_node_files(Node node): + + cdef list files = [] + cdef str junction + cdef tuple parsed_filename + cdef Node filename_node + cdef Node filename_iter + cdef object filename_iter_object + + # The node can be a single filename declaration + # + if type(node) is ScalarNode: + parsed_filename = _parse_dependency_filename(node, None) + files.append(parsed_filename) + + # Otherwise it is a dictionary + # + elif type(node) is MappingNode: + + junction = ( node).get_str( Symbol.JUNCTION, None) + filename_node = ( node).get_node( Symbol.FILENAME, allowed_types=_filename_allowed_types) + + if type(filename_node) is ScalarNode: + parsed_filename = _parse_dependency_filename(filename_node, junction) + files.append(parsed_filename) + else: + # The filename attribute is a list, iterate here + for filename_iter_object in ( filename_node).value: + filename_iter = filename_iter_object + + if type(filename_iter_object) is not ScalarNode: + raise LoadError( + "{}: Expected string while parsing the filename list".format(filename_iter.get_provenance()), + LoadErrorReason.INVALID_DATA + ) + + parsed_filename = _parse_dependency_filename(filename_iter, junction) + files.append(parsed_filename) + else: + raise LoadError("{}: Dependency is not specified as a string or a dictionary".format(node.get_provenance()), + LoadErrorReason.INVALID_DATA) + + return files + + # _extract_depends_from_node(): # # Helper for extract_depends_from_node to get dependencies of a particular type @@ -488,20 +572,30 @@ def sort_dependencies(LoadElement element, set visited): cdef void _extract_depends_from_node(Node node, str key, int default_dep_type, dict acc) except *: cdef SequenceNode depends = node.get_sequence(key, []) cdef Dependency existing_dep + cdef object dep_node_object cdef Node dep_node + cdef object deptup_object cdef tuple deptup + cdef str junction + cdef str filename - for dep_node in depends: - dependency = Dependency() - dependency.load(dep_node, default_dep_type) - deptup = (dependency.junction, dependency.name) + for dep_node_object in depends.value: + dep_node = dep_node_object - # Accumulate dependencies, merging any matching elements along the way - existing_dep = acc.get(deptup, None) - if existing_dep is not None: - existing_dep.merge(dependency) - else: - acc[deptup] = dependency + for deptup_object in _list_dependency_node_files(dep_node): + deptup = deptup_object + junction = deptup[0] + filename = deptup[1] + + dependency = Dependency() + dependency.load(dep_node, junction, filename, default_dep_type) + + # Accumulate dependencies, merging any matching elements along the way + existing_dep = acc.get(deptup, None) + if existing_dep is not None: + existing_dep.merge(dependency) + else: + acc[deptup] = dependency # Now delete the field, we dont want it anymore node.safe_del(key) -- cgit v1.2.1 From 14f9e473406d64c9b176412bd965a212553ca809 Mon Sep 17 00:00:00 2001 From: Tristan van Berkom Date: Sat, 26 Sep 2020 17:07:35 +0900 Subject: tests/format/dependencies3/elements/supported2.bst: Adding comment Adding some clarifications about an existing test --- tests/format/dependencies3/elements/supported2.bst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/format/dependencies3/elements/supported2.bst b/tests/format/dependencies3/elements/supported2.bst index 041ef08c1..51208de2e 100644 --- a/tests/format/dependencies3/elements/supported2.bst +++ b/tests/format/dependencies3/elements/supported2.bst @@ -1,5 +1,11 @@ kind: configsupported +# Here we test that the same dependency will be supplied to the +# plugin twice if they are listed twice (even though in this simple +# test case, we supply a redundant configuration, in real life it +# can be useful to configure the same dependency differently multiple +# times) +# depends: - filename: dep.bst config: -- cgit v1.2.1 From 295d65b9c753d378c97cafd132275016b9cea3f5 Mon Sep 17 00:00:00 2001 From: Tristan van Berkom Date: Sat, 26 Sep 2020 17:10:15 +0900 Subject: tests/format/dependencies.py: Testing shorthand dependency configurations --- tests/format/dependencies.py | 13 +++++++++++++ tests/format/dependencies3/elements/dep2.bst | 2 ++ tests/format/dependencies3/elements/shorthand-config.bst | 10 ++++++++++ tests/format/dependencies3/elements/shorthand-junction.bst | 11 +++++++++++ tests/format/dependencies3/elements/subproject.bst | 4 ++++ tests/format/dependencies3/subproject/project.conf | 2 ++ tests/format/dependencies3/subproject/sub.txt | 1 + tests/format/dependencies3/subproject/target-a.bst | 4 ++++ tests/format/dependencies3/subproject/target-b.bst | 4 ++++ 9 files changed, 51 insertions(+) create mode 100644 tests/format/dependencies3/elements/dep2.bst create mode 100644 tests/format/dependencies3/elements/shorthand-config.bst create mode 100644 tests/format/dependencies3/elements/shorthand-junction.bst create mode 100644 tests/format/dependencies3/elements/subproject.bst create mode 100644 tests/format/dependencies3/subproject/project.conf create mode 100644 tests/format/dependencies3/subproject/sub.txt create mode 100644 tests/format/dependencies3/subproject/target-a.bst create mode 100644 tests/format/dependencies3/subproject/target-b.bst diff --git a/tests/format/dependencies.py b/tests/format/dependencies.py index 12eb19d3a..0e2d4a1a7 100644 --- a/tests/format/dependencies.py +++ b/tests/format/dependencies.py @@ -281,3 +281,16 @@ def test_config_runtime_error(cli, datafiles): # result = cli.run(project=project, args=["show", "runtime-error.bst"]) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA) + + +@pytest.mark.datafiles(DATA_DIR) +@pytest.mark.parametrize( + "target,number", [("shorthand-config.bst", 2), ("shorthand-junction.bst", 2),], ids=["config", "junction"], +) +def test_shorthand(cli, datafiles, target, number): + project = os.path.join(str(datafiles), "dependencies3") + + result = cli.run(project=project, args=["show", target]) + result.assert_success() + + assert "TEST PLUGIN FOUND {} ENABLED DEPENDENCIES".format(number) in result.stderr diff --git a/tests/format/dependencies3/elements/dep2.bst b/tests/format/dependencies3/elements/dep2.bst new file mode 100644 index 000000000..9e5cf96b6 --- /dev/null +++ b/tests/format/dependencies3/elements/dep2.bst @@ -0,0 +1,2 @@ +kind: manual +description: Some kinda element diff --git a/tests/format/dependencies3/elements/shorthand-config.bst b/tests/format/dependencies3/elements/shorthand-config.bst new file mode 100644 index 000000000..72020c0d8 --- /dev/null +++ b/tests/format/dependencies3/elements/shorthand-config.bst @@ -0,0 +1,10 @@ +kind: configsupported + +# Here we test specification of multiple files with the same configuration +# +depends: +- filename: + - dep.bst + - dep2.bst + config: + enabled: true diff --git a/tests/format/dependencies3/elements/shorthand-junction.bst b/tests/format/dependencies3/elements/shorthand-junction.bst new file mode 100644 index 000000000..842a70dff --- /dev/null +++ b/tests/format/dependencies3/elements/shorthand-junction.bst @@ -0,0 +1,11 @@ +kind: configsupported + +# Here we test specification of multiple files through the same junction +# +depends: +- filename: + - target-a.bst + - target-b.bst + junction: subproject.bst + config: + enabled: true diff --git a/tests/format/dependencies3/elements/subproject.bst b/tests/format/dependencies3/elements/subproject.bst new file mode 100644 index 000000000..c88189cb0 --- /dev/null +++ b/tests/format/dependencies3/elements/subproject.bst @@ -0,0 +1,4 @@ +kind: junction +sources: +- kind: local + path: subproject diff --git a/tests/format/dependencies3/subproject/project.conf b/tests/format/dependencies3/subproject/project.conf new file mode 100644 index 000000000..39a53e2ab --- /dev/null +++ b/tests/format/dependencies3/subproject/project.conf @@ -0,0 +1,2 @@ +name: subtest +min-version: 2.0 diff --git a/tests/format/dependencies3/subproject/sub.txt b/tests/format/dependencies3/subproject/sub.txt new file mode 100644 index 000000000..f73f3093f --- /dev/null +++ b/tests/format/dependencies3/subproject/sub.txt @@ -0,0 +1 @@ +file diff --git a/tests/format/dependencies3/subproject/target-a.bst b/tests/format/dependencies3/subproject/target-a.bst new file mode 100644 index 000000000..e24d9bbb4 --- /dev/null +++ b/tests/format/dependencies3/subproject/target-a.bst @@ -0,0 +1,4 @@ +kind: import +sources: +- kind: local + path: sub.txt diff --git a/tests/format/dependencies3/subproject/target-b.bst b/tests/format/dependencies3/subproject/target-b.bst new file mode 100644 index 000000000..e24d9bbb4 --- /dev/null +++ b/tests/format/dependencies3/subproject/target-b.bst @@ -0,0 +1,4 @@ +kind: import +sources: +- kind: local + path: sub.txt -- cgit v1.2.1 From d2b0039cbd7ceaac9fd66bb4e17db04f321d494e Mon Sep 17 00:00:00 2001 From: Tristan van Berkom Date: Sat, 26 Sep 2020 17:18:23 +0900 Subject: tests/format/dependencies.py: Test bad filename configuration Test a dictionary instead of a string when given to the filename list. --- tests/format/dependencies.py | 11 +++++++++++ tests/format/dependencies3/elements/invalid-filenames.bst | 13 +++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 tests/format/dependencies3/elements/invalid-filenames.bst diff --git a/tests/format/dependencies.py b/tests/format/dependencies.py index 0e2d4a1a7..789df060f 100644 --- a/tests/format/dependencies.py +++ b/tests/format/dependencies.py @@ -294,3 +294,14 @@ def test_shorthand(cli, datafiles, target, number): result.assert_success() assert "TEST PLUGIN FOUND {} ENABLED DEPENDENCIES".format(number) in result.stderr + + +@pytest.mark.datafiles(DATA_DIR) +def test_invalid_filenames(cli, datafiles): + project = os.path.join(str(datafiles), "dependencies3") + + result = cli.run(project=project, args=["show", "invalid-filenames.bst"]) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA) + + # Assert expected provenance + assert "invalid-filenames.bst [line 9 column 4]" in result.stderr diff --git a/tests/format/dependencies3/elements/invalid-filenames.bst b/tests/format/dependencies3/elements/invalid-filenames.bst new file mode 100644 index 000000000..0a8325eae --- /dev/null +++ b/tests/format/dependencies3/elements/invalid-filenames.bst @@ -0,0 +1,13 @@ +kind: configsupported + +# Here we test an incorrect type of filename, e.g. a dictionary +# +depends: +- filename: + - target-a.bst + - target-b.bst + - multiple: foo + components: bar + junction: subproject.bst + config: + enabled: true -- cgit v1.2.1 From 4831f5bc8f7752447905fa34c241610b1024872c Mon Sep 17 00:00:00 2001 From: Tristan van Berkom Date: Sat, 26 Sep 2020 17:50:43 +0900 Subject: doc: Documenting ability to specify multiple filenames in a dependency. --- doc/source/format_declaring.rst | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/doc/source/format_declaring.rst b/doc/source/format_declaring.rst index 98b5f926e..380f367e0 100644 --- a/doc/source/format_declaring.rst +++ b/doc/source/format_declaring.rst @@ -417,7 +417,23 @@ Attributes: * ``filename`` - The :ref:`element name ` to depend on. + The :ref:`element name ` to depend on, or a list of mutiple element names. + + Specifying multiple element names in a single dependency will result in multiple dependencies + being declared with common properties. + + For example, one can declare multiple build dependencies with the same junction: + + .. code:: yaml + + # Declare three build dependencies from subproject.bst + depends: + - type: build + junction: subproject.bst + filename: + - element-a.bst + - element-b.bst + - element-c.bst * ``junction`` -- cgit v1.2.1