element.py: Instantiate the Artifact objects within update_state()

The Artifact instance within Element is now initialised when the
relevant keys are available, with the weak & strong keys being
in the constructor. It also introduces the notion of a strict_artifact,
which is used to make a distinction of the Artifact keys and cached
state dependant on the context defined strictness. The keys are updated
when a relevant key change occurs. As the keys are now internalised
within Artifact, the majority of key determinsim and parameterisation
in public Artifact method calls are now redundant.

2 files changed, 83 insertions, 109 deletions

diff --git a/buildstream/_artifact.py b/buildstream/_artifact.py
index 0eb198387..4b35518d1 100644
--- a/buildstream/_artifact.py
+++ b/buildstream/_artifact.py
@@ -32,8 +32,9 @@ import os
 import shutil
 
 from . import _yaml
+from . import utils
 from ._exceptions import ArtifactError
-from .types import Scope, _KeyStrength
+from .types import Scope
 from .storage._casbaseddirectory import CasBasedDirectory
 
 
@@ -43,47 +44,43 @@ from .storage._casbaseddirectory import CasBasedDirectory
 # Args:
 #     element (Element): The Element object
 #     context (Context): The BuildStream context
+#     strong_key (str): The elements strong cache key, dependant on context
+#     weak_key (str): The elements weak cache key
 #
 class Artifact():
 
-    def __init__(self, element, context):
+    def __init__(self, element, context, *, strong_key=None, weak_key=None):
         self._element = element
         self._context = context
         self._artifacts = context.artifactcache
+        self._cache_key = strong_key
+        self._weak_cache_key = weak_key
 
     # get_files():
     #
     # Get a virtual directory for the artifact files content
     #
-    # Args:
-    #    key (str): The key for the artifact to extract,
-    #               or None for the default key
-    #
     # Returns:
     #    (Directory): The virtual directory object
     #    (str): The chosen key
     #
-    def get_files(self, key=None):
+    def get_files(self):
         subdir = "files"
 
-        return self._get_subdirectory(subdir, key)
+        return self._get_subdirectory(subdir)
 
     # get_buildtree():
     #
     # Get a virtual directory for the artifact buildtree content
     #
-    # Args:
-    #    key (str): The key for the artifact to extract,
-    #               or None for the default key
-    #
     # Returns:
     #    (Directory): The virtual directory object
     #    (str): The chosen key
     #
-    def get_buildtree(self, key=None):
+    def get_buildtree(self):
         subdir = "buildtree"
 
-        return self._get_subdirectory(subdir, key)
+        return self._get_subdirectory(subdir)
 
     # get_extract_key():
     #
@@ -93,17 +90,7 @@ class Artifact():
     #    (str): The key
     #
     def get_extract_key(self):
-
-        element = self._element
-        context = self._context
-
-        # Use weak cache key, if context allows use of weak cache keys
-        key_strength = _KeyStrength.STRONG
-        key = element._get_cache_key(strength=key_strength)
-        if not context.get_strict() and not key:
-            key = element._get_cache_key(strength=_KeyStrength.WEAK)
-
-        return key
+        return self._cache_key or self._weak_cache_key
 
     # cache():
     #
@@ -120,7 +107,7 @@ class Artifact():
     # Returns:
     #    (int): The size of the newly cached artifact
     #
-    def cache(self, rootdir, sandbox_build_dir, collectvdir, buildresult, keys, publicdata):
+    def cache(self, rootdir, sandbox_build_dir, collectvdir, buildresult, publicdata):
 
         context = self._context
         element = self._element
@@ -163,8 +150,8 @@ class Artifact():
 
         # Store keys.yaml
         _yaml.dump(_yaml.node_sanitize({
-            'strong': element._get_cache_key(),
-            'weak': element._get_cache_key(_KeyStrength.WEAK),
+            'strong': self._cache_key,
+            'weak': self._weak_cache_key,
         }), os.path.join(metadir, 'keys.yaml'))
 
         # Store dependencies.yaml
@@ -189,6 +176,7 @@ class Artifact():
         logsvdir.import_files(logsdir)
 
         artifact_size = assemblevdir.get_size()
