Assign priorities to config scopes

Up to now configuration behaved like a stack, where scopes could be only be pushed at the top. This commit allows to assign priorities to scopes, and ensures that scopes of lower priorities are always "below" scopes of higher priorities. When scopes have the same priority, what matters is the insertion order.
spack · alalazo · Dec 25, 2024 · Dec 28, 2024 · Dec 30, 2024 · Jan 6, 2025
commit 122f78644a7233ffab7361b735468fc36a77e185
diff --git a/lib/spack/spack/cmd/common/arguments.py b/lib/spack/spack/cmd/common/arguments.py
@@ -528,7 +528,6 @@ def __call__(self, parser, namespace, values, option_string):
         # the const from the constructor or a value from the CLI.
         # Note that this is only called if the argument is actually
         # specified on the command line.
-        spack.config.CONFIG.ensure_scope_ordering()
         spack.config.set(self.config_path, self.const, scope="command_line")
 
 

diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py
@@ -34,7 +34,7 @@
 import os
 import re
 import sys
-from typing import Any, Callable, Dict, Generator, List, Optional, Tuple, Union
+from typing import Any, Callable, Dict, Generator, List, Optional, Sequence, Tuple, Union
 
 from llnl.util import filesystem, lang, tty
 
@@ -63,6 +63,8 @@
 import spack.util.web as web_util
 from spack.util.cpus import cpus_available
 
