junctions: Replace coalescing with explicit overrides

This patch removes the functionality of coalescing junction
configurations on junction element names, and replaces it with
an explicit format which allows junction declarations to override
junctions in subprojects with junction declarations from the local
project.

Changes:

  * plugins/elements/junction.py: Load the overrides dictionary

  * exceptions.py: Added new CIRCULAR_REFERENCE to LoadErrorReason.

  * _loader/loadcontext.py: Add new register_loader() function which
    is called by the Loader to register all loaders with the context,
    delegating the task of detecting conflicting junctions to the
    load context.

  * _loader/loader.py: When loading a junction, check if there is
    an override from the parent project and use that instead.

    Register with the LoadContext, and pass the LoadContext
    and provenance along when creating a Project

    Further, we now protect against circular element path references
    with a new _loader_search_provenances table.

  * _project.py: Pass new `provenance` member through the constructor
    and onwards into the child loader.

5 files changed, 156 insertions, 44 deletions

diff --git a/src/buildstream/_loader/loadcontext.py b/src/buildstream/_loader/loadcontext.py
index 161be913b..65b7e9157 100644
--- a/src/buildstream/_loader/loadcontext.py
+++ b/src/buildstream/_loader/loadcontext.py
@@ -17,6 +17,9 @@
 #  Authors:
 #        Tristan Van Berkom <tristan.vanberkom@codethink.co.uk>
 
+from .._exceptions import LoadError
+from ..exceptions import LoadErrorReason
+
 
 # LoaderContext()
 #
@@ -34,6 +37,9 @@ class LoadContext:
         self.fetch_subprojects = None
         self.task = None
 
+        # A table of all Loaders, indexed by project name
+        self._loaders = {}
+
     # set_rewritable()
     #
     # Sets whether the projects are to be loaded in a rewritable fashion,
@@ -64,3 +70,45 @@ class LoadContext:
     #
     def set_fetch_subprojects(self, fetch_subprojects):
         self.fetch_subprojects = fetch_subprojects
+
+    # register_loader()
+    #
+    # Registers a new loader in the load context, possibly
+    # raising an error in the case of a conflict
+    #
+    # Args:
+    #    loader (Loader): The Loader object to register into context
+    #
+    # Raises:
+    #    (LoadError): A CONFLICTING_JUNCTION LoadError in the case of a conflict
+    #
+    def register_loader(self, loader):
+        project = loader.project
+        existing_loader = self._loaders.get(project.name, None)
+
+        if existing_loader:
+
+            assert project.junction is not None
+
+            if existing_loader.project.junction:
+                # The existing provenance can be None even if there is a junction, this
+                # can happen when specifying a full element path on the command line.
+                #
+                provenance_str = ""
+                if existing_loader.provenance:
+                    provenance_str = ": {}".format(existing_loader.provenance)
+
+                detail = "Project '{}' was already loaded by junction '{}'{}".format(
+                    project.name, existing_loader.project.junction._get_full_name(), provenance_str
+                )
+            else:
+                detail = "Project '{}' is also the toplevel project".format(project.name)
+
+            raise LoadError(
+                "{}: Error loading project '{}' from junction: {}".format(
+                    loader.provenance, project.name, project.junction._get_full_name()
+                ),
+                LoadErrorReason.CONFLICTING_JUNCTION,
+                detail=detail,
+            )
+        self._loaders[project.name] = loader
diff --git a/src/buildstream/_loader/loader.py b/src/buildstream/_loader/loader.py
index e63dd6c57..76d5eef82 100644
--- a/src/buildstream/_loader/loader.py
+++ b/src/buildstream/_loader/loader.py
@@ -46,9 +46,10 @@ from .._message import Message, MessageType
 # Args:
 #    project (Project): The toplevel Project object
 #    parent (Loader): A parent Loader object, in the case this is a junctioned Loader
+#    provenance (ProvenanceInformation): The provenance of the reference to this project's junction
 #
 class Loader:
-    def __init__(self, project, *, parent=None):
+    def __init__(self, project, *, parent=None, provenance=None):
 
         # Ensure we have an absolute path for the base directory
         basedir = project.element_path
@@ -60,6 +61,7 @@ class Loader:
         #
         self.load_context = project.load_context  # The LoadContext
         self.project = project  # The associated Project
+        self.provenance = provenance  # The provenance of whence this loader was instantiated
         self.loaded = None  # The number of loaded Elements
 
         #