+        keys = utils._deduplicate([self._cache_key, self._weak_cache_key])
         self._artifacts.commit(element, assemblevdir, keys)
 
         return artifact_size
@@ -207,15 +195,13 @@ class Artifact():
     #
     def cached_buildtree(self):
 
-        context = self._context
         element = self._element
 
         if not element._cached():
             return False
 
-        key_strength = _KeyStrength.STRONG if context.get_strict() else _KeyStrength.WEAK
-        if not self._artifacts.contains_subdir_artifact(element, element._get_cache_key(strength=key_strength),
-                                                        'buildtree'):
+        key = self.get_extract_key()
+        if not self._artifacts.contains_subdir_artifact(element, key, 'buildtree'):
             return False
 
         return True
@@ -256,18 +242,14 @@ class Artifact():
     #
     # Load the build result from the cached artifact
     #
-    # Args:
-    #    key (str): The key for the artifact to extract
-    #
     # Returns:
     #    (bool): Whether the artifact of this element present in the artifact cache is of a success
     #    (str): Short description of the result
     #    (str): Detailed description of the result
     #
-    def load_build_result(self, key):
+    def load_build_result(self):
 
-        assert key is not None
-        meta_vdir, _ = self._get_subdirectory('meta', key)
+        meta_vdir, _ = self._get_subdirectory('meta')
 
         meta_file = meta_vdir._objpath('build-result.yaml')
         if not os.path.exists(meta_file):
@@ -289,16 +271,16 @@ class Artifact():
     # Retrieve the strong and weak keys from the given artifact.
     #
     # Args:
-    #    key (str): The artifact key, or None for the default key
     #    metadata_keys (dict): The elements cached strong/weak
     #                          metadata keys, empty if not yet cached
+    #    key (str): The artifact key, or None for the default key
     #
     # Returns:
     #    (str): The strong key
     #    (str): The weak key
     #    (dict): The key dict, None if not updated
     #
-    def get_metadata_keys(self, key, metadata_keys):
+    def get_metadata_keys(self, metadata_keys, key=None):
 
         # Now extract it and possibly derive the key
         artifact_vdir, key = self._get_directory(key)
@@ -351,7 +333,7 @@ class Artifact():
         meta = _yaml.load(meta_file, shortname='meta/dependencies.yaml')
 
         # Cache it under both strong and weak keys
-        strong_key, weak_key, metadata_keys = self.get_metadata_keys(key, metadata_keys)
+        strong_key, weak_key, metadata_keys = self.get_metadata_keys(metadata_keys, key=key)
         metadata_dependencies[strong_key] = _yaml.node_sanitize(meta)
         metadata_dependencies[weak_key] = _yaml.node_sanitize(meta)
 
@@ -362,7 +344,6 @@ class Artifact():
     # Retrieve the hash of dependency from the given artifact.
     #
     # Args:
-    #    key (str): The artifact key, or None for the default key
     #    meta_data_workspaced (dict): The elements cached boolean metadata
     #                                 of whether it's workspaced, empty if
     #                                 not yet cached
@@ -374,10 +355,10 @@ class Artifact():
     #    (dict): The workspaced key dict, None if not updated
     #    (dict): The elements key dict, None if not updated
     #
-    def get_metadata_workspaced(self, key, metadata_workspaced, metadata_keys):
+    def get_metadata_workspaced(self, metadata_workspaced, metadata_keys):
 
         # Extract it and possibly derive the key
-        artifact_vdir, key = self._get_directory(key)
+        artifact_vdir, key = self._get_directory()
 
         # Now try the cache, once we're sure about the key
         if key in metadata_workspaced:
@@ -389,7 +370,7 @@ class Artifact():
         workspaced = _yaml.node_get(meta, bool, 'workspaced')
 
         # Cache it under both strong and weak keys
-        strong_key, weak_key, metadata_keys = self.get_metadata_keys(key, metadata_keys)
+        strong_key, weak_key, metadata_keys = self.get_metadata_keys(metadata_keys)
         metadata_workspaced[strong_key] = workspaced
         metadata_workspaced[weak_key] = workspaced
 
