Merge branch 'tristan/strict-rebuild' into 'master'

Support strict build dependencies

Closes #254

See merge request BuildStream/buildstream!1542

16 files changed, 173 insertions, 16 deletions

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 <project_format_version>`
 
+* ``strict``
+
+  This attribute can be used to specify that this element should
+  be rebuilt when the dependency changes, even when
+  :ref:`strict mode <user_config_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
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..42a7b801f 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:
-                (<MappingNode> dep).validate_keys(['filename', 'junction'])
+                (<MappingNode> dep).validate_keys(['filename', 'junction', 'strict'])
                 dep_type = default_dep_type
             else:
-                (<MappingNode> dep).validate_keys(['filename', 'type', 'junction'])
+                (<MappingNode> dep).validate_keys(['filename', 'type', 'junction', 'strict'])
 
                 # Make type optional, for this we set it to None
                 dep_type = (<MappingNode> dep).get_str(<str> Symbol.TYPE, None)
@@ -95,11 +98,32 @@ cdef class Dependency:
             self.name = (<MappingNode> dep).get_str(<str> Symbol.FILENAME)
             self.dep_type = dep_type
             self.junction = (<MappingNode> dep).get_str(<str> Symbol.JUNCTION, None)
+            self.strict = (<MappingNode> dep).get_bool(<str> 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)
 
+        # 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 10c8320fa..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, recurse=False)
-                ]
+            # 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)
 
diff --git a/tests/format/dependencies.py b/tests/format/dependencies.py
index e8c50d3bc..f92b89afa 100644
--- a/tests/format/dependencies.py
+++ b/tests/format/dependencies.py
@@ -51,6 +51,20 @@ def test_invalid_dependency_type(cli, datafiles):
 
 
 @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_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')
     result = cli.run(project=project, args=['show', 'circulartarget.bst'])
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
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
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