@@ -73,9 +75,14 @@ class Loader:
         self._meta_elements = {}  # Dict of resolved meta elements by name
         self._elements = {}  # Dict of elements
         self._loaders = {}  # Dict of junction loaders
+        self._loader_search_provenances = {}  # Dictionary of provenances of ongoing child loader searches
 
         self._includes = Includes(self, copy_tree=True)
 
+        assert project.name is not None
+
+        self.load_context.register_loader(self)
+
     # load():
     #
     # Loads the project based on the parameters given to the constructor
@@ -163,12 +170,27 @@ class Loader:
     # Returns:
     #   (Loader): loader for sub-project
     #
-    def get_loader(self, name, provenance, *, level=0):
+    def get_loader(self, name, provenance):
         junction_path = name.split(":")
         loader = self
 
+        circular_provenance = self._loader_search_provenances.get(name, None)
+        if circular_provenance:
+            detail = None
+            if circular_provenance is not provenance:
+                detail = "Already searching for '{}' at: {}".format(name, circular_provenance)
+            raise LoadError(
+                "{}: Circular reference while searching for '{}'".format(provenance, name),
+                LoadErrorReason.CIRCULAR_REFERENCE,
+                detail=detail,
+            )
+
+        self._loader_search_provenances[name] = provenance
+
         for junction_name in junction_path:
-            loader = loader._get_loader(junction_name, provenance, level=level)
+            loader = loader._get_loader(junction_name, provenance)
+
+        del self._loader_search_provenances[name]
 
         return loader
 
@@ -514,6 +536,40 @@ class Loader:
 
         return self._meta_elements[top_element.name]
 
+    # _search_for_override():
+    #
+    # Search parent projects for an overridden subproject to replace this junction.
+    #
+    # Args:
+    #    filename (str): Junction name
+    #
+    def _search_for_override(self, filename):
+        loader = self
+        override_path = filename
+
+        # Collect any overrides to this junction in the ancestry
+        #
+        overriding_loaders = []
+        while loader._parent:
+            junction = loader.project.junction
+            override_filename, override_provenance = junction.overrides.get(override_path, (None, None))
+            if override_filename:
+                overriding_loaders.append((loader, override_filename, override_provenance))
+
+            override_path = junction.name + ":" + override_path
+            loader = loader._parent
+
+        # If there are any overriding loaders, use the highest one in
+        # the ancestry to lookup the loader for this project.
+        #
+        if overriding_loaders:
+            loader, filename, provenance = overriding_loaders[-1]
+            return loader._parent.get_loader(filename, provenance)
+
+        # No overrides were found in the ancestry
+        #
+        return None
+
     # _get_loader():
     #
     # Return loader for specified junction
@@ -526,7 +582,7 @@ class Loader:
     #
     # Returns: A Loader or None if specified junction does not exist
     #
-    def _get_loader(self, filename, provenance, *, level=0):
+    def _get_loader(self, filename, provenance):
         loader = None
         provenance_str = ""
         if provenance is not None:
@@ -534,43 +590,21 @@ class Loader:
 
         # return previously determined result
         if filename in self._loaders:
-            loader = self._loaders[filename]
-
-            if loader is None:
-                # do not allow junctions with the same name in different
-                # subprojects
-                raise LoadError(
-                    "{}Conflicting junction {} in subprojects, define junction in {}".format(
-                        provenance_str, filename, self.project.name
-                    ),
-                    LoadErrorReason.CONFLICTING_JUNCTION,
-                )
-
-            return loader
+            return self._loaders[filename]
 
-        if self._parent:
-            # junctions in the parent take precedence over junctions defined
-            # in subprojects
-            loader = self._parent._get_loader(filename, level=level + 1, provenance=provenance)
-            if loader:
-                self._loaders[filename] = loader
-                return loader
-
-        try:
-            self._load_file(filename, provenance=provenance)
-        except LoadError as e:
-            if e.reason != LoadErrorReason.MISSING_FILE:
-                # other load error
-                raise
-
-            if level == 0:
-                # junction element not found in this or ancestor projects
-                raise
+        #
+        # Search the ancestry for an overridden loader to use in place
+        # of using the locally defined junction.
+        #
+        override_loader = self._search_for_override(filename)
+        if override_loader:
+            self._loaders[filename] = override_loader
+            return override_loader
 