@@ -400,7 +381,6 @@ class Artifact():
     # Retrieve the hash of workspaced dependencies keys from the given artifact.
     #
     # Args:
-    #    key (str): The artifact key, or None for the default key
     #    metadata_workspaced_dependencies (dict): The elements cached metadata of
     #                                             which dependencies are workspaced,
     #                                             empty if not yet cached
@@ -412,11 +392,11 @@ class Artifact():
     #    (dict): The workspaced depedencies key dict, None if not updated
     #    (dict): The elements key dict, None if not updated
     #
-    def get_metadata_workspaced_dependencies(self, key, metadata_workspaced_dependencies,
+    def get_metadata_workspaced_dependencies(self, metadata_workspaced_dependencies,
                                              metadata_keys):
 
         # Extract it and possibly derive the key
-        artifact_vdir, key = self._get_directory(key)
+        artifact_vdir, key = self._get_directory()
 
         # Now try the cache, once we're sure about the key
         if key in metadata_workspaced_dependencies:
@@ -428,30 +408,28 @@ class Artifact():
         workspaced = _yaml.node_sanitize(_yaml.node_get(meta, list, 'workspaced-dependencies'))
 
         # Cache it under both strong and weak keys
-        strong_key, weak_key, metadata_keys = self.get_metadata_keys(key, metadata_keys)
+        strong_key, weak_key, metadata_keys = self.get_metadata_keys(metadata_keys)
         metadata_workspaced_dependencies[strong_key] = workspaced
         metadata_workspaced_dependencies[weak_key] = workspaced
         return (workspaced, metadata_workspaced_dependencies, metadata_keys)
 
     # cached():
     #
-    # Check whether the artifact corresponding to the specified cache key is
+    # Check whether the artifact corresponding to the stored cache key is
     # available. This also checks whether all required parts of the artifact
-    # are available, which may depend on command and configuration.
+    # are available, which may depend on command and configuration. The cache
+    # key used for querying is dependant on the current context.
     #
     # This is used by _update_state() to set __strong_cached and __weak_cached.
     #
-    # Args:
-    #     key (str): The artifact key
-    #
     # Returns:
     #     (bool): Whether artifact is in local cache
     #
-    def cached(self, key):
+    def cached(self):
         context = self._context
 
         try:
-            vdir, _ = self._get_directory(key)
+            vdir, _ = self._get_directory()
         except ArtifactError:
             # Either ref or top-level artifact directory missing
             return False
@@ -481,18 +459,15 @@ class Artifact():
     #
     # Check if the artifact is cached with log files.
     #
-    # Args:
-    #     key (str): The artifact key
-    #
     # Returns:
     #     (bool): True if artifact is cached with logs, False if
     #             element not cached or missing logs.
     #
-    def cached_logs(self, key=None):
+    def cached_logs(self):
         if not self._element._cached():
             return False
 
-        log_vdir, _ = self._get_subdirectory('logs', key)
+        log_vdir, _ = self._get_subdirectory('logs')
 
         logsdigest = log_vdir._get_digest()
         return self._artifacts.cas.contains_directory(logsdigest, with_files=True)
diff --git a/buildstream/element.py b/buildstream/element.py
index 7fb771697..d9f32bfc8 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -229,8 +229,9 @@ class Element(Plugin):
         self.__required = False                 # Whether the artifact is required in the current session
         self.__artifact_files_required = False  # Whether artifact files are required in the local cache
         self.__build_result = None              # The result of assembling this Element (success, description, detail)
-        self._build_log_path = None            # The path of the build log for this Element
-        self.__artifact = Artifact(self, context)  # Artifact class for direct artifact composite interaction
+        self._build_log_path = None             # The path of the build log for this Element
+        self.__artifact = None                  # Artifact class for direct artifact composite interaction
+        self.__strict_artifact = None           # Artifact for strict cache key
 
         self.__batch_prepare_assemble = False         # Whether batching across prepare()/assemble() is configured
         self.__batch_prepare_assemble_flags = 0       # Sandbox flags for batching across prepare()/assemble()
@@ -1055,7 +1056,7 @@ class Element(Plugin):
     #    (str): Detailed description of the result
     #
     def _get_build_result(self):
-        return self.__get_build_result(keystrength=None)
+        return self.__get_build_result()
 
     # __set_build_result():
     #
@@ -1076,7 +1077,7 @@ class Element(Plugin):
     #            the artifact cache and the element assembled successfully
     #
     def _cached_success(self):
-        return self.__cached_success(keystrength=None)
+        return self.__cached_success()
 
     # _cached_failure():
     #
@@ -1203,7 +1204,10 @@ class Element(Plugin):
                 return
 
             if not context.get_strict():
-                self.__weak_cached = self.__artifact.cached(self.__weak_cache_key)
+                # We've calculated the weak_key, so instantiate artifact instance member
+                self.__artifact = Artifact(self, context, weak_key=self.__weak_cache_key)
+                # and update the weak cached state (required early for workspaces)
+                self.__weak_cached = self.__artifact.cached()
 
         if not context.get_strict():
             # Full cache query in non-strict mode requires both the weak and
@@ -1213,7 +1217,7 @@ class Element(Plugin):
             # are sufficient. However, don't update the `cached` attributes
             # 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._cached_success() and
                     not self._pull_pending()):
                 # For uncached workspaced elements, assemble is required
                 # even if we only need the cache key