+from .enums import ConfigScopePriority
+
 #: Dict from section names -> schema for that section
 SECTION_SCHEMAS: Dict[str, Any] = {
     "compilers": spack.schema.compilers.schema,
@@ -404,16 +406,17 @@ def _method(self, *args, **kwargs):
     return _method
 
 
-class Configuration:
-    """A full Spack configuration, from a hierarchy of config files.
+ScopeWithOptionalPriority = Union[ConfigScope, Tuple[int, ConfigScope]]
+ScopeWithPriority = Tuple[int, ConfigScope]
 
-    This class makes it easy to add a new scope on top of an existing one.
-    """
+
+class Configuration:
+    """A hierarchical configuration, merging a number of scopes at different priorities."""
 
     # convert to typing.OrderedDict when we drop 3.6, or OrderedDict when we reach 3.9
     scopes: lang.PriorityOrderedMapping[str, ConfigScope]
 
-    def __init__(self, *scopes: ConfigScope) -> None:
+    def __init__(self, *scopes: ScopeWithOptionalPriority) -> None:
         """Initialize a configuration with an initial list of scopes.
 
         Args:
@@ -422,31 +425,35 @@ def __init__(self, *scopes: ConfigScope) -> None:
 
         """
         self.scopes = lang.PriorityOrderedMapping()
-        for scope in scopes:
-            self.push_scope(scope)
+        for item in scopes:
+            if isinstance(item, tuple):
+                priority, scope = item
+            else:
+                priority = ConfigScopePriority.CONFIG_FILES
+                scope = item
+
+            self.push_scope(scope, priority=priority)
         self.format_updates: Dict[str, List[ConfigScope]] = collections.defaultdict(list)
 
     def ensure_unwrapped(self) -> "Configuration":
         """Ensure we unwrap this object from any dynamic wrapper (like Singleton)"""
         return self
 
     def highest(self) -> ConfigScope:
-        """Scope with highest precedence"""
+        """Scope with the highest precedence"""
         return next(self.scopes.reversed_values())  # type: ignore
 
-    @_config_mutator
-    def ensure_scope_ordering(self):
-        """Ensure that scope order matches documented precedent"""
-        # FIXME: We also need to consider that custom configurations and other orderings
-        # may not be preserved correctly
-        if "command_line" in self.scopes:
-            # TODO (when dropping python 3.6): self.scopes.move_to_end
-            self.scopes.add("command_line", value=self.remove_scope("command_line"))
-
     @_config_mutator
     def push_scope(self, scope: ConfigScope, priority: Optional[int] = None) -> None:
-        """Add a higher precedence scope to the Configuration."""
-        tty.debug(f"[CONFIGURATION: PUSH SCOPE]: {str(scope)}", level=2)
+        """Adds a scope to the Configuration, at a given priority.
+
+        If a priority is not given, it is assumed to be the current highest priority.
+
+        Args:
+            scope: scope to be added
+            priority: priority of the scope
+        """
+        tty.debug(f"[CONFIGURATION: PUSH SCOPE]: {str(scope)}, priority={priority}", level=2)
         self.scopes.add(scope.name, value=scope, priority=priority)
 
     @_config_mutator
@@ -747,7 +754,7 @@ def override(
     """
     if isinstance(path_or_scope, ConfigScope):
         overrides = path_or_scope
-        CONFIG.push_scope(path_or_scope)
+        CONFIG.push_scope(path_or_scope, priority=None)
     else:
         base_name = _OVERRIDES_BASE_NAME
         # Ensure the new override gets a unique scope name
@@ -761,7 +768,7 @@ def override(
                 break
 
         overrides = InternalConfigScope(scope_name)
-        CONFIG.push_scope(overrides)
+        CONFIG.push_scope(overrides, priority=None)
         CONFIG.set(path_or_scope, value, scope=scope_name)
 
     try:
@@ -771,13 +778,15 @@ def override(
         assert scope is overrides
 
 
-def _add_platform_scope(cfg: Configuration, name: str, path: str, writable: bool = True) -> None:
+def _add_platform_scope(
+    cfg: Configuration, name: str, path: str, priority: ConfigScopePriority, writable: bool = True
+) -> None:
     """Add a platform-specific subdirectory for the current platform."""
     platform = spack.platforms.host().name
     scope = DirectoryConfigScope(
         f"{name}/{platform}", os.path.join(path, platform), writable=writable
     )
-    cfg.push_scope(scope)
+    cfg.push_scope(scope, priority=priority)
 
 
 def config_paths_from_entry_points() -> List[Tuple[str, str]]:
@@ -816,7 +825,7 @@ def create() -> Configuration:
 
     # first do the builtin, hardcoded defaults
     builtin = InternalConfigScope("_builtin", CONFIG_DEFAULTS)
-    cfg.push_scope(builtin)
+    cfg.push_scope(builtin, priority=ConfigScopePriority.BUILTIN)
 
     # Builtin paths to configuration files in Spack
     configuration_paths = [
@@ -846,10 +855,9 @@ def create() -> Configuration:
 
     # add each scope and its platform-specific directory
     for name, path in configuration_paths:
-        cfg.push_scope(DirectoryConfigScope(name, path))
-
-        # Each scope can have per-platfom overrides in subdirectories
-        _add_platform_scope(cfg, name, path)
+        cfg.push_scope(DirectoryConfigScope(name, path), priority=ConfigScopePriority.CONFIG_FILES)
+        # Each scope can have per-platform overrides in subdirectories
+        _add_platform_scope(cfg, name, path, priority=ConfigScopePriority.CONFIG_FILES)
 
     return cfg
 
@@ -1410,7 +1418,7 @@ def ensure_latest_format_fn(section: str) -> Callable[[YamlConfigDict], bool]:
 
 @contextlib.contextmanager
 def use_configuration(
-    *scopes_or_paths: Union[ConfigScope, str]
+    *scopes_or_paths: Union[ScopeWithOptionalPriority, str]
 ) -> Generator[Configuration, None, None]:
     """Use the configuration scopes passed as arguments within the context manager.
 
@@ -1436,23 +1444,27 @@ def use_configuration(
         CONFIG = saved_config
 
 
+def _normalize_input(entry: Union[ScopeWithOptionalPriority, str]) -> ScopeWithPriority:
+    if isinstance(entry, tuple):
+        return entry
+
+    default_priority = ConfigScopePriority.CONFIG_FILES
+    if isinstance(entry, ConfigScope):
+        return default_priority, entry
+
+    # Otherwise we need to construct it
+    path = os.path.normpath(entry)
+    assert os.path.isdir(path), f'"{path}" must be a directory'
+    name = os.path.basename(path)
+    return default_priority, DirectoryConfigScope(name, path)
+
+
 @lang.memoized
-def _config_from(scopes_or_paths: List[Union[ConfigScope, str]]) -> Configuration:
-    scopes = []
-    for scope_or_path in scopes_or_paths:
-        # If we have a config scope we are already done
-        if isinstance(scope_or_path, ConfigScope):
-            scopes.append(scope_or_path)
-            continue
-
-        # Otherwise we need to construct it
-        path = os.path.normpath(scope_or_path)
-        assert os.path.isdir(path), f'"{path}" must be a directory'
-        name = os.path.basename(path)
-        scopes.append(DirectoryConfigScope(name, path))
-
-    configuration = Configuration(*scopes)
-    return configuration
+def _config_from(
+    scopes_or_paths: Sequence[Union[ScopeWithOptionalPriority, str]]
+) -> Configuration:
+    scopes_with_priority = [_normalize_input(x) for x in scopes_or_paths]
+    return Configuration(*scopes_with_priority)
 
 
 def raw_github_gitlab_url(url: str) -> str:

diff --git a/lib/spack/spack/enums.py b/lib/spack/spack/enums.py
@@ -12,3 +12,14 @@ class InstallRecordStatus(enum.Flag):
     DEPRECATED = enum.auto()
     MISSING = enum.auto()
     ANY = INSTALLED | DEPRECATED | MISSING
+
+
+class ConfigScopePriority(enum.IntEnum):
+    """Priorities of the different kind of config scopes used by Spack"""
+
+    BUILTIN = 0
+    CONFIG_FILES = 1
+    ENVIRONMENT = 2
+    CUSTOM = 3
+    COMMAND_LINE = 4
+    OVERRIDE = 5
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
@@ -51,6 +51,8 @@
 from spack.spec_list import SpecList
 from spack.util.path import substitute_path_variables
 
+from ..enums import ConfigScopePriority
+
 SpecPair = spack.concretize.SpecPair
 
 #: environment variable used to indicate the active environment
@@ -3041,14 +3043,12 @@ def env_config_scopes(self) -> List[spack.config.ConfigScope]:
     def prepare_config_scope(self) -> None:
         """Add the manifest's scopes to the global configuration search path."""
         for scope in self.env_config_scopes:
-            spack.config.CONFIG.push_scope(scope)
-        spack.config.CONFIG.ensure_scope_ordering()
+            spack.config.CONFIG.push_scope(scope, priority=ConfigScopePriority.ENVIRONMENT)
 
     def deactivate_config_scope(self) -> None:
         """Remove any of the manifest's scopes from the global config path."""
         for scope in self.env_config_scopes:
             spack.config.CONFIG.remove_scope(scope.name)
-        spack.config.CONFIG.ensure_scope_ordering()
 
     @contextlib.contextmanager
     def use_config(self):

diff --git a/lib/spack/spack/main.py b/lib/spack/spack/main.py
@@ -48,6 +48,8 @@
 import spack.util.environment
 import spack.util.lock
 
+from .enums import ConfigScopePriority
+
 #: names of profile statistics
 stat_names = pstats.Stats.sort_arg_dict_default
 
@@ -876,14 +878,19 @@ def add_command_line_scopes(
         scopes = ev.environment_path_scopes(name, path)
         if scopes is None:
             if os.path.isdir(path):  # directory with config files
-                cfg.push_scope(spack.config.DirectoryConfigScope(name, path, writable=False))
-                spack.config._add_platform_scope(cfg, name, path, writable=False)
+                cfg.push_scope(
+                    spack.config.DirectoryConfigScope(name, path, writable=False),
+                    priority=ConfigScopePriority.CUSTOM,
+                )
+                spack.config._add_platform_scope(
+                    cfg, name, path, priority=ConfigScopePriority.CUSTOM, writable=False
+                )
                 continue
             else:
                 raise spack.error.ConfigError(f"Invalid configuration scope: {path}")
 
         for scope in scopes:
-            cfg.push_scope(scope)
+            cfg.push_scope(scope, priority=ConfigScopePriority.CUSTOM)
 
 
 def _main(argv=None):
@@ -955,7 +962,9 @@ def _main(argv=None):
     # Push scopes from the command line last
     if args.config_scopes:
         add_command_line_scopes(spack.config.CONFIG, args.config_scopes)
-    spack.config.CONFIG.push_scope(spack.config.InternalConfigScope("command_line"))
+    spack.config.CONFIG.push_scope(
+        spack.config.InternalConfigScope("command_line"), priority=ConfigScopePriority.COMMAND_LINE
+    )
     setup_main_options(args)
 
     # ------------------------------------------------------------------------

diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py
@@ -33,6 +33,8 @@
 import spack.util.path as spack_path
 import spack.util.spack_yaml as syaml
 
+from ..enums import ConfigScopePriority
+
 # sample config data
 config_low = {
     "config": {
@@ -1445,7 +1447,7 @@ def test_config_path_dsl(path, it_should_work, expected_parsed):
 
 
 @pytest.mark.regression("48254")
-def test_env_activation_preserves_config_scopes(mutable_mock_env_path):
+def test_env_activation_preserves_command_line_scope(mutable_mock_env_path):
     """Check that the "command_line" scope remains the highest priority scope, when we activate,
     or deactivate, environments.
     """
@@ -1469,3 +1471,33 @@ def test_env_activation_preserves_config_scopes(mutable_mock_env_path):
         assert spack.config.CONFIG.highest() == expected_cl_scope
 
     assert spack.config.CONFIG.highest() == expected_cl_scope
+
+
+@pytest.mark.regression("48414")
+def test_env_activation_preserves_config_scopes(mutable_mock_env_path):
+    """Check that the priority of scopes is respected when merging configuration files."""
+    custom_scope = spack.config.InternalConfigScope("custom_scope")
+    spack.config.CONFIG.push_scope(custom_scope, priority=ConfigScopePriority.CUSTOM)
+    expected_scopes = ["custom_scope", "command_line"]
+
+    def highest_priority_scopes(config):
+        return list(config.scopes)[-2:]
+
+    assert highest_priority_scopes(spack.config.CONFIG) == expected_scopes
+    # Creating an environment pushes a new scope
+    ev.create("test")
+    with ev.read("test"):
+        assert highest_priority_scopes(spack.config.CONFIG) == expected_scopes
+
+        # No active environment pops the scope
+        with ev.no_active_environment():
+            assert highest_priority_scopes(spack.config.CONFIG) == expected_scopes
+        assert highest_priority_scopes(spack.config.CONFIG) == expected_scopes
+
+        # Switch the environment to another one
+        ev.create("test-2")
+        with ev.read("test-2"):
+            assert highest_priority_scopes(spack.config.CONFIG) == expected_scopes
+        assert highest_priority_scopes(spack.config.CONFIG) == expected_scopes
+
+    assert highest_priority_scopes(spack.config.CONFIG) == expected_scopes
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
@@ -64,6 +64,8 @@
 from spack.main import SpackCommand
 from spack.util.pattern import Bunch
 
+from ..enums import ConfigScopePriority
+
 mirror_cmd = SpackCommand("mirror")
 
 
@@ -722,11 +724,23 @@ def configuration_dir(tmpdir_factory, linux_os):
 def _create_mock_configuration_scopes(configuration_dir):
     """Create the configuration scopes used in `config` and `mutable_config`."""
     return [
-        spack.config.InternalConfigScope("_builtin", spack.config.CONFIG_DEFAULTS),
-        spack.config.DirectoryConfigScope("site", str(configuration_dir.join("site"))),
-        spack.config.DirectoryConfigScope("system", str(configuration_dir.join("system"))),
-        spack.config.DirectoryConfigScope("user", str(configuration_dir.join("user"))),
-        spack.config.InternalConfigScope("command_line"),
+        (
+            ConfigScopePriority.BUILTIN,
+            spack.config.InternalConfigScope("_builtin", spack.config.CONFIG_DEFAULTS),
+        ),
+        (
+            ConfigScopePriority.CONFIG_FILES,
+            spack.config.DirectoryConfigScope("site", str(configuration_dir.join("site"))),
+        ),
+        (
+            ConfigScopePriority.CONFIG_FILES,
+            spack.config.DirectoryConfigScope("system", str(configuration_dir.join("system"))),
+        ),
+        (
+            ConfigScopePriority.CONFIG_FILES,
+            spack.config.DirectoryConfigScope("user", str(configuration_dir.join("user"))),
+        ),
+        (ConfigScopePriority.COMMAND_LINE, spack.config.InternalConfigScope("command_line")),
     ]