Merge branch 'juerg/artifact-pull' into 'master'

Artifact pull code improvements

See merge request BuildStream/buildstream!2074

3 files changed, 43 insertions, 60 deletions

diff --git a/src/buildstream/_artifact.py b/src/buildstream/_artifact.py
index 048f09cc6..c110e57f0 100644
--- a/src/buildstream/_artifact.py
+++ b/src/buildstream/_artifact.py
@@ -462,8 +462,35 @@ class Artifact:
     #
     def set_cached(self):
         self._proto = self._load_proto()
+        assert self._proto
         self._cached = True
 
+    # pull()
+    #
+    # Pull artifact from remote artifact repository into local artifact cache.
+    #
+    # Args:
+    #     pull_buildtrees (bool): Whether to pull buildtrees or not
+    #
+    # Returns: True if the artifact has been downloaded, False otherwise
+    #
+    def pull(self, *, pull_buildtrees):
+        artifacts = self._context.artifactcache
+
+        pull_key = self.get_extract_key()
+
+        if not artifacts.pull(self._element, pull_key, pull_buildtrees=pull_buildtrees):
+            return False
+
+        self.set_cached()
+
+        # Add reference for the other key (weak key when pulling with strong key,
+        # strong key when pulling with weak key)
+        for key in self.get_metadata_keys():
+            artifacts.link_key(self._element, pull_key, key)
+
+        return True
+
     #  load_proto()
     #
     # Returns:
diff --git a/src/buildstream/_artifactcache.py b/src/buildstream/_artifactcache.py
index 2a5f5faa6..3cd34070d 100644
--- a/src/buildstream/_artifactcache.py
+++ b/src/buildstream/_artifactcache.py
@@ -294,8 +294,11 @@ class ArtifactCache(AssetCache):
         oldref = element.get_artifact_name(oldkey)
         newref = element.get_artifact_name(newkey)
 
-        if not os.path.exists(os.path.join(self._basedir, newref)):
-            os.link(os.path.join(self._basedir, oldref), os.path.join(self._basedir, newref))
+        if oldref == newref:
+            # The two refs are identical, nothing to do
+            return
+
+        utils.safe_link(os.path.join(self._basedir, oldref), os.path.join(self._basedir, newref))
 
     # fetch_missing_blobs():
     #
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 9348dff48..5118f0193 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -1945,17 +1945,20 @@ class Element(Plugin):
         pull_buildtrees = context.pull_buildtrees
 
         # Attempt to pull artifact without knowing whether it's available
-        pulled = self.__pull_strong(pull_buildtrees=pull_buildtrees)
-
-        if not pulled and not self._cached() and not context.get_strict():
-            pulled = self.__pull_weak(pull_buildtrees=pull_buildtrees)
+        strict_artifact = Artifact(self, context, strong_key=self.__strict_cache_key, weak_key=self.__weak_cache_key)
+        if strict_artifact.pull(pull_buildtrees=pull_buildtrees):
+            # Notify successful download
+            return True
 
-        if not pulled:
+        if not context.get_strict() and not self._cached():
+            # In non-strict mode also try pulling weak artifact
+            # if no weak artifact is cached yet.
+            artifact = Artifact(self, context, weak_key=self.__weak_cache_key)
+            return artifact.pull(pull_buildtrees=pull_buildtrees)
+        else:
+            # No artifact has been downloaded
             return False
 
-        # Notify successfull download
-        return True
-
     def _skip_source_push(self):
         if not self.sources() or self._get_workspace():
             return True
@@ -3099,56 +3102,6 @@ class Element(Plugin):
 
         self.__build_result = self.__artifact.load_build_result()
 
-    # __pull_strong():
-    #
-    # Attempt pulling given element from configured artifact caches with
-    # the strict cache key
-    #
-    # Args:
-    #     progress (callable): The progress callback, if any
-    #     subdir (str): The optional specific subdir to pull
-    #     excluded_subdirs (list): The optional list of subdirs to not pull
-    #
-    # Returns:
-    #     (bool): Whether or not the pull was successful
-    #
-    def __pull_strong(self, *, pull_buildtrees):
-        weak_key = self._get_cache_key(strength=_KeyStrength.WEAK)
-        key = self.__strict_cache_key
-        if not self.__artifacts.pull(self, key, pull_buildtrees=pull_buildtrees):
-            return False
-
-        # update weak ref by pointing it to this newly fetched artifact
-        self.__artifacts.link_key(self, key, weak_key)
-
-        return True
-
-    # __pull_weak():
-    #
-    # Attempt pulling given element from configured artifact caches with
-    # the weak cache key
-    #
-    # Args:
-    #     subdir (str): The optional specific subdir to pull
-    #     excluded_subdirs (list): The optional list of subdirs to not pull
-    #
-    # Returns:
-    #     (bool): Whether or not the pull was successful
-    #
-    def __pull_weak(self, *, pull_buildtrees):
-        weak_key = self._get_cache_key(strength=_KeyStrength.WEAK)
-        if not self.__artifacts.pull(self, weak_key, pull_buildtrees=pull_buildtrees):
-            return False
-
-        # extract strong cache key from this newly fetched artifact
-        self._pull_done()
-
-        # create tag for strong cache key
-        key = self._get_cache_key(strength=_KeyStrength.STRONG)
-        self.__artifacts.link_key(self, weak_key, key)
-
-        return True
-
     # __update_cache_keys()
     #
     # Updates weak and strict cache keys