@@ -1226,17 +1230,28 @@ class Element(Plugin):
                 e.__strict_cache_key for e in self.dependencies(Scope.BUILD)
             ]
             self.__strict_cache_key = self._calculate_cache_key(dependencies)
+
             if self.__strict_cache_key is None:
                 # Strict cache key could not be calculated yet
                 return
 
-        # Query caches now that the weak and strict cache keys are available
-        key_for_cache_lookup = self.__strict_cache_key if context.get_strict() else self.__weak_cache_key
+            self.__strict_artifact = Artifact(self, context, strong_key=self.__strict_cache_key,
+                                              weak_key=self.__weak_cache_key)
+
+            # In strict mode, the strong cache key always matches the strict cache key
+            if context.get_strict():
+                self.__cache_key = self.__strict_cache_key
+                self.__artifact = self.__strict_artifact
+
+        # Query caches now that the weak and strict cache keys are available.
+        # strong_cached in non-strict mode is only of relevance when querying
+        # if a 'better' artifact could be pulled, which is redudant if we already
+        # have it cached locally with a strict_key. As such strong_cached is only
+        # checked against the 'strict' artifact.
         if not self.__strong_cached:
-            self.__strong_cached = self.__artifact.cached(self.__strict_cache_key)
-        if key_for_cache_lookup == self.__weak_cache_key:
-            if not self.__weak_cached:
-                self.__weak_cached = self.__artifact.cached(self.__weak_cache_key)
+            self.__strong_cached = self.__strict_artifact.cached()
+        if not self.__weak_cached and not context.get_strict():
+            self.__weak_cached = self.__artifact.cached()
 
         if (not self.__assemble_scheduled and not self.__assemble_done and
                 not self._cached_success() and not self._pull_pending()):
@@ -1251,11 +1266,9 @@ class Element(Plugin):
                 self._schedule_assemble()
                 return
 
+        # __cache_key can be None here only in non-strict mode
         if self.__cache_key is None:
-            # Calculate strong cache key
-            if context.get_strict():
-                self.__cache_key = self.__strict_cache_key
-            elif self._pull_pending():
+            if self._pull_pending():
                 # Effective strong cache key is unknown until after the pull
                 pass
             elif self._cached():
@@ -1273,6 +1286,9 @@ class Element(Plugin):
                 # Strong cache key could not be calculated yet
                 return
 
+            # Now we have the strong cache key, update the Artifact
+            self.__artifact._cache_key = self.__cache_key
+
         if not self.__ready_for_runtime and self.__cache_key is not None:
             self.__ready_for_runtime = all(
                 dep.__ready_for_runtime for dep in self.__runtime_dependencies)
@@ -1792,11 +1808,10 @@ class Element(Plugin):
 
         # ensure we have cache keys
         self._assemble_done()
