element.py: Name normalisation & artifact path constructer helpers

By extracting the functionality from the Element() it allows
the removal of code duplication for artifact assertion in
ArtifactShare(), via a new get_artifact_name() method in Cli().

13 files changed, 79 insertions, 64 deletions

diff --git a/buildstream/element.py b/buildstream/element.py
index 7f68af262..91b6ca63f 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -198,7 +198,7 @@ class Element(Plugin):
         if not self.__is_junction:
             project.ensure_fully_loaded()
 
-        self.normal_name = os.path.splitext(self.name.replace(os.sep, '-'))[0]
+        self.normal_name = _get_normal_name(self.name)
         """A normalized element name
 
         This is the original element without path separators or
@@ -620,15 +620,7 @@ class Element(Plugin):
 
         assert key is not None
 
-        valid_chars = string.digits + string.ascii_letters + '-._'
-        element_name = ''.join([
-            x if x in valid_chars else '_'
-            for x in self.normal_name
-        ])
-
-        # Note that project names are not allowed to contain slashes. Element names containing
-        # a '/' will have this replaced with a '-' upon Element object instantiation.
-        return '{0}/{1}/{2}'.format(project.name, element_name, key)
+        return _compose_artifact_name(project.name, self.normal_name, key)
 
     def stage_artifact(self, sandbox, *, path=None, include=None, exclude=None, orphans=True, update_mtimes=None):
         """Stage this element's output artifact in the sandbox
@@ -3017,3 +3009,42 @@ def _overlap_error_detail(f, forbidden_overlap_elements, elements):
                         " above ".join(reversed(elements))))
     else:
         return ""
+
+
+# _get_normal_name():
+#
+# Get the element name without path separators or
+# the extension.
+#
+# Args:
+#     element_name (str): The element's name
+#
+# Returns:
+#     (str): The normalised element name
+#
+def _get_normal_name(element_name):
+    return os.path.splitext(element_name.replace(os.sep, '-'))[0]
+
+
+# _compose_artifact_name():
+#
+# Compose the completely resolved 'artifact_name' as a filepath
+#
+# Args:
+#     project_name (str): The project's name
+#     normal_name (str): The element's normalised name
+#     cache_key (str): The relevant cache key
+#
+# Returns:
+#     (str): The constructed artifact name path
+#
+def _compose_artifact_name(project_name, normal_name, cache_key):
+    valid_chars = string.digits + string.ascii_letters + '-._'
+    normal_name = ''.join([
+        x if x in valid_chars else '_'
+        for x in normal_name
+    ])
+
+    # Note that project names are not allowed to contain slashes. Element names containing
+    # a '/' will have this replaced with a '-' upon Element object instantiation.
+    return '{0}/{1}/{2}'.format(project_name, normal_name, cache_key)
diff --git a/buildstream/testing/runcli.py b/buildstream/testing/runcli.py
index 934c31236..82bab55b5 100644
--- a/buildstream/testing/runcli.py
+++ b/buildstream/testing/runcli.py
@@ -53,6 +53,7 @@ from _pytest.capture import MultiCapture, FDCapture, FDCaptureBinary
 from buildstream._frontend import cli as bst_cli
 from buildstream import _yaml
 from buildstream._cas import CASCache
+from buildstream.element import _get_normal_name, _compose_artifact_name
 
 # Special private exception accessor, for test case purposes
 from buildstream._exceptions import BstError, get_last_exception, get_last_task_error
@@ -495,6 +496,17 @@ class Cli():
         result.assert_success()
         return result.output.splitlines()
 
+    # Fetch an element's complete artifact name, cache_key will be generated
+    # if not given.
+    #
+    def get_artifact_name(self, project, project_name, element_name, cache_key=None):
+        if not cache_key:
+            cache_key = self.get_element_key(project, element_name)
+
+        # Replace path separator and chop off the .bst suffix for normal name
+        normal_name = _get_normal_name(element_name)
+        return _compose_artifact_name(project_name, normal_name, cache_key)
+
 
 class CliIntegration(Cli):
 