-            # mark junction as not available to allow detection of
-            # conflicting junctions in subprojects
-            self._loaders[filename] = None
-            return None
+        #
+        # Load the junction file
+        #
+        self._load_file(filename, provenance)
 
         # At this point we've loaded the LoadElement
         load_element = self._elements[filename]
@@ -668,7 +702,12 @@ class Loader:
             from .._project import Project  # pylint: disable=cyclic-import
 
             project = Project(
-                project_dir, self.load_context.context, junction=element, parent_loader=self, search_for_project=False,
+                project_dir,
+                self.load_context.context,
+                junction=element,
+                parent_loader=self,
+                search_for_project=False,
+                provenance=provenance,
             )
         except LoadError as e:
             if e.reason == LoadErrorReason.MISSING_PROJECT_CONF:
diff --git a/src/buildstream/_project.py b/src/buildstream/_project.py
index e0ddf3d41..fcfb31988 100644
--- a/src/buildstream/_project.py
+++ b/src/buildstream/_project.py
@@ -99,6 +99,7 @@ class Project:
         cli_options=None,
         default_mirror=None,
         parent_loader=None,
+        provenance=None,
         search_for_project=True,
     ):
 
@@ -164,7 +165,7 @@ class Project:
         self._project_includes = None
 
         with PROFILER.profile(Topics.LOAD_PROJECT, self.directory.replace(os.sep, "-")):
-            self._load(parent_loader=parent_loader)
+            self._load(parent_loader=parent_loader, provenance=provenance)
 
         self._partially_loaded = True
 
@@ -653,7 +654,7 @@ class Project:
     #
     # Raises: LoadError if there was a problem with the project.conf
     #
-    def _load(self, *, parent_loader=None):
+    def _load(self, *, parent_loader=None, provenance=None):
 
         # Load builtin default
         projectfile = os.path.join(self.directory, _PROJECT_CONF_FILE)
@@ -700,7 +701,7 @@ class Project:
         # Fatal warnings
         self._fatal_warnings = pre_config_node.get_str_list("fatal-warnings", default=[])
 
-        self.loader = Loader(self, parent=parent_loader)
+        self.loader = Loader(self, parent=parent_loader, provenance=provenance)
 
         self._project_includes = Includes(self.loader, copy_tree=False)
 
diff --git a/src/buildstream/exceptions.py b/src/buildstream/exceptions.py
index 2c455be84..e77d64fe7 100644
--- a/src/buildstream/exceptions.py
+++ b/src/buildstream/exceptions.py
@@ -142,3 +142,6 @@ class LoadErrorReason(Enum):
 
     LINK_FORBIDDEN_DEPENDENCIES = 25
     """A link element declared dependencies"""
+
+    CIRCULAR_REFERENCE = 26
+    """A circular element reference was detected"""
diff --git a/src/buildstream/plugins/elements/junction.py b/src/buildstream/plugins/elements/junction.py
index 3e221cce7..3396a04ab 100644
--- a/src/buildstream/plugins/elements/junction.py
+++ b/src/buildstream/plugins/elements/junction.py
@@ -161,13 +161,34 @@ class JunctionElement(Element):
 
     def configure(self, node):
 
-        node.validate_keys(["path", "options", "cache-junction-elements", "ignore-junction-remotes"])
+        node.validate_keys(["path", "options", "cache-junction-elements", "ignore-junction-remotes", "overrides"])
 
         self.path = node.get_str("path", default="")
         self.options = node.get_mapping("options", default={})
         self.cache_junction_elements = node.get_bool("cache-junction-elements", default=False)
         self.ignore_junction_remotes = node.get_bool("ignore-junction-remotes", default=False)
 
+        # The overrides dictionary has the target junction
+        # to override as a key, and a tuple consisting
+        # of the local overriding junction and the provenance
+        # of the override declaration.
+        self.overrides = {}
+        overrides_node = node.get_mapping("overrides", {})
+        for key, value in overrides_node.items():
+            junction_name = value.as_str()
+            provenance = value.get_provenance()
+
+            # Cannot override a subproject with the project itself
+            #
+            if junction_name == self.name:
+                raise ElementError(
+                    "{}: Attempt to override subproject junction '{}' with the overriding junction '{}' itself".format(
+                        provenance, key, junction_name
+                    ),
+                    reason="override-junction-with-self",
+                )
+            self.overrides[key] = (junction_name, provenance)
+
     def preflight(self):
         pass