-        keys = self.__get_cache_keys_for_commit()
 
         with self.timed_activity("Caching artifact"):
             artifact_size = self.__artifact.cache(rootdir, sandbox_build_dir, collectvdir,
-                                                  buildresult, keys, publicdata)
+                                                  buildresult, publicdata)
 
         if collect is not None and collectvdir is None:
             raise ElementError(
@@ -2778,18 +2793,15 @@ class Element(Plugin):
     #
     # Retrieve the strong and weak keys from the given artifact.
     #
-    # Args:
-    #     key (str): The artifact key, or None for the default key
-    #
     # Returns:
     #     (str): The strong key
     #     (str): The weak key
     #
-    def __get_artifact_metadata_keys(self, key=None):
+    def __get_artifact_metadata_keys(self):
 
         metadata_keys = self.__metadata_keys
 
-        strong_key, weak_key, metadata_keys = self.__artifact.get_metadata_keys(key, metadata_keys)
+        strong_key, weak_key, metadata_keys = self.__artifact.get_metadata_keys(metadata_keys)
 
         # Update keys if needed
         if metadata_keys:
@@ -2825,17 +2837,14 @@ class Element(Plugin):
     #
     # Retrieve the hash of dependency strong keys from the given artifact.
     #
-    # Args:
-    #     key (str): The artifact key, or None for the default key
-    #
     # Returns:
     #     (bool): Whether the given artifact was workspaced
     #
 
-    def __get_artifact_metadata_workspaced(self, key=None):
+    def __get_artifact_metadata_workspaced(self):
 
         metadata = [self.__metadata_workspaced, self.__metadata_keys]
-        workspaced, meta_workspaced, meta_keys = self.__artifact.get_metadata_workspaced(key, *metadata)
+        workspaced, meta_workspaced, meta_keys = self.__artifact.get_metadata_workspaced(*metadata)
 
         # Update workspaced if needed
         if meta_workspaced:
@@ -2850,17 +2859,14 @@ class Element(Plugin):
     #
     # Retrieve the hash of dependency strong keys from the given artifact.
     #
-    # Args:
-    #     key (str): The artifact key, or None for the default key
-    #
     # Returns:
     #     (list): List of which dependencies are workspaced
     #
-    def __get_artifact_metadata_workspaced_dependencies(self, key=None):
+    def __get_artifact_metadata_workspaced_dependencies(self):
 
         metadata = [self.__metadata_workspaced_dependencies, self.__metadata_keys]
         workspaced, meta_workspaced_deps,\
-            meta_keys = self.__artifact.get_metadata_workspaced_dependencies(key, *metadata)
+            meta_keys = self.__artifact.get_metadata_workspaced_dependencies(*metadata)
 
         # Update workspaced if needed
         if meta_workspaced_deps:
@@ -2881,31 +2887,24 @@ class Element(Plugin):
 
         self.__dynamic_public = self.__artifact.load_public_data()
 
-    def __load_build_result(self, keystrength):
-        self.__assert_cached(keystrength=keystrength)
+    def __load_build_result(self):
+        self.__assert_cached()
         assert self.__build_result is None
 
-        # _get_cache_key with _KeyStrength.STRONG returns self.__cache_key, which can be `None`
-        # leading to a failed assertion from get_artifact_directory() using get_artifact_name(),
-        # so explicility pass self.__strict_cache_key
-        key = self.__weak_cache_key if keystrength is _KeyStrength.WEAK else self.__strict_cache_key
+        self.__build_result = self.__artifact.load_build_result()
 
-        self.__build_result = self.__artifact.load_build_result(key)
-
-    def __get_build_result(self, keystrength):
-        if keystrength is None:
-            keystrength = _KeyStrength.STRONG if self._get_context().get_strict() else _KeyStrength.WEAK
+    def __get_build_result(self):
 
         if self.__build_result is None:
-            self.__load_build_result(keystrength)
+            self.__load_build_result()
 
         return self.__build_result
 
-    def __cached_success(self, keystrength):
-        if not self.__is_cached(keystrength=keystrength):
+    def __cached_success(self):
+        if not self._cached():
             return False
 
-        success, _, _ = self.__get_build_result(keystrength=keystrength)
+        success, _, _ = self.__get_build_result()
         return success
 
     def __get_cache_keys_for_commit(self):