diff --git a/tests/artifactcache/junctions.py b/tests/artifactcache/junctions.py
index 1eb67b659..d2eceb842 100644
--- a/tests/artifactcache/junctions.py
+++ b/tests/artifactcache/junctions.py
@@ -20,8 +20,7 @@ def assert_shared(cli, share, project_name, project, element_name):
     # NOTE: 'test' here is the name of the project
     # specified in the project.conf we are testing with.
     #
-    cache_key = cli.get_element_key(project, element_name)
-    if not share.has_artifact(project_name, element_name, cache_key):
+    if not share.has_artifact(cli.get_artifact_name(project, project_name, element_name)):
         raise AssertionError("Artifact share at {} does not contain the expected element {}"
                              .format(share.repo, element_name))
 
diff --git a/tests/artifactcache/pull.py b/tests/artifactcache/pull.py
index 40fed7637..4f34156d7 100644
--- a/tests/artifactcache/pull.py
+++ b/tests/artifactcache/pull.py
@@ -81,8 +81,7 @@ def test_pull(cli, tmpdir, datafiles):
         # Assert that we are now cached locally
         assert cli.get_element_state(project_dir, 'target.bst') == 'cached'
         # Assert that we shared/pushed the cached artifact
-        element_key = cli.get_element_key(project_dir, 'target.bst')
-        assert share.has_artifact('test', 'target.bst', element_key)
+        assert share.has_artifact(cli.get_artifact_name(project_dir, 'test', 'target.bst'))
 
         # Delete the artifact locally
         cli.remove_artifact_from_cache(project_dir, 'target.bst')
@@ -191,8 +190,7 @@ def test_pull_tree(cli, tmpdir, datafiles):
         # Assert that we are now cached locally
         assert cli.get_element_state(project_dir, 'target.bst') == 'cached'
         # Assert that we shared/pushed the cached artifact
-        element_key = cli.get_element_key(project_dir, 'target.bst')
-        assert share.has_artifact('test', 'target.bst', element_key)
+        assert share.has_artifact(cli.get_artifact_name(project_dir, 'test', 'target.bst'))
 
         # Fake minimal context
         context = Context()
diff --git a/tests/artifactcache/push.py b/tests/artifactcache/push.py
index 4c1d2cd79..a099ad136 100644
--- a/tests/artifactcache/push.py
+++ b/tests/artifactcache/push.py
@@ -99,7 +99,7 @@ def test_push(cli, tmpdir, datafiles):
             raise
 
         assert not error
-        assert share.has_artifact('test', 'target.bst', element_key)
+        assert share.has_artifact(cli.get_artifact_name(project_dir, 'test', 'target.bst', cache_key=element_key))
 
 
 def _test_push(user_config_file, project_dir, element_name, element_key, queue):
