Merge branch 'tristan/backport-junction-includes' into 'bst-1'

Process options in includes files with the options of their junction

See merge request BuildStream/buildstream!1912

20 files changed, 227 insertions, 28 deletions

diff --git a/NEWS b/NEWS
index 7da79eb33..cb13e4846 100644
--- a/NEWS
+++ b/NEWS
@@ -1,4 +1,17 @@
 =================
+  (unreleased)
+=================
+
+  o Process options in included files in the context of the project they
+    were included from.
+
+    This is technically a breaking change, however it is highly unlikely
+    that this will break projects. In some cases projects were working around
+    the broken behavior by ensuring matching project option names in junctioned
+    projects, and in other cases simply avoiding including files which have
+    project option conditional statements.
+
+=================
 buildstream 1.4.2
 =================
 
diff --git a/buildstream/_includes.py b/buildstream/_includes.py
index e30003630..e6f32cae5 100644
--- a/buildstream/_includes.py
+++ b/buildstream/_includes.py
@@ -26,16 +26,56 @@ class Includes:
     #
     # Args:
     #    node (dict): A YAML node
+    #    only_local (bool): Whether to ignore junction files
+    #    process_project_options (bool): Whether to process options from current project
+    #
+    def process(self, node, *, only_local=False, process_project_options=True):
+        self._process(node, only_local=only_local, process_project_options=process_project_options)
+
+    # _process()
+    #
+    # Process recursively include directives in a YAML node. This
+    # method is a recursively called on loaded nodes from files.
+    #
+    # Args:
+    #    node (dict): A YAML node
     #    included (set): Fail for recursion if trying to load any files in this set
     #    current_loader (Loader): Use alternative loader (for junction files)
     #    only_local (bool): Whether to ignore junction files
-    def process(self, node, *,
-                included=set(),
-                current_loader=None,
-                only_local=False):
+    #    process_project_options (bool): Whether to process options from current project
+    #
+    def _process(self, node, *, included=set(), current_loader=None, only_local=False, process_project_options=True):
+
         if current_loader is None:
             current_loader = self._loader
 
+        if process_project_options:
+            current_loader.project.options.process_node(node)
+
+        self._process_node(
+            node,
+            included=included,
+            only_local=only_local,
+            current_loader=current_loader,
+            process_project_options=process_project_options,
+        )
+
+    # _process_node()
+    #
+    # Process recursively include directives in a YAML node. This
+    # method is recursively called on all nodes.
+    #
+    # Args:
+    #    node (dict): A YAML node
+    #    included (set): Fail for recursion if trying to load any files in this set
+    #    current_loader (Loader): Use alternative loader (for junction files)
+    #    only_local (bool): Whether to ignore junction files
+    #    process_project_options (bool): Whether to process options from current project
+    #
+    def _process_node(
+        self, node, *, included=set(), current_loader=None, only_local=False, process_project_options=True
+    ):
+
         if isinstance(node.get('(@)'), str):
             includes = [_yaml.node_get(node, str, '(@)')]
         else:
@@ -61,9 +101,12 @@ class Includes:
 
                 try:
                     included.add(file_path)
-                    self.process(include_node, included=included,
-                                 current_loader=sub_loader,
-                                 only_local=only_local)
+                    self._process(
+                        include_node, included=included,
+                        current_loader=sub_loader,
+                        only_local=only_local,
+                        process_project_options=process_project_options or current_loader != sub_loader,
+                    )
                 finally:
                     included.remove(file_path)
 
@@ -75,10 +118,13 @@ class Includes:
                     del node[key]
 
         for _, value in _yaml.node_items(node):
-            self._process_value(value,
-                                included=included,
-                                current_loader=current_loader,
-                                only_local=only_local)
+            self._process_value(
+                value,
+                included=included,
+                current_loader=current_loader,
+                only_local=only_local,
+                process_project_options=process_project_options,
+            )
 
     # _include_file()
     #
@@ -94,6 +140,7 @@ class Includes:
             junction, include = include.split(':', 1)
             junction_loader = loader._get_loader(junction, fetch_subprojects=True)
             current_loader = junction_loader
