Merge branch 'juerg/workspaced-dependencies' into 'master'

Fix key invalidation for workspaced dependencies

Closes #461

See merge request BuildStream/buildstream!740

2 files changed, 89 insertions, 7 deletions

diff --git a/buildstream/element.py b/buildstream/element.py
index d9c096992..0bc35ce38 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -1101,9 +1101,12 @@ class Element(Plugin):
             # until the full cache query below.
             if (not self.__assemble_scheduled and not self.__assemble_done and
                     not self.__cached_success(keystrength=_KeyStrength.WEAK) and
-                    not self._pull_pending() and self._is_required()):
-                self._schedule_assemble()
-                return
+                    not self._pull_pending()):
+                # For uncached workspaced elements, assemble is required
+                # even if we only need the cache key
+                if self._is_required() or self._get_workspace():
+                    self._schedule_assemble()
+                    return
 
         if self.__strict_cache_key is None:
             dependencies = [
@@ -1126,13 +1129,17 @@ class Element(Plugin):
                 self.__weak_cached = self.__artifacts.contains(self, self.__weak_cache_key)
 
         if (not self.__assemble_scheduled and not self.__assemble_done and
-                not self._cached_success() and not self._pull_pending() and self._is_required()):
+                not self._cached_success() and not self._pull_pending()):
             # Workspaced sources are considered unstable if a build is pending
             # as the build will modify the contents of the workspace.
             # Determine as early as possible if a build is pending to discard
             # unstable cache keys.
-            self._schedule_assemble()
-            return
+
+            # For uncached workspaced elements, assemble is required
+            # even if we only need the cache key
+            if self._is_required() or self._get_workspace():
+                self._schedule_assemble()
+                return
 
         if self.__cache_key is None:
             # Calculate strong cache key
@@ -1430,7 +1437,6 @@ class Element(Plugin):
     # in a subprocess.
     #
     def _schedule_assemble(self):
-        assert self._is_required()
         assert not self.__assemble_scheduled
         self.__assemble_scheduled = True
 
@@ -1438,6 +1444,8 @@ class Element(Plugin):
         for dep in self.dependencies(Scope.BUILD, recurse=False):
             dep._set_required()
 
+        self._set_required()
+
         # Invalidate workspace key as the build modifies the workspace directory
         workspace = self._get_workspace()
         if workspace:
@@ -1661,6 +1669,10 @@ class Element(Plugin):
     #   (bool): Whether a pull operation is pending
     #
     def _pull_pending(self):
+        if self._get_workspace():
+            # Workspace builds are never pushed to artifact servers
+            return False
+
         if self.__strong_cached:
             # Artifact already in local cache
             return False
diff --git a/tests/frontend/workspace.py b/tests/frontend/workspace.py
index bd4bacd74..a6dd54648 100644
--- a/tests/frontend/workspace.py
+++ b/tests/frontend/workspace.py
@@ -780,3 +780,73 @@ def test_inconsitent_pipeline_message(cli, tmpdir, datafiles, kind):
         'build', element_name
     ])
     result.assert_main_error(ErrorDomain.PIPELINE, "inconsistent-pipeline-workspaced")
+
+
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.parametrize("kind", repo_kinds)
+@pytest.mark.parametrize("strict", [("strict"), ("non-strict")])
+def test_cache_key_workspace_in_dependencies(cli, tmpdir, datafiles, kind, strict):
+    checkout = os.path.join(str(tmpdir), 'checkout')
+    element_name, project, workspace = open_workspace(cli, os.path.join(str(tmpdir), 'repo-a'), datafiles, kind, False)
+
+    element_path = os.path.join(project, 'elements')
+    back_dep_element_name = 'workspace-test-{}-back-dep.bst'.format(kind)
+
+    # Write out our test target
+    element = {
+        'kind': 'compose',
+        'depends': [
+            {
+                'filename': element_name,
+                'type': 'build'
+            }
+        ]
+    }
+    _yaml.dump(element,
+               os.path.join(element_path,
+                            back_dep_element_name))
+
+    # Modify workspace
+    shutil.rmtree(os.path.join(workspace, 'usr', 'bin'))
+    os.makedirs(os.path.join(workspace, 'etc'))
+    with open(os.path.join(workspace, 'etc', 'pony.conf'), 'w') as f:
+        f.write("PONY='pink'")
+
+    # Configure strict mode
+    strict_mode = True
+    if strict != 'strict':
+        strict_mode = False
+    cli.configure({
+        'projects': {
+            'test': {
+                'strict': strict_mode
+            }
+        }
+    })
+
+    # Build artifact with dependency's modified workspace
+    assert cli.get_element_state(project, element_name) == 'buildable'
+    assert cli.get_element_key(project, element_name) == "{:?<64}".format('')
+    assert cli.get_element_state(project, back_dep_element_name) == 'waiting'
+    assert cli.get_element_key(project, back_dep_element_name) == "{:?<64}".format('')
+    result = cli.run(project=project, args=['build', back_dep_element_name])
+    result.assert_success()
+    assert cli.get_element_state(project, element_name) == 'cached'
+    assert cli.get_element_key(project, element_name) != "{:?<64}".format('')
+    assert cli.get_element_state(project, back_dep_element_name) == 'cached'
+    assert cli.get_element_key(project, back_dep_element_name) != "{:?<64}".format('')
+    result = cli.run(project=project, args=['build', back_dep_element_name])
+    result.assert_success()
+
+    # Checkout the result
+    result = cli.run(project=project, args=[
+        'checkout', back_dep_element_name, checkout
+    ])
+    result.assert_success()
+
+    # Check that the pony.conf from the modified workspace exists
+    filename = os.path.join(checkout, 'etc', 'pony.conf')
+    assert os.path.exists(filename)
+
+    # Check that the original /usr/bin/hello is not in the checkout
+    assert not os.path.exists(os.path.join(checkout, 'usr', 'bin', 'hello'))