diff --git a/tests/frontend/artifact.py b/tests/frontend/artifact.py
index 10cb4f513..716c4b8a1 100644
--- a/tests/frontend/artifact.py
+++ b/tests/frontend/artifact.py
@@ -193,8 +193,7 @@ def test_artifact_delete_pulled_artifact_without_buildtree(cli, tmpdir, datafile
         result.assert_success()
 
         # Make sure it's in the share
-        cache_key = cli.get_element_key(project, element)
-        assert remote.has_artifact('test', element, cache_key)
+        assert remote.has_artifact(cli.get_artifact_name(project, 'test', element))
 
         # Delete and then pull the artifact (without its buildtree)
         result = cli.run(project=project, args=['artifact', 'delete', element])
diff --git a/tests/frontend/pull.py b/tests/frontend/pull.py
index cc62afe92..bf25ab1af 100644
--- a/tests/frontend/pull.py
+++ b/tests/frontend/pull.py
@@ -23,8 +23,7 @@ def assert_shared(cli, share, project, element_name):
     # NOTE: 'test' here is the name of the project
     # specified in the project.conf we are testing with.
     #
-    cache_key = cli.get_element_key(project, element_name)
-    if not share.has_artifact('test', element_name, cache_key):
+    if not share.has_artifact(cli.get_artifact_name(project, 'test', element_name)):
         raise AssertionError("Artifact share at {} does not contain the expected element {}"
                              .format(share.repo, element_name))
 
@@ -35,8 +34,7 @@ def assert_not_shared(cli, share, project, element_name):
     # NOTE: 'test' here is the name of the project
     # specified in the project.conf we are testing with.
     #
-    cache_key = cli.get_element_key(project, element_name)
-    if share.has_artifact('test', element_name, cache_key):
+    if share.has_artifact(cli.get_artifact_name(project, 'test', element_name)):
         raise AssertionError("Artifact share at {} unexpectedly contains the element {}"
                              .format(share.repo, element_name))
 
diff --git a/tests/frontend/push.py b/tests/frontend/push.py
index 67c53f2bb..a9b072cf7 100644
--- a/tests/frontend/push.py
+++ b/tests/frontend/push.py
@@ -44,8 +44,7 @@ def assert_shared(cli, share, project, element_name):
     # NOTE: 'test' here is the name of the project
     # specified in the project.conf we are testing with.
     #
-    cache_key = cli.get_element_key(project, element_name)
-    if not share.has_artifact('test', element_name, cache_key):
+    if not share.has_artifact(cli.get_artifact_name(project, 'test', element_name)):
         raise AssertionError("Artifact share at {} does not contain the expected element {}"
                              .format(share.repo, element_name))
 
@@ -56,8 +55,7 @@ def assert_not_shared(cli, share, project, element_name):
     # NOTE: 'test' here is the name of the project
     # specified in the project.conf we are testing with.
     #
-    cache_key = cli.get_element_key(project, element_name)
-    if share.has_artifact('test', element_name, cache_key):
+    if share.has_artifact(cli.get_artifact_name(project, 'test', element_name)):
         raise AssertionError("Artifact share at {} unexpectedly contains the element {}"
                              .format(share.repo, element_name))
 
@@ -400,7 +398,7 @@ def test_push_cross_junction(cli, tmpdir, datafiles):
         cli.run(project=project, args=['artifact', 'push', 'junction.bst:import-etc.bst'])
 
         cache_key = cli.get_element_key(project, 'junction.bst:import-etc.bst')
-        assert share.has_artifact('subtest', 'import-etc.bst', cache_key)
+        assert share.has_artifact(cli.get_artifact_name(project, 'subtest', 'import-etc.bst', cache_key=cache_key))
 
 
 @pytest.mark.datafiles(DATA_DIR)
diff --git a/tests/integration/artifact.py b/tests/integration/artifact.py
index a5e1f4d77..cb9f070d5 100644
--- a/tests/integration/artifact.py
+++ b/tests/integration/artifact.py
@@ -65,11 +65,11 @@ def test_cache_buildtrees(cli, tmpdir, datafiles):
         result = cli.run(project=project, args=['build', element_name])
         assert result.exit_code == 0
         assert cli.get_element_state(project, element_name) == 'cached'
-        assert share1.has_artifact('test', element_name, cli.get_element_key(project, element_name))
+        assert share1.has_artifact(cli.get_artifact_name(project, 'test', element_name))
 
         # The buildtree dir should not exist, as we set the config to not cache buildtrees.
         cache_key = cli.get_element_key(project, element_name)
-        elementdigest = share1.has_artifact('test', element_name, cache_key)
+        elementdigest = share1.has_artifact(cli.get_artifact_name(project, 'test', element_name, cache_key=cache_key))
         with cli.artifact.extract_buildtree(tmpdir, elementdigest) as buildtreedir:
             assert not os.path.isdir(buildtreedir)
 
@@ -102,10 +102,10 @@ def test_cache_buildtrees(cli, tmpdir, datafiles):
         result = cli.run(project=project, args=['--cache-buildtrees', 'always', 'build', element_name])
         assert result.exit_code == 0
         assert cli.get_element_state(project, element_name) == 'cached'
-        assert share2.has_artifact('test', element_name, cli.get_element_key(project, element_name))
+        assert share2.has_artifact(cli.get_artifact_name(project, 'test', element_name))
 
         # Cache key will be the same however the digest hash will have changed as expected, so reconstruct paths
-        elementdigest = share2.has_artifact('test', element_name, cache_key)
+        elementdigest = share2.has_artifact(cli.get_artifact_name(project, 'test', element_name, cache_key=cache_key))
         with cli.artifact.extract_buildtree(tmpdir, elementdigest) as buildtreedir:
             assert os.path.isdir(buildtreedir)
             assert os.listdir(buildtreedir)
@@ -132,8 +132,7 @@ def test_cache_buildtrees(cli, tmpdir, datafiles):
         result = cli.run(project=project, args=['build', element_name])
         assert result.exit_code == 0
         assert cli.get_element_state(project, element_name) == 'cached'
-        cache_key = cli.get_element_key(project, element_name)
-        elementdigest = share3.has_artifact('test', element_name, cache_key)
+        elementdigest = share3.has_artifact(cli.get_artifact_name(project, 'test', element_name))
         with cli.artifact.extract_buildtree(tmpdir, elementdigest) as buildtreedir:
             assert os.path.isdir(buildtreedir)
             assert os.listdir(buildtreedir)
diff --git a/tests/integration/cachedfail.py b/tests/integration/cachedfail.py
index 28174353d..489e48379 100644
--- a/tests/integration/cachedfail.py
+++ b/tests/integration/cachedfail.py
@@ -161,7 +161,7 @@ def test_push_cached_fail(cli, tmpdir, datafiles, on_error):
         # This element should have failed
         assert cli.get_element_state(project, 'element.bst') == 'failed'
         # This element should have been pushed to the remote
-        assert share.has_artifact('test', 'element.bst', cli.get_element_key(project, 'element.bst'))
+        assert share.has_artifact(cli.get_artifact_name(project, 'test', 'element.bst'))
 
 
 @pytest.mark.skipif(not (IS_LINUX and HAVE_BWRAP), reason='Only available with bubblewrap on Linux')
diff --git a/tests/integration/pullbuildtrees.py b/tests/integration/pullbuildtrees.py
index 91acff4a3..dfef18e7f 100644
--- a/tests/integration/pullbuildtrees.py
+++ b/tests/integration/pullbuildtrees.py
@@ -55,7 +55,7 @@ def test_pullbuildtrees(cli2, tmpdir, datafiles):
         result = cli2.run(project=project, args=['build', element_name])
         assert result.exit_code == 0
         assert cli2.get_element_state(project, element_name) == 'cached'
-        assert share1.has_artifact('test', element_name, cli2.get_element_key(project, element_name))
+        assert share1.has_artifact(cli2.get_artifact_name(project, 'test', element_name))
         default_state(cli2, tmpdir, share1)
 
         # Pull artifact with default config, assert that pulling again
@@ -75,7 +75,7 @@ def test_pullbuildtrees(cli2, tmpdir, datafiles):
         # Also assert that the buildtree is added to the local CAS.
         result = cli2.run(project=project, args=['artifact', 'pull', element_name])
         assert element_name in result.get_pulled_elements()
-        elementdigest = share1.has_artifact('test', element_name, cli2.get_element_key(project, element_name))
+        elementdigest = share1.has_artifact(cli2.get_artifact_name(project, 'test', element_name))
         with cli2.artifact.extract_buildtree(tmpdir, elementdigest) as buildtreedir:
             assert not buildtreedir
         result = cli2.run(project=project, args=['--pull-buildtrees', 'artifact', 'pull', element_name])
@@ -115,7 +115,7 @@ def test_pullbuildtrees(cli2, tmpdir, datafiles):
         cli2.configure({'artifacts': {'url': share2.repo, 'push': True}})
         result = cli2.run(project=project, args=['artifact', 'push', element_name])
         assert element_name not in result.get_pushed_elements()
-        assert not share2.has_artifact('test', element_name, cli2.get_element_key(project, element_name))
+        assert not share2.has_artifact(cli2.get_artifact_name(project, 'test', element_name))
 
         # Assert that after pulling the missing buildtree the element artifact can be
         # successfully pushed to the remote. This will attempt to pull the buildtree
@@ -126,7 +126,7 @@ def test_pullbuildtrees(cli2, tmpdir, datafiles):
         cli2.configure({'artifacts': {'url': share2.repo, 'push': True}})
         result = cli2.run(project=project, args=['artifact', 'push', element_name])
         assert element_name in result.get_pushed_elements()
-        assert share2.has_artifact('test', element_name, cli2.get_element_key(project, element_name))
+        assert share2.has_artifact(cli2.get_artifact_name(project, 'test', element_name))
         default_state(cli2, tmpdir, share1)
 
         # Assert that bst artifact push will automatically attempt to pull a missing buildtree
@@ -142,7 +142,7 @@ def test_pullbuildtrees(cli2, tmpdir, datafiles):
         with cli2.artifact.extract_buildtree(tmpdir, elementdigest) as buildtreedir:
             assert not buildtreedir
         assert element_name not in result.get_pushed_elements()
-        assert not share3.has_artifact('test', element_name, cli2.get_element_key(project, element_name))
+        assert not share3.has_artifact(cli2.get_artifact_name(project, 'test', element_name))
 
         # Assert that if we add an extra remote that has the buildtree artfact cached, bst artifact push will
         # automatically attempt to pull it and will be successful, leading to the full artifact being pushed
@@ -155,7 +155,7 @@ def test_pullbuildtrees(cli2, tmpdir, datafiles):
         with cli2.artifact.extract_buildtree(tmpdir, elementdigest) as buildtreedir:
             assert os.path.isdir(buildtreedir)
         assert element_name in result.get_pushed_elements()
-        assert share3.has_artifact('test', element_name, cli2.get_element_key(project, element_name))
+        assert share3.has_artifact(cli2.get_artifact_name(project, 'test', element_name))
 
 
 # Ensure that only valid pull-buildtrees boolean options make it through the loading
diff --git a/tests/integration/shellbuildtrees.py b/tests/integration/shellbuildtrees.py
index 3d59c78b9..d03344992 100644
--- a/tests/integration/shellbuildtrees.py
+++ b/tests/integration/shellbuildtrees.py
@@ -225,7 +225,7 @@ def test_buildtree_options(cli, tmpdir, datafiles):
         result = cli.run(project=project, args=['--cache-buildtrees', 'always', 'build', element_name])
         result.assert_success()
         assert cli.get_element_state(project, element_name) == 'cached'
-        assert share.has_artifact('test', element_name, cli.get_element_key(project, element_name))
+        assert share.has_artifact(cli.get_artifact_name(project, 'test', element_name))
 
         # Discard the cache
         shutil.rmtree(str(os.path.join(str(tmpdir), 'cache', 'cas')))
diff --git a/tests/testutils/artifactshare.py b/tests/testutils/artifactshare.py
index 6c484ceb7..fca01497a 100644
--- a/tests/testutils/artifactshare.py
+++ b/tests/testutils/artifactshare.py
@@ -1,4 +1,3 @@
-import string
 import os
 import shutil
 import signal
@@ -122,31 +121,13 @@ class ArtifactShare():
     # Checks whether the artifact is present in the share
     #
     # Args:
-    #    project_name (str): The project name
-    #    element_name (str): The element name
-    #    cache_key (str): The cache key
+    #    artifact_name (str): The composed complete artifact name
     #
     # Returns:
     #    (str): artifact digest if the artifact exists in the share, otherwise None.
-    def has_artifact(self, project_name, element_name, cache_key):
-
-        # NOTE: This should be kept in line with our
-        #       artifact cache code, the below is the
-        #       same algo for creating an artifact reference
-        #
-
-        # Replace path separator and chop off the .bst suffix
-        element_name = os.path.splitext(element_name.replace(os.sep, '-'))[0]
-
-        valid_chars = string.digits + string.ascii_letters + '-._'
-        element_name = ''.join([
-            x if x in valid_chars else '_'
-            for x in element_name
-        ])
-        artifact_key = '{0}/{1}/{2}'.format(project_name, element_name, cache_key)
-
+    def has_artifact(self, artifact_name):
         try:
-            tree = self.cas.resolve_ref(artifact_key)
+            tree = self.cas.resolve_ref(artifact_name)
             reachable = set()
             try:
                 self.cas._reachable_refs_dir(reachable, tree, update_mtime=False, check_exists=True)