+            current_loader.project.ensure_fully_loaded()
         else:
             current_loader = loader
         project = current_loader.project
@@ -116,18 +163,25 @@ class Includes:
     #    included (set): Fail for recursion if trying to load any files in this set
     #    current_loader (Loader): Use alternative loader (for junction files)
     #    only_local (bool): Whether to ignore junction files
-    def _process_value(self, value, *,
-                       included=set(),
-                       current_loader=None,
-                       only_local=False):
+    #    process_project_options (bool): Whether to process options from current project
+    #
+    def _process_value(
+        self, value, *, included=set(), current_loader=None, only_local=False, process_project_options=True
+    ):
         if isinstance(value, Mapping):
-            self.process(value,
-                         included=included,
-                         current_loader=current_loader,
-                         only_local=only_local)
+            self._process_node(
+                value,
+                included=included,
+                current_loader=current_loader,
+                only_local=only_local,
+                process_project_options=process_project_options,
+            )
         elif isinstance(value, list):
             for v in value:
-                self._process_value(v,
-                                    included=included,
-                                    current_loader=current_loader,
-                                    only_local=only_local)
+                self._process_value(
+                    v,
+                    included=included,
+                    current_loader=current_loader,
+                    only_local=only_local,
+                    process_project_options=process_project_options,
+                )
diff --git a/buildstream/_loader/loader.py b/buildstream/_loader/loader.py
index ec929eadb..24f2b595b 100644
--- a/buildstream/_loader/loader.py
+++ b/buildstream/_loader/loader.py
@@ -267,8 +267,6 @@ class Loader():
 
             self._includes.process(node)
 
-            self._options.process_node(node)
-
         element = LoadElement(node, filename, self)
 
         self._elements[filename] = element
diff --git a/buildstream/_project.py b/buildstream/_project.py
index c5172e210..5530aa23d 100644
--- a/buildstream/_project.py
+++ b/buildstream/_project.py
@@ -436,7 +436,7 @@ class Project():
         self._project_includes = Includes(self.loader, copy_tree=False)
 
         project_conf_first_pass = _yaml.node_copy(self._project_conf)
-        self._project_includes.process(project_conf_first_pass, only_local=True)
+        self._project_includes.process(project_conf_first_pass, only_local=True, process_project_options=False)
         config_no_include = _yaml.node_copy(self._default_config_node)
         _yaml.composite(config_no_include, project_conf_first_pass)
 
@@ -460,7 +460,7 @@ class Project():
     #
     def _load_second_pass(self):
         project_conf_second_pass = _yaml.node_copy(self._project_conf)
-        self._project_includes.process(project_conf_second_pass)
+        self._project_includes.process(project_conf_second_pass, process_project_options=False)
         config = _yaml.node_copy(self._default_config_node)
         _yaml.composite(config, project_conf_second_pass)
 
diff --git a/tests/format/include.py b/tests/format/include.py
index 36e723ed0..cfbfb66e3 100644
--- a/tests/format/include.py
+++ b/tests/format/include.py
@@ -261,3 +261,61 @@ def test_include_project_file(cli, datafiles):
     result.assert_success()
     loaded = _yaml.load_data(result.output)
     assert loaded['included'] == 'True'
