Refactor ArtifactElement instantiation

When instantiating an ArtifactElement, use an ArtifactProject to ensure
that the Element does not accidentally have access to any incidentally
existing project loaded from the current working directory.

Also pass along the Artifact to the Element's initializer directly, and
conditionally instantiate the element based on it's artifact instead of
based on loading YAML configuration.

Fixes #1410

Summary of changes:

  * _artifactelement.py:

    - Now load the Artifact and pass it along to the Element constructor
    - Now use an ArtifactProject for the element's project
    - Remove overrides of Element methods, instead of behaving differently,
      now we just fill in all the blanks for an Element to behave more
      naturally when loaded from an artifact.
    - Avoid redundantly loading the same artifact twice, if the artifact
      was cached then we will load only one artifact.

  * element.py:

    - Conditionally instantiate from the passed Artifact instead of
      considering any YAML loading.
    - Error out early in _prepare_sandbox() in case that we are trying
      to instantiate a sandbox for an uncached artifact, in which case
      we don't have any SandboxConfig at hand to do so.

  * _stream.py:

    - Clear the ArtifactProject cache after loading artifacts
    - Ensure we load a list of unique artifacts without any duplicates

  * tests/frontend/buildcheckout.py: Expect a different error when trying
    to checkout an uncached artifact

  * tests/frontend/push.py, tests/frontend/artifact_show.py: No longer expect
    duplicates to show up with wild card statements which would capture multiple
    versions of the same artifact (this changes because of #1410 being fixed)

6 files changed, 169 insertions, 113 deletions

diff --git a/src/buildstream/_artifactelement.py b/src/buildstream/_artifactelement.py
index 3dce09202..63bb904fd 100644
--- a/src/buildstream/_artifactelement.py
+++ b/src/buildstream/_artifactelement.py
@@ -1,5 +1,6 @@
 #
 #  Copyright (C) 2019 Bloomberg Finance LP
+#  Copyright (C) 2020 Codethink Limited
 #
 #  This program is free software; you can redistribute it and/or
 #  modify it under the terms of the GNU Lesser General Public
@@ -16,17 +17,22 @@
 #
 #  Authors:
 #        James Ennis <james.ennis@codethink.co.uk>
+#        Tristan Van Berkom <tristan.vanberkom@codethink.co.uk>
 
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Optional, Dict
+from contextlib import suppress
 
 from . import Element
 from . import _cachekey
+from ._artifact import Artifact
+from ._artifactproject import ArtifactProject
 from ._exceptions import ArtifactElementError
 from ._loader import LoadElement
 from .node import Node
 
 if TYPE_CHECKING:
-    from typing import Dict
+    from ._context import Context
+    from ._state import _Task
 
 
 # ArtifactElement()
@@ -40,51 +46,70 @@ if TYPE_CHECKING:
 class ArtifactElement(Element):
 
     # A hash of ArtifactElement by ref
-    __instantiated_artifacts = {}  # type: Dict[str, ArtifactElement]
+    __instantiated_artifacts: Dict[str, "ArtifactElement"] = {}
 
     def __init__(self, context, ref):
-        _, element, key = verify_artifact_ref(ref)
+        project_name, element_name, key = verify_artifact_ref(ref)
 
-        self._ref = ref
-        self._key = key
+        # At this point we only know the key which was specified on the command line,
+        # so we will pretend all keys are equal.
+        #
+        # If the artifact is cached, then the real keys will be loaded from the
+        # artifact instead.
+        #
+        artifact = Artifact(self, context, strong_key=key, strict_key=key, weak_key=key)
+        project = ArtifactProject(project_name, context)
+        load_element = LoadElement(Node.from_dict({}), element_name, project.loader)  # NOTE element has no .bst suffix
 
-        project = context.get_toplevel_project()
-        load_element = LoadElement(Node.from_dict({}), element, project.loader)  # NOTE element has no .bst suffix
+        super().__init__(context, project, load_element, None, artifact=artifact)
 
-        super().__init__(context, project, load_element, None)
+    ########################################################
+    #                      Public API                      #
+    ########################################################
 
-    # _new_from_artifact_name():
+    # new_from_artifact_name():
     #
     # Recursively instantiate a new ArtifactElement instance, and its
     # dependencies from an artifact name
     #
     # Args:
-    #    ref (String): The artifact ref
-    #    context (Context): The Context object
-    #    task (Task): A task object to report progress to
+    #    artifact_name: The artifact name
+    #    context: The Context object
+    #    task: A task object to report progress to
     #
     # Returns:
     #    (ArtifactElement): A newly created Element instance
     #
     @classmethod
-    def _new_from_artifact_name(cls, ref, context, task=None):
+    def new_from_artifact_name(cls, artifact_name: str, context: "Context", task: Optional["_Task"] = None):
 
-        if ref in cls.__instantiated_artifacts:
-            return cls.__instantiated_artifacts[ref]
+        # Initial lookup for already loaded artifact.
+        with suppress(KeyError):
+            return cls.__instantiated_artifacts[artifact_name]
 
-        artifact_element = ArtifactElement(context, ref)
-        # XXX: We need to call initialize_state as it is responsible for
-        # initialising an Element/ArtifactElement's Artifact (__artifact)
-        artifact_element._initialize_state()
-        cls.__instantiated_artifacts[ref] = artifact_element
+        # Instantiate the element, this can result in having a different
+        # artifact name, if we loaded the artifact by it's weak key then
+        # we will have the artifact loaded via it's strong key.
+        element = ArtifactElement(context, artifact_name)
+        artifact_name = element.get_artifact_name()
 
-        for dep_ref in artifact_element.get_dependency_artifact_names():
-            dependency = ArtifactElement._new_from_artifact_name(dep_ref, context, task)
-            artifact_element._add_build_dependency(dependency)
+        # Perform a second lookup, avoid loading the same artifact
+        # twice, even if we've loaded it both with weak and strong keys.
+        with suppress(KeyError):
+            return cls.__instantiated_artifacts[artifact_name]
 
-        return artifact_element
+        # Now cache the loaded artifact
+        cls.__instantiated_artifacts[artifact_name] = element
 
-    # _clear_artifact_refs_cache()
+        # Walk the dependencies and load recursively
+        artifact = element._get_artifact()
+        for dep_artifact_name in artifact.get_dependency_artifact_names():
+            dependency = ArtifactElement.new_from_artifact_name(dep_artifact_name, context, task)
+            element._add_build_dependency(dependency)
+
+        return element
+
+    # clear_artifact_name_cache()
     #
     # Clear the internal artifact refs cache
     #
@@ -96,53 +121,24 @@ class ArtifactElement(Element):
     # to save memory.
     #
     @classmethod
-    def _clear_artifact_refs_cache(cls):
+    def clear_artifact_name_cache(cls):
         cls.__instantiated_artifacts = {}
 
-    # Override Element.get_artifact_name()
-    def get_artifact_name(self, key=None):
-        return self._ref
-
-    # Dummy configure method
+    ########################################################
+    #         Implement Element abstract methods           #
+    ########################################################
     def configure(self, node):
         pass
 
-    # Dummy preflight method
     def preflight(self):
         pass
 
-    # get_dependency_artifact_names()
-    #
-    # Retrieve the artifact names of all of the dependencies in _Scope.BUILD
-    #
-    # Returns:
-    #   (list [str]): A list of artifact refs
-    #
-    def get_dependency_artifact_names(self):
-        artifact = self._get_artifact()
-        return artifact.get_dependency_artifact_names()
-
-    # configure_sandbox()
-    #
-    # Configure a sandbox for installing artifacts into
-    #
-    # Args:
-    #    sandbox (Sandbox)
-    #
     def configure_sandbox(self, sandbox):
         install_root = self.get_variable("install-root")
 
         # Tell the sandbox to mount the build root and install root
         sandbox.mark_directory(install_root)
 
-    # Override Element._calculate_cache_key
-    def _calculate_cache_key(self, dependencies=None):
-        return self._key
-
-    # Override Element._get_cache_key()
-    def _get_cache_key(self, strength=None):
-        return self._key
-
 
 # verify_artifact_ref()
 #
diff --git a/src/buildstream/_stream.py b/src/buildstream/_stream.py
index fc5b15211..a5391562a 100644
--- a/src/buildstream/_stream.py
+++ b/src/buildstream/_stream.py
@@ -31,6 +31,7 @@ from collections import deque
 from typing import List, Tuple
 
 from ._artifactelement import verify_artifact_ref, ArtifactElement
+from ._artifactproject import ArtifactProject
 from ._exceptions import StreamError, ImplError, BstError, ArtifactElementError, ArtifactError
 from ._message import Message, MessageType
 from ._scheduler import (
@@ -1098,12 +1099,22 @@ class Stream:
     #
     def _load_artifacts(self, artifact_names):
         with self._context.messenger.simple_task("Loading artifacts") as task:
-            artifacts = []
+
+            # Use a set here to avoid duplicates.
+            #
+            # ArtifactElement.new_from_artifact_name() will take care of ensuring
+            # uniqueness of multiple artifact names which refer to the same artifact
+            # (e.g., if both weak and strong names happen to be requested), here we
+            # still need to ensure we generate a list that does not contain duplicates.
+            #
+            artifacts = set()
             for artifact_name in artifact_names:
-                artifacts.append(ArtifactElement._new_from_artifact_name(artifact_name, self._context, task))
+                artifact = ArtifactElement.new_from_artifact_name(artifact_name, self._context, task)
+                artifacts.add(artifact)
 
-        ArtifactElement._clear_artifact_refs_cache()
-        return artifacts
+        ArtifactElement.clear_artifact_name_cache()
+        ArtifactProject.clear_project_cache()
+        return list(artifacts)
 
     # _load_elements_from_targets
     #
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index c453e3c88..945ca5cf7 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -220,11 +220,17 @@ class Element(Plugin):
     """
 
     def __init__(
-        self, context: "Context", project: "Project", load_element: "LoadElement", plugin_conf: Dict[str, Any]
+        self,
+        context: "Context",
+        project: "Project",
+        load_element: "LoadElement",
+        plugin_conf: Dict[str, Any],
+        *,
+        artifact: Artifact = None,
     ):
 
         self.__cache_key_dict = None  # Dict for cache key calculation
-        self.__cache_key = None  # Our cached cache key
+        self.__cache_key: Optional[str] = None  # Our cached cache key
 
         super().__init__(load_element.name, context, project, load_element.node, "element")
 
@@ -278,8 +284,8 @@ class Element(Plugin):
         self.__ready_for_runtime_and_cached = False  # Whether all runtime deps are cached, as well as the element
         self.__cached_remotely = None  # Whether the element is cached remotely
         self.__sources = ElementSources(context, project, self)  # The element sources
-        self.__weak_cache_key = None  # Our cached weak cache key
-        self.__strict_cache_key = None  # Our cached cache key for strict builds
+        self.__weak_cache_key: Optional[str] = None  # Our cached weak cache key
+        self.__strict_cache_key: Optional[str] = None  # Our cached cache key for strict builds
         self.__artifacts = context.artifactcache  # Artifact cache
         self.__sourcecache = context.sourcecache  # Source cache
         self.__assemble_scheduled = False  # Element is scheduled to be assembled
@@ -294,6 +300,8 @@ class Element(Plugin):
         self.__build_result = None  # The result of assembling this Element (success, description, detail)
         # Artifact class for direct artifact composite interaction
         self.__artifact = None  # type: Optional[Artifact]
+        self.__dynamic_public = None
+        self.__sandbox_config = None  # type: Optional[SandboxConfig]
 
         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()
@@ -307,47 +315,10 @@ class Element(Plugin):
 
         self.__resolved_initial_state = False  # Whether the initial state of the Element has been resolved
 
-        # Ensure we have loaded this class's defaults
-        self.__init_defaults(project, plugin_conf, load_element.kind, load_element.first_pass)
-
-        # Collect the composited variables and resolve them
-        variables = self.__extract_variables(project, load_element)
-        variables["element-name"] = self.name
-        self.__variables = Variables(variables)
-        if not load_element.first_pass:
-            self.__variables.check()
-
-        # Collect the composited environment now that we have variables
-        unexpanded_env = self.__extract_environment(project, load_element)
-        self.__variables.expand(unexpanded_env)
-        self.__environment = unexpanded_env.strip_node_info()
-
-        # Collect the environment nocache blacklist list
-        nocache = self.__extract_env_nocache(project, load_element)
-        self.__env_nocache = nocache
-
-        # Grab public domain data declared for this instance
-        self.__public = self.__extract_public(load_element)
-        self.__variables.expand(self.__public)
-        self.__dynamic_public = None
-
-        # Collect the composited element configuration and
-        # ask the element to configure itself.
-        self.__config = self.__extract_config(load_element)
-        self.__variables.expand(self.__config)
-
-        self._configure(self.__config)
-
-        # Extract remote execution URL
-        if load_element.first_pass:
-            self.__remote_execution_specs = None
+        if artifact:
+            self.__initialize_from_artifact(artifact)
         else:
-            self.__remote_execution_specs = project.remote_execution_specs
-
-        # Extract Sandbox config
-        sandbox_config = self.__extract_sandbox_config(project, load_element)
-        self.__variables.expand(sandbox_config)
-        self.__sandbox_config = SandboxConfig.new_from_node(sandbox_config, platform=context.platform)
+            self.__initialize_from_yaml(load_element, plugin_conf)
 
     def __lt__(self, other):
         return self.name < other.name
@@ -1447,6 +1418,15 @@ class Element(Plugin):
     #
     @contextmanager
     def _prepare_sandbox(self, scope, shell=False, integrate=True, usebuildtree=False):
+
+        # Assert first that we have a sandbox configuration
+        if not self.__sandbox_config:
+            raise ElementError(
+                "Error preparing sandbox for element: {}".format(self.name),
+                detail="This is most likely an artifact that is not yet cached, try building or pulling the artifact first",
+                reason="missing-sandbox-config",
+            )
+
         # bst shell and bst artifact checkout require a local sandbox.
         with self.__sandbox(None, config=self.__sandbox_config, allow_remote=False) as sandbox:
             sandbox._usebuildtree = usebuildtree
@@ -2822,6 +2802,74 @@ class Element(Plugin):
             ) as sandbox:
                 yield sandbox
 
+    # __initialize_from_yaml()
+    #
+    # Normal element initialization procedure.
+    #
+    def __initialize_from_yaml(self, load_element: "LoadElement", plugin_conf: Dict[str, Any]):
+
+        context = self._get_context()
+        project = self._get_project()
+
+        # Ensure we have loaded this class's defaults
+        self.__init_defaults(project, plugin_conf, load_element.kind, load_element.first_pass)
+
+        # Collect the composited variables and resolve them
+        variables = self.__extract_variables(project, load_element)
+        variables["element-name"] = self.name
+        self.__variables = Variables(variables)
+        if not load_element.first_pass:
+            self.__variables.check()
+
+        # Collect the composited environment now that we have variables
+        unexpanded_env = self.__extract_environment(project, load_element)
+        self.__variables.expand(unexpanded_env)
+        self.__environment = unexpanded_env.strip_node_info()
+
+        # Collect the environment nocache blacklist list
+        nocache = self.__extract_env_nocache(project, load_element)
+        self.__env_nocache = nocache
+
+        # Grab public domain data declared for this instance
+        self.__public = self.__extract_public(load_element)
+        self.__variables.expand(self.__public)
+
+        # Collect the composited element configuration and
+        # ask the element to configure itself.
+        self.__config = self.__extract_config(load_element)
+        self.__variables.expand(self.__config)
+
+        self._configure(self.__config)
+
+        # Extract remote execution URL
+        if load_element.first_pass:
+            self.__remote_execution_specs = None
+        else:
+            self.__remote_execution_specs = project.remote_execution_specs
+
+        # Extract Sandbox config
+        sandbox_config = self.__extract_sandbox_config(project, load_element)
+        self.__variables.expand(sandbox_config)
+        self.__sandbox_config = SandboxConfig.new_from_node(sandbox_config, platform=context.platform)
+
+    # __initialize_from_artifact()
+    #
+    # Initialize the element state from an Artifact object
+    #
+    def __initialize_from_artifact(self, artifact: Artifact):
+        self.__artifact = artifact
+
+        # Load bits which have been stored on the artifact
+        #
+        if artifact.cached():
+            self.__environment = artifact.load_environment()
+            self.__sandbox_config = artifact.load_sandbox_config()
+            self.__variables = artifact.load_variables()
+
+        self.__cache_key = artifact.strong_key
+        self.__strict_cache_key = artifact.strict_key
+        self.__weak_cache_key = artifact.weak_key
+
     @classmethod
     def __compose_default_splits(cls, project, defaults, first_pass):
 
diff --git a/tests/frontend/artifact_show.py b/tests/frontend/artifact_show.py
index ebea7cf33..392a9e2c6 100644
--- a/tests/frontend/artifact_show.py
+++ b/tests/frontend/artifact_show.py
@@ -110,7 +110,7 @@ def test_artifact_show_artifact_ref(cli, tmpdir, datafiles):
     [
         # List only artifact results in the test/project
         #
-        ("test/**", ["test/target/", "test/target/", "test/compose-all/", "test/import-bin", "test/import-dev"]),
+        ("test/**", ["test/target/", "test/compose-all/", "test/import-bin", "test/import-dev"]),
         # List only artifact results by their .bst element names
         #
         ("**.bst", ["import-bin.bst", "import-dev.bst", "compose-all.bst", "target.bst", "subdir/target.bst"]),
diff --git a/tests/frontend/buildcheckout.py b/tests/frontend/buildcheckout.py
index 709259397..6d1190667 100644
--- a/tests/frontend/buildcheckout.py
+++ b/tests/frontend/buildcheckout.py
@@ -469,8 +469,7 @@ def test_build_checkout_invalid_ref(datafiles, cli):
     non_existent_artifact = "test/checkout-deps/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
     checkout_args = ["artifact", "checkout", "--deps", "none", "--tar", checkout, non_existent_artifact]
     result = cli.run(project=project, args=checkout_args)
-
-    assert "Error while staging dependencies into a sandbox: 'No artifacts to stage'" in result.stderr
+    result.assert_main_error(ErrorDomain.STREAM, "missing-sandbox-config")
 
 
 @pytest.mark.datafiles(DATA_DIR)
diff --git a/tests/frontend/push.py b/tests/frontend/push.py
index 80cf07067..0b1d988f9 100644
--- a/tests/frontend/push.py
+++ b/tests/frontend/push.py
@@ -166,11 +166,13 @@ def test_push_artifact_glob(cli, tmpdir, datafiles):
         # Configure artifact share
         cli.configure({"artifacts": {"url": share.repo, "push": True}})
 
-        # Run bst artifact push with a wildcard.
-        # This matches two artifact refs (weak and strong cache keys).
+        # Run bst artifact push with a wildcard, there is only one artifact
+        # matching "test/target/*", even though it can be accessed both by it's
+        # strong and weak key.
+        #
         result = cli.run(project=project, args=["artifact", "push", "test/target/*"])
         result.assert_success()
-        assert len(result.get_pushed_elements()) == 2
+        assert len(result.get_pushed_elements()) == 1
 
 
 # Tests that: