Evaluate options before includesvalentindavid/options-order

Since option directives `(?)` expect lists, they cannot easily be
merged. So multiple includes with conflicting option directives
would only get one list if option processing is done after
includes.

9 files changed, 111 insertions, 34 deletions

diff --git a/src/buildstream/_frontend/widget.py b/src/buildstream/_frontend/widget.py
index 81ca2f7b5..eab701b47 100644
--- a/src/buildstream/_frontend/widget.py
+++ b/src/buildstream/_frontend/widget.py
@@ -473,11 +473,12 @@ class LogLine(Widget):
 
         # Project Options
         values = OrderedDict()
-        project.options.printable_variables(values)
-        if values:
-            text += self.content_profile.fmt("Project Options\n", bold=True)
-            text += self._format_values(values)
-            text += "\n"
+        if project.options:
+            project.options.printable_variables(values)
+            if values:
+                text += self.content_profile.fmt("Project Options\n", bold=True)
+                text += self._format_values(values)
+                text += "\n"
 
         # Plugins
         text += self._format_plugins(
diff --git a/src/buildstream/_includes.py b/src/buildstream/_includes.py
index 7f4863e52..f6234230a 100644
--- a/src/buildstream/_includes.py
+++ b/src/buildstream/_includes.py
@@ -27,9 +27,10 @@ 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)
+    #    first_pass (bool): Whether to use first pass options.
+    #    process_options (bool): Whether to process options.
+    def process(self, node, *, only_local=False, first_pass=False, process_options=True):
+        self._process(node, only_local=only_local, first_pass=first_pass, process_options=process_options)
 
     # _process()
     #
@@ -41,12 +42,20 @@ 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
-    #    process_project_options (bool): Whether to process options from current project
-    def _process(self, node, *, included=None, current_loader=None, only_local=False, process_project_options=True):
+    #    first_pass (bool): Whether to use first pass options.
+    #    process_options (bool): Whether to process options.
+    def _process(
+        self, node, *, included=None, current_loader=None, only_local=False, first_pass=False, process_options=True,
+    ):
         if current_loader is None:
             current_loader = self._loader
 
-        if process_project_options:
+        if not process_options:
+            pass
+        elif first_pass:
+            current_loader.project.first_pass_config.options.process_node(node)
+        else:
+            current_loader.project.ensure_fully_loaded()
             current_loader.project.options.process_node(node)
 
         self._process_node(
@@ -54,7 +63,8 @@ class Includes:
             included=included,
             only_local=only_local,
             current_loader=current_loader,
-            process_project_options=process_project_options,
+            first_pass=first_pass,
+            process_options=process_options,
         )
 
     # _process_node()
@@ -67,9 +77,10 @@ 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
-    #    process_project_options (bool): Whether to process options from current project
+    #    first_pass (bool): Whether to use first pass options.
+    #    process_options (bool): Whether to process options.
     def _process_node(
-        self, node, *, included=None, current_loader=None, only_local=False, process_project_options=True
+        self, node, *, included=None, current_loader=None, only_local=False, first_pass=False, process_options=True,
     ):
         if included is None:
             included = set()
@@ -123,7 +134,8 @@ class Includes:
                         included=included,
                         current_loader=sub_loader,
                         only_local=only_local,
-                        process_project_options=process_project_options or current_loader != sub_loader,
+                        first_pass=first_pass,
+                        process_options=process_options,
                     )
                 finally:
                     included.remove(file_path)
@@ -136,7 +148,8 @@ class Includes:
                 included=included,
                 current_loader=current_loader,
                 only_local=only_local,
-                process_project_options=process_project_options,
+                first_pass=first_pass,
+                process_options=process_options,
             )
 
     # _include_file()
@@ -172,9 +185,10 @@ 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
-    #    process_project_options (bool): Whether to process options from current project
+    #    first_pass (bool): Whether to use first pass options.
+    #    process_options (bool): Whether to process options.
     def _process_value(
-        self, value, *, included=None, current_loader=None, only_local=False, process_project_options=True
+        self, value, *, included=None, current_loader=None, only_local=False, first_pass=False, process_options=True,
     ):
         value_type = type(value)
 
@@ -184,7 +198,8 @@ class Includes:
                 included=included,
                 current_loader=current_loader,
                 only_local=only_local,
-                process_project_options=process_project_options,
+                first_pass=first_pass,
+                process_options=process_options,
             )
         elif value_type is SequenceNode:
             for v in value:
@@ -193,5 +208,6 @@ class Includes:
                     included=included,
                     current_loader=current_loader,
                     only_local=only_local,
-                    process_project_options=process_project_options,
+                    first_pass=first_pass,
+                    process_options=process_options,
                 )
diff --git a/src/buildstream/_loader/loader.py b/src/buildstream/_loader/loader.py
index b73f5b862..28a1cff96 100644
--- a/src/buildstream/_loader/loader.py
+++ b/src/buildstream/_loader/loader.py
@@ -69,7 +69,6 @@ class Loader:
         self._context = context
         self._options = project.options  # Project options (OptionPool)
         self._basedir = basedir  # Base project directory
-        self._first_pass_options = project.first_pass_config.options  # Project options (OptionPool)
         self._parent = parent  # The parent loader
         self._fetch_subprojects = fetch_subprojects
 
@@ -518,7 +517,7 @@ class Loader:
 
         kind = node.get_str(Symbol.KIND)
         if kind == "junction":
-            self._first_pass_options.process_node(node)
+            self.project.first_pass_config.options.process_node(node)
         else:
             self.project.ensure_fully_loaded()
 
diff --git a/src/buildstream/_project.py b/src/buildstream/_project.py
index 508afa68b..031a6876c 100644
--- a/src/buildstream/_project.py
+++ b/src/buildstream/_project.py
@@ -680,9 +680,6 @@ class Project:
             self.directory, self.get_path_from_node(pre_config_node.get_scalar("element-path"), check_is_dir=True)
         )
 
-        self.config.options = OptionPool(self.element_path)
-        self.first_pass_config.options = OptionPool(self.element_path)
-
         defaults = pre_config_node.get_mapping("defaults")
         defaults.validate_keys(["targets"])
         self._default_targets = defaults.get_str_list("targets")
@@ -694,12 +691,17 @@ class Project:
 
         self._project_includes = Includes(self.loader, copy_tree=False)
 
+        # We need to process includes without options to load options included in separate files.
+        project_conf_for_options = self._project_conf.clone()
+        self._project_includes.process(project_conf_for_options, only_local=True, process_options=False)
+        self._load_options(project_conf_for_options, self.first_pass_config, ignore_unknown=True)
+
         project_conf_first_pass = self._project_conf.clone()
-        self._project_includes.process(project_conf_first_pass, only_local=True, process_project_options=False)
+        self._project_includes.process(project_conf_first_pass, only_local=True, first_pass=True)
         config_no_include = self._default_config_node.clone()
         project_conf_first_pass._composite(config_no_include)
 
-        self._load_pass(config_no_include, self.first_pass_config, ignore_unknown=True)
+        self._load_pass(config_no_include, self.first_pass_config)
 
         # Use separate file for storing source references
         ref_storage_node = pre_config_node.get_scalar("ref-storage")
@@ -719,8 +721,13 @@ class Project:
     # Process the second pass of loading the project configuration.
     #
     def _load_second_pass(self):
+        # We need to process includes without options to load options included in separate files.
+        project_conf_for_options = self._project_conf.clone()
+        self._project_includes.process(project_conf_for_options, only_local=True, process_options=False)
+        self._load_options(project_conf_for_options, self.config)
+
         project_conf_second_pass = self._project_conf.clone()
-        self._project_includes.process(project_conf_second_pass, process_project_options=False)
+        self._project_includes.process(project_conf_second_pass)
         config = self._default_config_node.clone()
         project_conf_second_pass._composite(config)
 
@@ -818,19 +825,17 @@ class Project:
 
             self._shell_host_files.append(mount)
 
-    # _load_pass():
+    # _load_options():
     #
-    # Loads parts of the project configuration that are different
-    # for first and second pass configurations.
+    # Loads options for a given configuration pass
     #
     # Args:
     #    config (dict) - YaML node of the configuration file.
     #    output (ProjectConfig) - ProjectConfig to load configuration onto.
     #    ignore_unknown (bool) - Whether option loader shoud ignore unknown options.
     #
-    def _load_pass(self, config, output, *, ignore_unknown=False):
-
-        self._load_plugin_factories(config, output)
+    def _load_options(self, config, output, *, ignore_unknown=False):
+        output.options = OptionPool(self.element_path)
 
         # Load project options
         options_node = config.get_mapping("options", default={})
@@ -855,6 +860,19 @@ class Project:
         # or conditionally specifying the project name; will be ignored.
         output.options.process_node(config)
 
+    # _load_pass():
+    #
+    # Loads parts of the project configuration that are common
+    # for first and second pass configurations.
+    #
+    # Args:
+    #    config (dict) - YaML node of the configuration file.
+    #    output (ProjectConfig) - ProjectConfig to load configuration onto.
+    #
+    def _load_pass(self, config, output):
+
+        self._load_plugin_factories(config, output)
+
         # Element and Source  type configurations will be composited later onto
         # element/source types, so we delete it from here and run our final
         # assertion after.
@@ -886,6 +904,8 @@ class Project:
         # Export options into variables, if that was requested
         output.options.export_variables(output.base_variables)
 
+        overrides = self._context.get_overrides(self.name)
+
         # Override default_mirror if not set by command-line
         output.default_mirror = self._default_mirror or overrides.get_str("default-mirror", default=None)
 
diff --git a/tests/format/include.py b/tests/format/include.py
index 12b043c8e..766a403fc 100644
--- a/tests/format/include.py
+++ b/tests/format/include.py
@@ -220,6 +220,20 @@ def test_conditional_in_fragment(cli, datafiles):
 
 
 @pytest.mark.datafiles(DATA_DIR)
+def test_conditional_in_fragment_conflicts(cli, datafiles):
+    project = os.path.join(str(datafiles), "conditional-conflicts")
+
+    result = cli.run(
+        project=project,
+        args=["-o", "build_arch", "i586", "show", "--deps", "none", "--format", "%{vars}", "element.bst"],
+    )
+    result.assert_success()
+    loaded = _yaml.load_data(result.output)
+    assert loaded.get_str("enable-work-around") == "true"
+    assert loaded.get_str("size") == "4"
+
+
+@pytest.mark.datafiles(DATA_DIR)
 def test_inner(cli, datafiles):
     project = os.path.join(str(datafiles), "inner")
     result = cli.run(
diff --git a/tests/format/include/conditional-conflicts/element.bst b/tests/format/include/conditional-conflicts/element.bst
new file mode 100644
index 000000000..4d7f70266
--- /dev/null
+++ b/tests/format/include/conditional-conflicts/element.bst
@@ -0,0 +1 @@
+kind: manual
diff --git a/tests/format/include/conditional-conflicts/extra_conf.yml b/tests/format/include/conditional-conflicts/extra_conf.yml
new file mode 100644
index 000000000..dd58c9855
--- /dev/null
+++ b/tests/format/include/conditional-conflicts/extra_conf.yml
@@ -0,0 +1,6 @@
+variables:
+  (?):
+    - build_arch == "i586":
+        size: "4"
+    - build_arch == "x86_64":
+        size: "8"
diff --git a/tests/format/include/conditional-conflicts/project.conf b/tests/format/include/conditional-conflicts/project.conf
new file mode 100644
index 000000000..6978fa813
--- /dev/null
+++ b/tests/format/include/conditional-conflicts/project.conf
@@ -0,0 +1,15 @@
+name: test
+min-version: 2.0
+
+options:
+  build_arch:
+    type: arch
+    description: Architecture
+    variable: build_arch
+    values:
+      - i586
+      - x86_64
+
+(@):
+  - extra_conf.yml
+  - work_around.yml
diff --git a/tests/format/include/conditional-conflicts/work_around.yml b/tests/format/include/conditional-conflicts/work_around.yml
new file mode 100644
index 000000000..a527fe124
--- /dev/null
+++ b/tests/format/include/conditional-conflicts/work_around.yml
@@ -0,0 +1,5 @@
+variables:
+  enable-work-around: "false"
+  (?):
+    - build_arch == "i586":
+        enable-work-around: "true"