From ccf32059d0ddd0d78f830b11e9d576d771cb7c74 Mon Sep 17 00:00:00 2001 From: Chandan Singh Date: Thu, 6 Dec 2018 16:18:04 +0000 Subject: loader: Allow dependencies to use ":" to refer to junctioned elements This will allow cross-junction dependencies to be listed as strings on a single line. As part of this, some logic around initializing `Dependency()` objects have been moved out of `_extract_depends_from_node()` method into the constructor of `Dependency()` class, to keep all related code in one place. * _loader/types.py: While initializing `Dependency` objects, attempt to split filenames, only if no `junction` was specified explicitly. If a `junction` was specified, then filenames with `:` in their names will result in an error. * _loader/loadelement.py: Refactor logic to initialize `Dependency()` objects to move it to the `Dependency()` constructor. * tests/frontend/buildcheckout.py: Add tests to ensure the above. * _versions.py: Bump BST_FORMAT_VERSION. Fixes #809. --- buildstream/_loader/loadelement.py | 43 ++---------- buildstream/_loader/types.py | 60 ++++++++++++++-- buildstream/_versions.py | 2 +- tests/frontend/buildcheckout.py | 136 +++++++++++++++++++++++++++++++++++++ 4 files changed, 197 insertions(+), 44 deletions(-) diff --git a/buildstream/_loader/loadelement.py b/buildstream/_loader/loadelement.py index 4104dfd59..0b38cad65 100644 --- a/buildstream/_loader/loadelement.py +++ b/buildstream/_loader/loadelement.py @@ -18,10 +18,10 @@ # Tristan Van Berkom # System imports -from collections import Mapping +from itertools import count + # BuildStream toplevel imports -from .._exceptions import LoadError, LoadErrorReason from .. import _yaml # Local package imports @@ -146,42 +146,9 @@ def _extract_depends_from_node(node, *, key=None): depends = _yaml.node_get(node, list, key, default_value=[]) output_deps = [] - for dep in depends: - dep_provenance = _yaml.node_get_provenance(node, key=key, indices=[depends.index(dep)]) - - if isinstance(dep, str): - dependency = Dependency(dep, provenance=dep_provenance, dep_type=default_dep_type) - - elif isinstance(dep, Mapping): - if default_dep_type: - _yaml.node_validate(dep, ['filename', 'junction']) - dep_type = default_dep_type - else: - _yaml.node_validate(dep, ['filename', 'type', 'junction']) - - # Make type optional, for this we set it to None - dep_type = _yaml.node_get(dep, str, Symbol.TYPE, default_value=None) - if dep_type is None or dep_type == Symbol.ALL: - dep_type = None - elif dep_type not in [Symbol.BUILD, Symbol.RUNTIME]: - provenance = _yaml.node_get_provenance(dep, key=Symbol.TYPE) - raise LoadError(LoadErrorReason.INVALID_DATA, - "{}: Dependency type '{}' is not 'build', 'runtime' or 'all'" - .format(provenance, dep_type)) - - filename = _yaml.node_get(dep, str, Symbol.FILENAME) - junction = _yaml.node_get(dep, str, Symbol.JUNCTION, default_value=None) - dependency = Dependency(filename, - dep_type=dep_type, - junction=junction, - provenance=dep_provenance) - - else: - index = depends.index(dep) - p = _yaml.node_get_provenance(node, key=key, indices=[index]) - raise LoadError(LoadErrorReason.INVALID_DATA, - "{}: Dependency is not specified as a string or a dictionary".format(p)) - + for index, dep in enumerate(depends): + dep_provenance = _yaml.node_get_provenance(node, key=key, indices=[index]) + dependency = Dependency(dep, dep_provenance, default_dep_type=default_dep_type) output_deps.append(dependency) # Now delete the field, we dont want it anymore diff --git a/buildstream/_loader/types.py b/buildstream/_loader/types.py index 25b785532..eb6932b0b 100644 --- a/buildstream/_loader/types.py +++ b/buildstream/_loader/types.py @@ -17,6 +17,11 @@ # Authors: # Tristan Van Berkom +from collections.abc import Mapping + +from .._exceptions import LoadError, LoadErrorReason +from .. import _yaml + # Symbol(): # @@ -56,9 +61,54 @@ class Symbol(): # dependency was declared # class Dependency(): - def __init__(self, name, - dep_type=None, junction=None, provenance=None): - self.name = name - self.dep_type = dep_type - self.junction = junction + def __init__(self, dep, provenance, default_dep_type=None): self.provenance = provenance + + if isinstance(dep, str): + self.name = dep + self.dep_type = default_dep_type + self.junction = None + + elif isinstance(dep, Mapping): + if default_dep_type: + _yaml.node_validate(dep, ['filename', 'junction']) + dep_type = default_dep_type + else: + _yaml.node_validate(dep, ['filename', 'type', 'junction']) + + # Make type optional, for this we set it to None + dep_type = _yaml.node_get(dep, str, Symbol.TYPE, default_value=None) + if dep_type is None or dep_type == Symbol.ALL: + dep_type = None + elif dep_type not in [Symbol.BUILD, Symbol.RUNTIME]: + provenance = _yaml.node_get_provenance(dep, key=Symbol.TYPE) + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Dependency type '{}' is not 'build', 'runtime' or 'all'" + .format(provenance, dep_type)) + + self.name = _yaml.node_get(dep, str, Symbol.FILENAME) + self.dep_type = dep_type + self.junction = _yaml.node_get(dep, str, Symbol.JUNCTION, default_value=None) + + else: + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Dependency is not specified as a string or a dictionary".format(provenance)) + + # `:` characters are not allowed in filename if a junction was + # explicitly specified + if self.junction and ':' in self.name: + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Dependency {} contains `:` in its name. " + "`:` characters are not allowed in filename when " + "junction attribute is specified.".format(self.provenance, self.name)) + + # Name of the element should never contain more than one `:` characters + if self.name.count(':') > 1: + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Dependency {} contains multiple `:` in its name. " + "Recursive lookups for cross-junction elements is not " + "allowed.".format(self.provenance, self.name)) + + # Attempt to split name if no junction was specified explicitly + if not self.junction and self.name.count(':') == 1: + self.junction, self.name = self.name.split(':') diff --git a/buildstream/_versions.py b/buildstream/_versions.py index 289f6cd06..e0ec4cdec 100644 --- a/buildstream/_versions.py +++ b/buildstream/_versions.py @@ -23,7 +23,7 @@ # This version is bumped whenever enhancements are made # to the `project.conf` format or the core element format. # -BST_FORMAT_VERSION = 14 +BST_FORMAT_VERSION = 15 # The base BuildStream artifact version diff --git a/tests/frontend/buildcheckout.py b/tests/frontend/buildcheckout.py index 10849b918..fb4ef72fe 100644 --- a/tests/frontend/buildcheckout.py +++ b/tests/frontend/buildcheckout.py @@ -558,3 +558,139 @@ def test_build_checkout_cross_junction(datafiles, cli, tmpdir): filename = os.path.join(checkout, 'etc', 'animal.conf') assert os.path.exists(filename) + + +@pytest.mark.datafiles(DATA_DIR) +def test_build_junction_short_notation(cli, tmpdir, datafiles): + project = os.path.join(datafiles.dirname, datafiles.basename) + subproject_path = os.path.join(project, 'files', 'sub-project') + junction_path = os.path.join(project, 'elements', 'junction.bst') + element_path = os.path.join(project, 'elements', 'junction-dep.bst') + workspace = os.path.join(cli.directory, 'workspace') + checkout = os.path.join(cli.directory, 'checkout') + + # Create a repo to hold the subproject and generate a junction element for it + ref = generate_junction(tmpdir, subproject_path, junction_path) + + # Create a stack element to depend on a cross junction element, using + # colon (:) as the separator + element = { + 'kind': 'stack', + 'depends': ['junction.bst:import-etc.bst'] + } + _yaml.dump(element, element_path) + + # Now try to build it, this should automatically result in fetching + # the junction itself at load time. + result = cli.run(project=project, args=['build', 'junction-dep.bst']) + result.assert_success() + + # Assert that it's cached now + assert cli.get_element_state(project, 'junction-dep.bst') == 'cached' + + # Now check it out + result = cli.run(project=project, args=[ + 'checkout', 'junction-dep.bst', checkout + ]) + result.assert_success() + + # Assert the content of /etc/animal.conf + filename = os.path.join(checkout, 'etc', 'animal.conf') + assert os.path.exists(filename) + with open(filename, 'r') as f: + contents = f.read() + assert contents == 'animal=Pony\n' + + +@pytest.mark.datafiles(DATA_DIR) +def test_build_junction_short_notation_filename(cli, tmpdir, datafiles): + project = os.path.join(datafiles.dirname, datafiles.basename) + subproject_path = os.path.join(project, 'files', 'sub-project') + junction_path = os.path.join(project, 'elements', 'junction.bst') + element_path = os.path.join(project, 'elements', 'junction-dep.bst') + checkout = os.path.join(cli.directory, 'checkout') + + # Create a repo to hold the subproject and generate a junction element for it + ref = generate_junction(tmpdir, subproject_path, junction_path) + + # Create a stack element to depend on a cross junction element, using + # colon (:) as the separator + element = { + 'kind': 'stack', + 'depends': [{'filename': 'junction.bst:import-etc.bst'}] + } + _yaml.dump(element, element_path) + + # Now try to build it, this should automatically result in fetching + # the junction itself at load time. + result = cli.run(project=project, args=['build', 'junction-dep.bst']) + result.assert_success() + + # Assert that it's cached now + assert cli.get_element_state(project, 'junction-dep.bst') == 'cached' + + # Now check it out + result = cli.run(project=project, args=[ + 'checkout', 'junction-dep.bst', checkout + ]) + result.assert_success() + + # Assert the content of /etc/animal.conf + filename = os.path.join(checkout, 'etc', 'animal.conf') + assert os.path.exists(filename) + with open(filename, 'r') as f: + contents = f.read() + assert contents == 'animal=Pony\n' + + +@pytest.mark.datafiles(DATA_DIR) +def test_build_junction_short_notation_with_junction(cli, tmpdir, datafiles): + project = os.path.join(datafiles.dirname, datafiles.basename) + subproject_path = os.path.join(project, 'files', 'sub-project') + junction_path = os.path.join(project, 'elements', 'junction.bst') + element_path = os.path.join(project, 'elements', 'junction-dep.bst') + checkout = os.path.join(cli.directory, 'checkout') + + # Create a repo to hold the subproject and generate a junction element for it + ref = generate_junction(tmpdir, subproject_path, junction_path) + + # Create a stack element to depend on a cross junction element, using + # colon (:) as the separator + element = { + 'kind': 'stack', + 'depends': [{ + 'filename': 'junction.bst:import-etc.bst', + 'junction': 'junction.bst', + }] + } + _yaml.dump(element, element_path) + + # Now try to build it, this should fail as filenames should not contain + # `:` when junction is explicity specified + result = cli.run(project=project, args=['build', 'junction-dep.bst']) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA) + + +@pytest.mark.datafiles(DATA_DIR) +def test_build_junction_short_notation_with_junction(cli, tmpdir, datafiles): + project = os.path.join(datafiles.dirname, datafiles.basename) + subproject_path = os.path.join(project, 'files', 'sub-project') + junction_path = os.path.join(project, 'elements', 'junction.bst') + element_path = os.path.join(project, 'elements', 'junction-dep.bst') + checkout = os.path.join(cli.directory, 'checkout') + + # Create a repo to hold the subproject and generate a junction element for it + ref = generate_junction(tmpdir, subproject_path, junction_path) + + # Create a stack element to depend on a cross junction element, using + # colon (:) as the separator + element = { + 'kind': 'stack', + 'depends': ['junction.bst:import-etc.bst:foo.bst'] + } + _yaml.dump(element, element_path) + + # Now try to build it, this should fail as recursive lookups for + # cross-junction elements is not allowed. + result = cli.run(project=project, args=['build', 'junction-dep.bst']) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA) -- cgit v1.2.1