From 39148926f39f393977c69819b3190a69154e5d07 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sat, 31 Aug 2019 15:22:00 +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 --- buildstream/element.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/buildstream/element.py b/buildstream/element.py index 5f0e300e1..ff0fcd8d9 100644 --- a/buildstream/element.py +++ b/buildstream/element.py @@ -1077,7 +1077,7 @@ class Element(Plugin): ] else: dependencies = [ - e.name for e in self.dependencies(Scope.BUILD, recurse=False) + e.name for e in self.dependencies(Scope.BUILD) ] self.__weak_cache_key = self.__calculate_cache_key(dependencies) -- cgit v1.2.1 From 1b663ed9a8e487050926ffdd628366c611d4d838 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sat, 31 Aug 2019 15:35:52 +0300 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/types.pyx: Added 'strict' attribute to Dependency and 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). --- buildstream/_loader/loader.py | 2 ++ buildstream/_loader/metaelement.py | 1 + buildstream/_loader/types.py | 14 ++++++++++++-- buildstream/element.py | 28 ++++++++++++++++++---------- 4 files changed, 33 insertions(+), 12 deletions(-) diff --git a/buildstream/_loader/loader.py b/buildstream/_loader/loader.py index b861e86cd..ec929eadb 100644 --- a/buildstream/_loader/loader.py +++ b/buildstream/_loader/loader.py @@ -474,6 +474,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) return meta_element diff --git a/buildstream/_loader/metaelement.py b/buildstream/_loader/metaelement.py index c13d5591e..370ce553b 100644 --- a/buildstream/_loader/metaelement.py +++ b/buildstream/_loader/metaelement.py @@ -54,4 +54,5 @@ class MetaElement(): self.sandbox = sandbox self.build_dependencies = [] self.dependencies = [] + self.strict_dependencies = [] self.first_pass = first_pass diff --git a/buildstream/_loader/types.py b/buildstream/_loader/types.py index eb6932b0b..5b4d0121e 100644 --- a/buildstream/_loader/types.py +++ b/buildstream/_loader/types.py @@ -46,6 +46,7 @@ class Symbol(): DIRECTORY = "directory" JUNCTION = "junction" SANDBOX = "sandbox" + STRICT = "strict" # Dependency() @@ -68,13 +69,14 @@ class Dependency(): self.name = dep self.dep_type = default_dep_type self.junction = None + self.strict = False elif isinstance(dep, Mapping): if default_dep_type: - _yaml.node_validate(dep, ['filename', 'junction']) + _yaml.node_validate(dep, ['filename', 'junction', 'strict']) dep_type = default_dep_type else: - _yaml.node_validate(dep, ['filename', 'type', 'junction']) + _yaml.node_validate(dep, ['filename', 'type', 'junction', 'strict']) # Make type optional, for this we set it to None dep_type = _yaml.node_get(dep, str, Symbol.TYPE, default_value=None) @@ -89,11 +91,19 @@ class Dependency(): 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) + self.strict = _yaml.node_get(dep, bool, Symbol.STRICT, default_value=False) else: raise LoadError(LoadErrorReason.INVALID_DATA, "{}: Dependency is not specified as a string or a dictionary".format(provenance)) + # Only build dependencies are allowed to be strict + # + if self.strict and self.dep_type == Symbol.RUNTIME: + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Runtime dependency {} specified as `strict`.".format(self.provenance, self.name), + 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/buildstream/element.py b/buildstream/element.py index ff0fcd8d9..703f062da 100644 --- a/buildstream/element.py +++ b/buildstream/element.py @@ -201,6 +201,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_dependencies = set() # Direct reverse dependency Elements self.__ready_for_runtime = False # Wether the element has all its dependencies ready and has a cache key self.__sources = [] # List of Sources @@ -939,6 +940,9 @@ class Element(Plugin): element.__build_dependencies.append(dependency) dependency.__reverse_dependencies.add(element) + if meta_dep in meta.strict_dependencies: + element.__strict_dependencies.append(dependency) + return element # _get_redundant_source_refs() @@ -1069,16 +1073,20 @@ 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.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.name + for e in self.dependencies(Scope.BUILD) + ] self.__weak_cache_key = self.__calculate_cache_key(dependencies) -- cgit v1.2.1 From 1d4561b89e0c22dbb72839d3762974fdf037b27b 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 f69d4a479..5c9d64030 100644 --- a/doc/source/format_declaring.rst +++ b/doc/source/format_declaring.rst @@ -350,6 +350,7 @@ Dependency dictionary: - filename: foo.bst type: build junction: baseproject.bst + strict: false Attributes: @@ -380,6 +381,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 c707cd04b..8b6cb58b0 100644 --- a/doc/source/using_config.rst +++ b/doc/source/using_config.rst @@ -65,6 +65,8 @@ by the project. If you give a list of URLs, earlier entries in the list will have higher priority than later ones. +.. _user_config_strict_mode: + Strict build plan ~~~~~~~~~~~~~~~~~ The strict build plan option decides whether you want elements -- cgit v1.2.1 From b082f9aad9b33c1dcbbc457a1ae1c0b7f31f6258 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sat, 31 Aug 2019 15:49:31 +0300 Subject: tests/loader/dependencies.py: Testing for error of invalid strict runtime deps --- tests/loader/dependencies.py | 11 +++++++++++ tests/loader/dependencies/elements/invalidstrict.bst | 9 +++++++++ 2 files changed, 20 insertions(+) create mode 100644 tests/loader/dependencies/elements/invalidstrict.bst diff --git a/tests/loader/dependencies.py b/tests/loader/dependencies.py index cb750fcb1..1a8365259 100644 --- a/tests/loader/dependencies.py +++ b/tests/loader/dependencies.py @@ -123,6 +123,17 @@ def test_invalid_dependency_type(datafiles): assert (exc.value.reason == LoadErrorReason.INVALID_DATA) +@pytest.mark.datafiles(DATA_DIR) +def test_invalid_strict_dependency(cli, datafiles): + basedir = os.path.join(datafiles.dirname, datafiles.basename) + loader = make_loader(basedir) + + with pytest.raises(LoadError) as exc: + element = loader.load(['elements/invalidstrict.bst'])[0] + + assert (exc.value.reason == LoadErrorReason.INVALID_DATA) + + @pytest.mark.datafiles(DATA_DIR) def test_build_dependency(datafiles): basedir = os.path.join(datafiles.dirname, datafiles.basename) diff --git a/tests/loader/dependencies/elements/invalidstrict.bst b/tests/loader/dependencies/elements/invalidstrict.bst new file mode 100644 index 000000000..8893b4aa9 --- /dev/null +++ b/tests/loader/dependencies/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 5044fd257046073fc6f1c43029c71bef07b8cbe7 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 | 60 ++++++++++++++++++++-- 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, 73 insertions(+), 4 deletions(-) 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 8bb0bab3f..92fa1f5f7 100644 --- a/tests/frontend/show.py +++ b/tests/frontend/show.py @@ -11,10 +11,8 @@ from buildstream._exceptions import ErrorDomain, LoadErrorReason from . import configure_project # Project directory -DATA_DIR = os.path.join( - os.path.dirname(os.path.realpath(__file__)), - "project", -) +TOP_DIR = os.path.dirname(os.path.realpath(__file__)) +DATA_DIR = os.path.join(TOP_DIR, "project") @pytest.mark.datafiles(DATA_DIR) @@ -371,3 +369,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(TOP_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, 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, 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 ce4fe960180d1719b691d6e1895d240c5ab2087c Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sat, 31 Aug 2019 16:01:50 +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. --- buildstream/_loader/types.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/buildstream/_loader/types.py b/buildstream/_loader/types.py index 5b4d0121e..a49ff380b 100644 --- a/buildstream/_loader/types.py +++ b/buildstream/_loader/types.py @@ -93,6 +93,20 @@ class Dependency(): self.junction = _yaml.node_get(dep, str, Symbol.JUNCTION, default_value=None) self.strict = _yaml.node_get(dep, bool, Symbol.STRICT, default_value=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 = _yaml.node_get_provenance(dep, key=Symbol.STRICT) + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Setting 'strict' to False is unsupported" + .format(provenance)) + else: raise LoadError(LoadErrorReason.INVALID_DATA, "{}: Dependency is not specified as a string or a dictionary".format(provenance)) -- cgit v1.2.1 From 3471d122c45b6056ba20b46f7995c9f61affb954 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sat, 31 Aug 2019 16:05:48 +0300 Subject: tests/loader/dependencies.py: Test errors when explicitly setting strict to True --- tests/loader/dependencies.py | 11 +++++++++++ tests/loader/dependencies/elements/invalidnonstrict.bst | 10 ++++++++++ 2 files changed, 21 insertions(+) create mode 100644 tests/loader/dependencies/elements/invalidnonstrict.bst diff --git a/tests/loader/dependencies.py b/tests/loader/dependencies.py index 1a8365259..e4b9c73ed 100644 --- a/tests/loader/dependencies.py +++ b/tests/loader/dependencies.py @@ -134,6 +134,17 @@ def test_invalid_strict_dependency(cli, datafiles): assert (exc.value.reason == LoadErrorReason.INVALID_DATA) +@pytest.mark.datafiles(DATA_DIR) +def test_invalid_non_strict_dependency(cli, datafiles): + basedir = os.path.join(datafiles.dirname, datafiles.basename) + loader = make_loader(basedir) + + with pytest.raises(LoadError) as exc: + element = loader.load(['elements/invalidnonstrict.bst'])[0] + + assert (exc.value.reason == LoadErrorReason.INVALID_DATA) + + @pytest.mark.datafiles(DATA_DIR) def test_build_dependency(datafiles): basedir = os.path.join(datafiles.dirname, datafiles.basename) diff --git a/tests/loader/dependencies/elements/invalidnonstrict.bst b/tests/loader/dependencies/elements/invalidnonstrict.bst new file mode 100644 index 000000000..3cb31a077 --- /dev/null +++ b/tests/loader/dependencies/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 From 8d06e863b75c79bc5bdf5c6cd35a80056ada9a3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jard=C3=B3n?= Date: Mon, 2 Sep 2019 11:48:00 +0100 Subject: buildstream/_loader/types.py: Fix style for E712 --- buildstream/_loader/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/buildstream/_loader/types.py b/buildstream/_loader/types.py index a49ff380b..193fb5da9 100644 --- a/buildstream/_loader/types.py +++ b/buildstream/_loader/types.py @@ -101,7 +101,7 @@ class Dependency(): # 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: + if self.strict is False and Symbol.STRICT in dep: provenance = _yaml.node_get_provenance(dep, key=Symbol.STRICT) raise LoadError(LoadErrorReason.INVALID_DATA, "{}: Setting 'strict' to False is unsupported" -- cgit v1.2.1