From 0ca33d853ec19ce8243e45295cc1acaed3724614 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sat, 31 Aug 2019 13:28:28 +0300 Subject: element.py: Use recursive element names for strict rebuild dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As discussed with Jürg on gitlab[0], it makes more sense to store the recursive element names and record the shape of the dependencies for non strict cache keys as well as strict ones. [0]: https://gitlab.com/BuildStream/buildstream/merge_requests/1542#note_205598556 --- src/buildstream/element.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buildstream/element.py b/src/buildstream/element.py index 10c8320fa..837ad2fd6 100644 --- a/src/buildstream/element.py +++ b/src/buildstream/element.py @@ -3116,7 +3116,7 @@ class Element(Plugin): ] else: dependencies = [ - [e.project_name, e.name] for e in self.dependencies(Scope.BUILD, recurse=False) + [e.project_name, e.name] for e in self.dependencies(Scope.BUILD) ] self.__weak_cache_key = self._calculate_cache_key(dependencies) -- cgit v1.2.1 From cc51e4a2d4f338b4dd49337432170b4e30ebe8d0 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Mon, 12 Aug 2019 15:33:53 -0400 Subject: Implement strict dependency semantics This patch allows specifying a dependency as `strict`, e.g.: build-depends: - filename: element.bst strict: true This allows finer tuning of projects which want to leverage the non-strict build mode; dependencies which are statically linked to, or who's content is otherwise included verbatim in the resulting output, should be marked `strict` to ensure these bits get reassembled if necessary when building in non-strict mode. This fixes #254 Change summary: o _loader/loadelement.pyx: Added 'strict' attribute to Dependency o _loader/types.pyx: Added 'strict' attribute to Dependency do the parsing work. o _loader/metaelement.py: Added 'strict_dependencies' list o _loader/loader.py: Resolve the 'strict_dependencies' list o element.py: Added __strict_dependencies list, and use this to conditionally use weak cache keys in place of names for the purpose of building the weak cache key (in the case of dependencies which are marked as strict). --- src/buildstream/_loader/loadelement.pyx | 7 +++++-- src/buildstream/_loader/loader.py | 6 ++++-- src/buildstream/_loader/metaelement.py | 1 + src/buildstream/_loader/types.pyx | 15 +++++++++++++-- src/buildstream/element.py | 30 ++++++++++++++++++++---------- 5 files changed, 43 insertions(+), 16 deletions(-) diff --git a/src/buildstream/_loader/loadelement.pyx b/src/buildstream/_loader/loadelement.pyx index 867de6f29..fb1dd1d15 100644 --- a/src/buildstream/_loader/loadelement.pyx +++ b/src/buildstream/_loader/loadelement.pyx @@ -39,18 +39,21 @@ cdef int _next_synthetic_counter(): # A link from a LoadElement to its dependencies. # # Keeps a link to one of the current Element's dependencies, together with -# its dependency type. +# its dependency attributes. # # Args: # element (LoadElement): a LoadElement on which there is a dependency # dep_type (str): the type of dependency this dependency link is +# strict (bint): whether the dependency is strict cdef class Dependency: cdef readonly LoadElement element cdef readonly str dep_type + cdef readonly bint strict - def __cinit__(self, LoadElement element, str dep_type): + def __cinit__(self, LoadElement element, str dep_type, bint strict): self.element = element self.dep_type = dep_type + self.strict = strict # LoadElement(): diff --git a/src/buildstream/_loader/loader.py b/src/buildstream/_loader/loader.py index 061d28bf0..89458d40c 100644 --- a/src/buildstream/_loader/loader.py +++ b/src/buildstream/_loader/loader.py @@ -124,7 +124,7 @@ class Loader(): dummy_target = LoadElement(Node.from_dict({}), "", self) # Pylint is not very happy with Cython and can't understand 'dependencies' is a list dummy_target.dependencies.extend( # pylint: disable=no-member - Dependency(element, Symbol.RUNTIME) + Dependency(element, Symbol.RUNTIME, False) for element in target_elements ) @@ -344,7 +344,7 @@ class Loader(): # All is well, push the dependency onto the LoadElement # Pylint is not very happy with Cython and can't understand 'dependencies' is a list current_element[0].dependencies.append( # pylint: disable=no-member - Dependency(dep_element, dep.dep_type)) + Dependency(dep_element, dep.dep_type, dep.strict)) else: # We do not have any more dependencies to load for this # element on the queue, report any invalid dep names @@ -499,6 +499,8 @@ class Loader(): meta_element.build_dependencies.append(meta_dep) if dep.dep_type != 'build': meta_element.dependencies.append(meta_dep) + if dep.strict: + meta_element.strict_dependencies.append(meta_dep) element.meta_done = True diff --git a/src/buildstream/_loader/metaelement.py b/src/buildstream/_loader/metaelement.py index 67d2ec771..00d8560f8 100644 --- a/src/buildstream/_loader/metaelement.py +++ b/src/buildstream/_loader/metaelement.py @@ -56,5 +56,6 @@ class MetaElement(): self.sandbox = sandbox or Node.from_dict({}) self.build_dependencies = [] self.dependencies = [] + self.strict_dependencies = [] self.first_pass = first_pass self.is_junction = kind == "junction" diff --git a/src/buildstream/_loader/types.pyx b/src/buildstream/_loader/types.pyx index 0a223f9a9..cd1302b45 100644 --- a/src/buildstream/_loader/types.pyx +++ b/src/buildstream/_loader/types.pyx @@ -44,6 +44,7 @@ class Symbol(): DIRECTORY = "directory" JUNCTION = "junction" SANDBOX = "sandbox" + STRICT = "strict" # Dependency() @@ -63,6 +64,7 @@ cdef class Dependency: cdef public str name cdef public str dep_type cdef public str junction + cdef public bint strict def __init__(self, Node dep, @@ -75,13 +77,14 @@ cdef class Dependency: self.name = dep.as_str() self.dep_type = default_dep_type self.junction = None + self.strict = False elif type(dep) is MappingNode: if default_dep_type: - ( dep).validate_keys(['filename', 'junction']) + ( dep).validate_keys(['filename', 'junction', 'strict']) dep_type = default_dep_type else: - ( dep).validate_keys(['filename', 'type', 'junction']) + ( dep).validate_keys(['filename', 'type', 'junction', 'strict']) # Make type optional, for this we set it to None dep_type = ( dep).get_str( Symbol.TYPE, None) @@ -95,11 +98,19 @@ cdef class Dependency: self.name = ( dep).get_str( Symbol.FILENAME) self.dep_type = dep_type self.junction = ( dep).get_str( Symbol.JUNCTION, None) + self.strict = ( dep).get_bool( Symbol.STRICT, False) else: raise LoadError("{}: Dependency is not specified as a string or a dictionary".format(self.provenance), LoadErrorReason.INVALID_DATA) + # Only build dependencies are allowed to be strict + # + if self.strict and self.dep_type == Symbol.RUNTIME: + raise LoadError("{}: Runtime dependency {} specified as `strict`.".format(self.provenance, self.name), + 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: diff --git a/src/buildstream/element.py b/src/buildstream/element.py index 837ad2fd6..633e29c88 100644 --- a/src/buildstream/element.py +++ b/src/buildstream/element.py @@ -208,6 +208,7 @@ class Element(Plugin): self.__runtime_dependencies = [] # Direct runtime dependency Elements self.__build_dependencies = [] # Direct build dependency Elements + self.__strict_dependencies = [] # Direct build dependency subset which require strict rebuilds self.__reverse_build_deps = set() # Direct reverse build dependency Elements self.__reverse_runtime_deps = set() # Direct reverse runtime dependency Elements self.__build_deps_without_strict_cache_key = None # Number of build dependencies without a strict key @@ -989,6 +990,10 @@ class Element(Plugin): dependency = Element._new_from_meta(meta_dep, task) element.__build_dependencies.append(dependency) dependency.__reverse_build_deps.add(element) + + if meta_dep in meta.strict_dependencies: + element.__strict_dependencies.append(dependency) + no_of_build_deps = len(element.__build_dependencies) element.__build_deps_without_strict_cache_key = no_of_build_deps element.__build_deps_without_cache_key = no_of_build_deps @@ -3107,17 +3112,22 @@ class Element(Plugin): if self.__weak_cache_key is None: # Calculate weak cache key + # # Weak cache key includes names of direct build dependencies - # but does not include keys of dependencies. - if self.BST_STRICT_REBUILD: - dependencies = [ - e._get_cache_key(strength=_KeyStrength.WEAK) - for e in self.dependencies(Scope.BUILD) - ] - else: - dependencies = [ - [e.project_name, e.name] for e in self.dependencies(Scope.BUILD) - ] + # so as to only trigger rebuilds when the shape of the + # dependencies change. + # + # Some conditions cause dependencies to be strict, such + # that this element will be rebuilt anyway if the dependency + # changes even in non strict mode, for these cases we just + # encode the dependency's weak cache key instead of it's name. + # + dependencies = [ + e._get_cache_key(strength=_KeyStrength.WEAK) + if self.BST_STRICT_REBUILD or e in self.__strict_dependencies + else [e.project_name, e.name] + for e in self.dependencies(Scope.BUILD) + ] self.__weak_cache_key = self._calculate_cache_key(dependencies) -- cgit v1.2.1 From f538fce83a0b902f6097c9a02b273cf2091b106e Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Mon, 12 Aug 2019 15:54:59 -0400 Subject: doc/source/format_declaring.rst: Documenting strict dependencies This adds documentation on the new keyword `strict` in dependency declarations, and adds a link to the strict mode user config section. --- doc/source/format_declaring.rst | 11 +++++++++++ doc/source/using_config.rst | 2 ++ 2 files changed, 13 insertions(+) diff --git a/doc/source/format_declaring.rst b/doc/source/format_declaring.rst index a38973347..80365c6d5 100644 --- a/doc/source/format_declaring.rst +++ b/doc/source/format_declaring.rst @@ -370,6 +370,7 @@ Dependency dictionary: - filename: foo.bst type: build junction: baseproject.bst + strict: false Attributes: @@ -400,6 +401,16 @@ Attributes: The ``junction`` attribute is available since :ref:`format version 1 ` +* ``strict`` + + This attribute can be used to specify that this element should + be rebuilt when the dependency changes, even when + :ref:`strict mode ` has been turned off. + + This is appropriate whenever a dependency's output is consumed + verbatim in the output of the depending element, for instance + when static linking is in use. + Cross-junction dependencies ~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/doc/source/using_config.rst b/doc/source/using_config.rst index 8a6b9ac76..967dde2ed 100644 --- a/doc/source/using_config.rst +++ b/doc/source/using_config.rst @@ -201,6 +201,8 @@ configuration will be used as fallback. instance-name: main +.. _user_config_strict_mode: + Strict build plan ~~~~~~~~~~~~~~~~~ The strict build plan option decides whether you want elements -- cgit v1.2.1 From 9c7435bf408db49d16915e6d808c6f746232be3b Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Mon, 12 Aug 2019 16:08:13 -0400 Subject: tests/format/dependencies.py: Testing for error of invalid strict runtime deps --- tests/format/dependencies.py | 7 +++++++ tests/format/dependencies1/elements/invalidstrict.bst | 9 +++++++++ 2 files changed, 16 insertions(+) create mode 100644 tests/format/dependencies1/elements/invalidstrict.bst diff --git a/tests/format/dependencies.py b/tests/format/dependencies.py index e8c50d3bc..e8be68a5d 100644 --- a/tests/format/dependencies.py +++ b/tests/format/dependencies.py @@ -50,6 +50,13 @@ def test_invalid_dependency_type(cli, datafiles): result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA) +@pytest.mark.datafiles(DATA_DIR) +def test_invalid_strict_dependency(cli, datafiles): + project = os.path.join(str(datafiles), 'dependencies1') + result = cli.run(project=project, args=['show', 'invalidstrict.bst']) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA) + + @pytest.mark.datafiles(DATA_DIR) def test_circular_dependency(cli, datafiles): project = os.path.join(str(datafiles), 'dependencies1') diff --git a/tests/format/dependencies1/elements/invalidstrict.bst b/tests/format/dependencies1/elements/invalidstrict.bst new file mode 100644 index 000000000..8893b4aa9 --- /dev/null +++ b/tests/format/dependencies1/elements/invalidstrict.bst @@ -0,0 +1,9 @@ +kind: manual +description: | + + This is an invalid strict dependency because runtime + dependencies cannot be strict. + +runtime-depends: +- filename: firstdep.bst + strict: true -- cgit v1.2.1 From 4d36ca3a3407a29afff2006167dd83f788759ec0 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Mon, 12 Aug 2019 16:53:35 -0400 Subject: tests/frontend/show.py: Test that strict dependencies behave as expected This tests that the target which depends on a common dependency strictly in non strict mode needs to be rebuilt after this common dependency changes, while it is not the case when depending on the same common target non strictly. This is a regression test for #254 --- tests/frontend/show.py | 54 ++++++++++++++++++++++ tests/frontend/strict-depends/elements/base.bst | 5 ++ .../strict-depends/elements/non-strict-depends.bst | 4 ++ .../strict-depends/elements/strict-depends.bst | 5 ++ tests/frontend/strict-depends/files/hello.txt | 1 + tests/frontend/strict-depends/project.conf | 2 + 6 files changed, 71 insertions(+) create mode 100644 tests/frontend/strict-depends/elements/base.bst create mode 100644 tests/frontend/strict-depends/elements/non-strict-depends.bst create mode 100644 tests/frontend/strict-depends/elements/strict-depends.bst create mode 100644 tests/frontend/strict-depends/files/hello.txt create mode 100644 tests/frontend/strict-depends/project.conf diff --git a/tests/frontend/show.py b/tests/frontend/show.py index 0aa720681..0d444a925 100644 --- a/tests/frontend/show.py +++ b/tests/frontend/show.py @@ -560,3 +560,57 @@ def test_max_jobs(cli, datafiles, cli_value, config_value): else: # Check that we got the explicitly set value assert loaded_value == int(expected_value) + + +# This tests that cache keys behave as expected when +# dependencies have been specified as `strict` and +# when building in strict mode. +# +# This test will: +# +# * Build the target once (and assert that it is cached) +# * Modify some local files which are imported +# by an import element which the target depends on +# * Assert that the cached state of the target element +# is as expected +# +# We run the test twice, once with an element which strict +# depends on the changing import element, and one which +# depends on it regularly. +# +@pytest.mark.datafiles(os.path.join(DATA_DIR, 'strict-depends')) +@pytest.mark.parametrize("target, expected_state", [ + ("non-strict-depends.bst", "cached"), + ("strict-depends.bst", "waiting"), +]) +def test_strict_dependencies(cli, datafiles, target, expected_state): + project = str(datafiles) + + # Configure non strict mode, this will have + # an effect on the build and the `bst show` + # commands run via cli.get_element_states() + cli.configure({ + 'projects': { + 'test': { + 'strict': False + } + } + }) + + result = cli.run(project=project, silent=True, args=['build', target]) + result.assert_success() + + states = cli.get_element_states(project, ['base.bst', target]) + assert states['base.bst'] == 'cached' + assert states[target] == 'cached' + + # Now modify the file, effectively causing the common base.bst + # dependency to change it's cache key + hello_path = os.path.join(project, 'files', 'hello.txt') + with open(hello_path, 'w') as f: + f.write("Goodbye") + + # Now assert that we have the states we expect as a result + states = cli.get_element_states(project, ['base.bst', target]) + assert states['base.bst'] == 'buildable' + assert states[target] == expected_state diff --git a/tests/frontend/strict-depends/elements/base.bst b/tests/frontend/strict-depends/elements/base.bst new file mode 100644 index 000000000..ed6197699 --- /dev/null +++ b/tests/frontend/strict-depends/elements/base.bst @@ -0,0 +1,5 @@ +kind: import + +sources: +- kind: local + path: files diff --git a/tests/frontend/strict-depends/elements/non-strict-depends.bst b/tests/frontend/strict-depends/elements/non-strict-depends.bst new file mode 100644 index 000000000..9ab119bde --- /dev/null +++ b/tests/frontend/strict-depends/elements/non-strict-depends.bst @@ -0,0 +1,4 @@ +kind: stack + +build-depends: +- base.bst diff --git a/tests/frontend/strict-depends/elements/strict-depends.bst b/tests/frontend/strict-depends/elements/strict-depends.bst new file mode 100644 index 000000000..1e4e29414 --- /dev/null +++ b/tests/frontend/strict-depends/elements/strict-depends.bst @@ -0,0 +1,5 @@ +kind: stack + +build-depends: +- filename: base.bst + strict: true diff --git a/tests/frontend/strict-depends/files/hello.txt b/tests/frontend/strict-depends/files/hello.txt new file mode 100644 index 000000000..f62144808 --- /dev/null +++ b/tests/frontend/strict-depends/files/hello.txt @@ -0,0 +1 @@ +pony diff --git a/tests/frontend/strict-depends/project.conf b/tests/frontend/strict-depends/project.conf new file mode 100644 index 000000000..627522526 --- /dev/null +++ b/tests/frontend/strict-depends/project.conf @@ -0,0 +1,2 @@ +name: test +element-path: elements -- cgit v1.2.1 From 97c520fdcf75a4b11ea3df22f88cbc564c1c6357 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sat, 31 Aug 2019 14:54:33 +0300 Subject: _loader/types.pyx: Disallow explicitly setting 'strict' to False This is in order to keep the door open to allowing the project.conf set the default of dependency 'strict'-ness which might be useful for projects which use mostly static linking and the like, in which case we can later interpret explicitly non-strict dependencies as an override of the project default. --- src/buildstream/_loader/types.pyx | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/buildstream/_loader/types.pyx b/src/buildstream/_loader/types.pyx index cd1302b45..42a7b801f 100644 --- a/src/buildstream/_loader/types.pyx +++ b/src/buildstream/_loader/types.pyx @@ -100,6 +100,19 @@ cdef class Dependency: self.junction = ( dep).get_str( Symbol.JUNCTION, None) self.strict = ( dep).get_bool( Symbol.STRICT, False) + # Here we disallow explicitly setting 'strict' to False. + # + # This is in order to keep the door open to allowing the project.conf + # set the default of dependency 'strict'-ness which might be useful + # for projects which use mostly static linking and the like, in which + # case we can later interpret explicitly non-strict dependencies + # as an override of the project default. + # + if self.strict == False and Symbol.STRICT in dep: + provenance = dep.get_scalar(Symbol.STRICT).get_provenance() + raise LoadError("{}: Setting 'strict' to False is unsupported" + .format(provenance), LoadErrorReason.INVALID_DATA) + else: raise LoadError("{}: Dependency is not specified as a string or a dictionary".format(self.provenance), LoadErrorReason.INVALID_DATA) -- cgit v1.2.1 From 4caab108812b6ecc06cbb9ecc28bb604fb585a63 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sat, 31 Aug 2019 15:04:56 +0300 Subject: tests/format/dependencies.py: Test errors when explicitly setting strict to True --- tests/format/dependencies.py | 7 +++++++ tests/format/dependencies1/elements/invalidnonstrict.bst | 10 ++++++++++ 2 files changed, 17 insertions(+) create mode 100644 tests/format/dependencies1/elements/invalidnonstrict.bst diff --git a/tests/format/dependencies.py b/tests/format/dependencies.py index e8be68a5d..f92b89afa 100644 --- a/tests/format/dependencies.py +++ b/tests/format/dependencies.py @@ -57,6 +57,13 @@ def test_invalid_strict_dependency(cli, datafiles): result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA) +@pytest.mark.datafiles(DATA_DIR) +def test_invalid_non_strict_dependency(cli, datafiles): + project = os.path.join(str(datafiles), 'dependencies1') + result = cli.run(project=project, args=['show', 'invalidnonstrict.bst']) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA) + + @pytest.mark.datafiles(DATA_DIR) def test_circular_dependency(cli, datafiles): project = os.path.join(str(datafiles), 'dependencies1') diff --git a/tests/format/dependencies1/elements/invalidnonstrict.bst b/tests/format/dependencies1/elements/invalidnonstrict.bst new file mode 100644 index 000000000..3cb31a077 --- /dev/null +++ b/tests/format/dependencies1/elements/invalidnonstrict.bst @@ -0,0 +1,10 @@ +kind: manual +description: | + + This is an invalid non strict dependency because it is + currently illegal to explicitly set a dependency to be + non-strict (even though this is the default). + +depends: +- filename: firstdep.bst + strict: false -- cgit v1.2.1