source.py: Validate cache when it's used, not in `_is_cached()`

`_is_cached()` is indirectly called by the frontend, which is not
optimal for handling per-plugin errors. Instead, call `validate_cache()`
right before the cache is used: in fetch jobs and when opening a
workspace.

2 files changed, 23 insertions, 40 deletions

diff --git a/src/buildstream/source.py b/src/buildstream/source.py
index f15d5a628..4c284a57c 100644
--- a/src/buildstream/source.py
+++ b/src/buildstream/source.py
@@ -539,10 +539,10 @@ class Source(Plugin):
         """Implement any validations once we know the sources are cached
 
         This is guaranteed to be called only once for a given session
-        once the sources are known to be cached.
-        If source tracking is enabled in the session for this source,
-        then this will only be called if the sources become cached after
-        tracking completes.
+        once the sources are known to be cached, before
+        :func:`Source.stage() <buildstream.source.Source.stage>` or
+        :func:`Source.init_workspace() <buildstream.source.Source.init_workspace>`
+        is called.
         """
 
     def is_cached(self) -> bool:
@@ -780,9 +780,6 @@ class Source(Plugin):
                     reason="source-bug",
                 )
 
-            if self.__is_cached:
-                self.validate_cache()
-
         return self.__is_cached
 
     # Wrapper function around plugin provided fetch method
@@ -800,8 +797,6 @@ class Source(Plugin):
         else:
             self.__do_fetch()
 
-        self.validate_cache()
-
     # _fetch_done()
     #
     # Indicates that fetching the source has been done.
@@ -831,6 +826,7 @@ class Source(Plugin):
             cas_dir = CasBasedDirectory(self._get_context().get_cascache(), digest=self.__digest)
             directory.import_files(cas_dir)
         else:
+            self.validate_cache()
             self.stage(directory)
 
     # Wrapper for init_workspace()
@@ -840,6 +836,7 @@ class Source(Plugin):
 
         directory = self.__ensure_directory(directory)
 
+        self.validate_cache()
         self.init_workspace(directory)
 
     # _get_unique_key():
diff --git a/tests/sources/git.py b/tests/sources/git.py
index 033db1bf9..30657d825 100644
--- a/tests/sources/git.py
+++ b/tests/sources/git.py
@@ -498,9 +498,8 @@ def test_unlisted_submodule(cli, tmpdir, datafiles, fail):
     element = {"kind": "import", "sources": [gitsource]}
     generate_element(project, "target.bst", element)
 
-    # We will not see the warning or error before the first fetch, because
-    # we don't have the repository yet and so we have no knowledge of
-    # the unlisted submodule.
+    # The warning or error is reported during fetch. There should be no
+    # error with `bst show`.
     result = cli.run(project=project, args=["show", "target.bst"])
     result.assert_success()
     assert "git:unlisted-submodule" not in result.stderr
@@ -517,17 +516,10 @@ def test_unlisted_submodule(cli, tmpdir, datafiles, fail):
         result.assert_success()
         assert "git:unlisted-submodule" in result.stderr
 
-    # Now that we've fetched it, `bst show` will discover the unlisted submodule too
+    # Verify that `bst show` will still not error out after fetching.
     result = cli.run(project=project, args=["show", "target.bst"])
-
-    # Assert a warning or an error depending on what we're checking
-    if fail == "error":
-        result.assert_main_error(ErrorDomain.PLUGIN, "git:unlisted-submodule")
-    else:
-        result.assert_success()
-        # We have cached things internally and successfully. Therefore, the plugin
-        # is not involved in checking whether the cache is correct or not.
-        assert "git:unlisted-submodule" not in result.stderr
+    result.assert_success()
+    assert "git:unlisted-submodule" not in result.stderr
 
 
 @pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
@@ -612,9 +604,8 @@ def test_invalid_submodule(cli, tmpdir, datafiles, fail):
     element = {"kind": "import", "sources": [gitsource]}
     generate_element(project, "target.bst", element)
 
-    # We will not see the warning or error before the first fetch, because
-    # we don't have the repository yet and so we have no knowledge of
-    # the unlisted submodule.
+    # The warning or error is reported during fetch. There should be no
+    # error with `bst show`.
     result = cli.run(project=project, args=["show", "target.bst"])
     result.assert_success()
     assert "git:invalid-submodule" not in result.stderr
@@ -631,17 +622,10 @@ def test_invalid_submodule(cli, tmpdir, datafiles, fail):
         result.assert_success()
         assert "git:invalid-submodule" in result.stderr
 
-    # Now that we've fetched it, `bst show` will discover the unlisted submodule too
+    # Verify that `bst show` will still not error out after fetching.
     result = cli.run(project=project, args=["show", "target.bst"])
-
-    # Assert a warning or an error depending on what we're checking
-    if fail == "error":
-        result.assert_main_error(ErrorDomain.PLUGIN, "git:invalid-submodule")
-    else:
-        result.assert_success()
-        # We have cached things internally and successfully. Therefore, the plugin
-        # is not involved in checking whether the cache is correct or not.
-        assert "git:invalid-submodule" not in result.stderr
+    result.assert_success()
+    assert "git:invalid-submodule" not in result.stderr
 
 
 @pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
@@ -683,12 +667,14 @@ def test_track_invalid_submodule(cli, tmpdir, datafiles, fail):
     result.assert_success()
     assert "git:invalid-submodule" not in result.stderr
 
-    # In this case, we will get the error directly after tracking,
-    # since the new HEAD does not require any submodules which are
-    # not locally cached, the Source will be CACHED directly after
-    # tracking and the validations will occur as a result.
-    #
+    # After tracking we're pointing to a ref, which would trigger an invalid
+    # submodule warning. However, cache validation is only performed as part
+    # of fetch.
     result = cli.run(project=project, args=["source", "track", "target.bst"])
+    result.assert_success()
+
+    # Fetch to trigger cache validation
+    result = cli.run(project=project, args=["source", "fetch", "target.bst"])
     if fail == "error":
         result.assert_main_error(ErrorDomain.STREAM, None)
         result.assert_task_error(ErrorDomain.PLUGIN, "git:invalid-submodule")