+
+
+@pytest.mark.datafiles(DATA_DIR)
+def test_option_from_junction(cli, tmpdir, datafiles):
+    project = os.path.join(str(datafiles), "junction_options")
+
+    generate_junction(
+        tmpdir,
+        os.path.join(project, "subproject"),
+        os.path.join(project, "junction.bst"),
+        store_ref=True,
+        options={"local_option": "set"},
+    )
+
+    result = cli.run(project=project, args=["show", "--deps", "none", "--format", "%{vars}", "element.bst"])
+    result.assert_success()
+    loaded = _yaml.load_data(result.output)
+    assert loaded["is-default"] == 'False'
+
+
+@pytest.mark.datafiles(DATA_DIR)
+def test_option_from_junction_element(cli, tmpdir, datafiles):
+    project = os.path.join(str(datafiles), "junction_options_element")
+
+    generate_junction(
+        tmpdir,
+        os.path.join(project, "subproject"),
+        os.path.join(project, "junction.bst"),
+        store_ref=True,
+        options={"local_option": "set"},
+    )
+
+    result = cli.run(project=project, args=["show", "--deps", "none", "--format", "%{vars}", "element.bst"])
+    result.assert_success()
+    loaded = _yaml.load_data(result.output)
+    assert loaded["is-default"] == 'False'
+
+
+@pytest.mark.datafiles(DATA_DIR)
+def test_option_from_deep_junction(cli, tmpdir, datafiles):
+    project = os.path.join(str(datafiles), "junction_options_deep")
+
+    generate_junction(
+        tmpdir,
+        os.path.join(project, "subproject-2"),
+        os.path.join(project, "subproject-1", "junction-2.bst"),
+        store_ref=True,
+        options={"local_option": "set"},
+    )
+
+    generate_junction(
+        tmpdir, os.path.join(project, "subproject-1"), os.path.join(project, "junction-1.bst"), store_ref=True,
+    )
+
+    result = cli.run(project=project, args=["show", "--deps", "none", "--format", "%{vars}", "element.bst"])
+    result.assert_success()
+    loaded = _yaml.load_data(result.output)
+    assert loaded["is-default"] == 'False'
diff --git a/tests/format/include/junction_options/element.bst b/tests/format/include/junction_options/element.bst
new file mode 100644
index 000000000..4d7f70266
--- /dev/null
+++ b/tests/format/include/junction_options/element.bst
@@ -0,0 +1 @@
+kind: manual
diff --git a/tests/format/include/junction_options/project.conf b/tests/format/include/junction_options/project.conf
new file mode 100644
index 000000000..4836c5f8b
--- /dev/null
+++ b/tests/format/include/junction_options/project.conf
@@ -0,0 +1,4 @@
+name: test
+
+(@):
+  - junction.bst:extra_conf.yml
diff --git a/tests/format/include/junction_options/subproject/extra_conf.yml b/tests/format/include/junction_options/subproject/extra_conf.yml
new file mode 100644
index 000000000..1edbeee36
--- /dev/null
+++ b/tests/format/include/junction_options/subproject/extra_conf.yml
@@ -0,0 +1,7 @@
+(?):
+- local_option == 'default':
+    variables:
+      is-default: 'True'
+- local_option == 'set':
+    variables:
+      is-default: 'False'
diff --git a/tests/format/include/junction_options/subproject/project.conf b/tests/format/include/junction_options/subproject/project.conf
new file mode 100644
index 000000000..33ab0c8af
--- /dev/null
+++ b/tests/format/include/junction_options/subproject/project.conf
@@ -0,0 +1,11 @@
+name: test-sub
+
+options:
+  local_option:
+    type: enum
+    description: Testing
+    variable: local_option
+    default: default
+    values:
+      - default
+      - set
diff --git a/tests/format/include/junction_options_deep/element.bst b/tests/format/include/junction_options_deep/element.bst
new file mode 100644
index 000000000..4d7f70266
--- /dev/null
+++ b/tests/format/include/junction_options_deep/element.bst
@@ -0,0 +1 @@
+kind: manual
diff --git a/tests/format/include/junction_options_deep/project.conf b/tests/format/include/junction_options_deep/project.conf
new file mode 100644
index 000000000..2525081ce
--- /dev/null
+++ b/tests/format/include/junction_options_deep/project.conf
@@ -0,0 +1,4 @@
+name: test
+
+(@):
+  - junction-1.bst:extra_conf.yml
diff --git a/tests/format/include/junction_options_deep/subproject-1/extra_conf.yml b/tests/format/include/junction_options_deep/subproject-1/extra_conf.yml
new file mode 100644
index 000000000..faa1a40f7
--- /dev/null
+++ b/tests/format/include/junction_options_deep/subproject-1/extra_conf.yml
@@ -0,0 +1,2 @@
+(@):
+  junction-2.bst:extra_conf.yml
diff --git a/tests/format/include/junction_options_deep/subproject-1/project.conf b/tests/format/include/junction_options_deep/subproject-1/project.conf
new file mode 100644
index 000000000..f0cd28202
--- /dev/null
+++ b/tests/format/include/junction_options_deep/subproject-1/project.conf
@@ -0,0 +1 @@
+name: test-sub-1
diff --git a/tests/format/include/junction_options_deep/subproject-2/extra_conf.yml b/tests/format/include/junction_options_deep/subproject-2/extra_conf.yml
new file mode 100644
index 000000000..1edbeee36
--- /dev/null
+++ b/tests/format/include/junction_options_deep/subproject-2/extra_conf.yml
@@ -0,0 +1,7 @@
+(?):
+- local_option == 'default':
+    variables:
+      is-default: 'True'
+- local_option == 'set':
+    variables:
+      is-default: 'False'
diff --git a/tests/format/include/junction_options_deep/subproject-2/project.conf b/tests/format/include/junction_options_deep/subproject-2/project.conf
new file mode 100644
index 000000000..d44ccd809
--- /dev/null
+++ b/tests/format/include/junction_options_deep/subproject-2/project.conf
@@ -0,0 +1,11 @@
+name: test-sub-2
+
+options:
+  local_option:
+    type: enum
+    description: Testing
+    variable: local_option
+    default: default
+    values:
+      - default
+      - set
diff --git a/tests/format/include/junction_options_element/element.bst b/tests/format/include/junction_options_element/element.bst
new file mode 100644
index 000000000..c815951b6
--- /dev/null
+++ b/tests/format/include/junction_options_element/element.bst
@@ -0,0 +1,4 @@
+kind: manual
+
+(@):
+  - junction.bst:extra_conf.yml
diff --git a/tests/format/include/junction_options_element/project.conf b/tests/format/include/junction_options_element/project.conf
new file mode 100644
index 000000000..b32753625
--- /dev/null
+++ b/tests/format/include/junction_options_element/project.conf
@@ -0,0 +1 @@
+name: test
diff --git a/tests/format/include/junction_options_element/subproject/extra_conf.yml b/tests/format/include/junction_options_element/subproject/extra_conf.yml
new file mode 100644
index 000000000..1edbeee36
--- /dev/null
+++ b/tests/format/include/junction_options_element/subproject/extra_conf.yml
@@ -0,0 +1,7 @@
+(?):
+- local_option == 'default':
+    variables:
+      is-default: 'True'
+- local_option == 'set':
+    variables:
+      is-default: 'False'
diff --git a/tests/format/include/junction_options_element/subproject/project.conf b/tests/format/include/junction_options_element/subproject/project.conf
new file mode 100644
index 000000000..33ab0c8af
--- /dev/null
+++ b/tests/format/include/junction_options_element/subproject/project.conf
@@ -0,0 +1,11 @@
+name: test-sub
+
+options:
+  local_option:
+    type: enum
+    description: Testing
+    variable: local_option
+    default: default
+    values:
+      - default
+      - set
diff --git a/tests/testutils/junction.py b/tests/testutils/junction.py
index efc429ef6..9ab63eb73 100644
--- a/tests/testutils/junction.py
+++ b/tests/testutils/junction.py
@@ -16,7 +16,7 @@ from buildstream import _yaml
 # Returns:
 #    (str): The ref
 #
-def generate_junction(tmpdir, subproject_path, junction_path, *, store_ref=True):
+def generate_junction(tmpdir, subproject_path, junction_path, *, store_ref=True, options={}):
     # Create a repo to hold the subproject and generate
     # a junction element for it
     #
@@ -31,6 +31,10 @@ def generate_junction(tmpdir, subproject_path, junction_path, *, store_ref=True)
             repo.source_config(ref=source_ref)
         ]
     }
+
+    if options:
+        element["config"] = {"options": options}
+
     _yaml.dump(element, junction_path)
 
     return ref