From 3ad99d75f99fe926f434049f5d507bfe6a7f2331 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Tue, 25 Feb 2025 09:58:16 +0100 Subject: [PATCH] Assign priorities to configuration scopes (#48420) Currently, environments can end up with higher priority than `-C` custom config scopes and `-c` command line arguments sometimes. This shouldn't happen -- those explicit CLI scopes should override active environments. Up to now configuration behaved like a stack, where scopes could be only be pushed at the top. This PR 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. Modifications: - [x] Add a mapping that iterates over keys according to priorities set when adding the key/value pair - [x] Use that mapping to allow assigning priorities to configuration scopes - [x] Assign different priorities for different kind of scopes, to fix a bug, and add a regression test - [x] Simplify `Configuration` constructor - [x] Remove `Configuration.pop_scope` - [x] Remove `unify:false` from custom `-C` scope in pipelines On the last modification: on `develop`, pipelines are relying on the environment being able to override `-C` scopes, which is a bug. After this fix, we need to be explicit about the unification strategy in each stack, and remove the blanket `unify:false` from the highest priority scope Signed-off-by: Massimiliano Culpo --- lib/spack/docs/conf.py | 4 + lib/spack/llnl/util/lang.py | 87 +++++++++- lib/spack/spack/cmd/common/arguments.py | 1 - lib/spack/spack/config.py | 156 +++++++++--------- lib/spack/spack/enums.py | 10 ++ lib/spack/spack/environment/environment.py | 6 +- lib/spack/spack/main.py | 17 +- lib/spack/spack/test/build_environment.py | 10 +- lib/spack/spack/test/config.py | 39 ++++- lib/spack/spack/test/conftest.py | 32 ++-- lib/spack/spack/test/env.py | 13 +- lib/spack/spack/test/llnl/util/lang.py | 41 +++++ .../cloud_pipelines/configs/concretizer.yaml | 1 - .../aws-pcluster-neoverse_v1/spack.yaml | 3 + .../stacks/aws-pcluster-x86_64_v4/spack.yaml | 3 + .../bootstrap-aarch64-darwin/spack.yaml | 3 + .../bootstrap-x86_64-linux-gnu/spack.yaml | 2 + .../stacks/build_systems/spack.yaml | 2 + .../stacks/ml-darwin-aarch64-mps/spack.yaml | 2 + .../stacks/ml-linux-aarch64-cpu/spack.yaml | 2 + .../stacks/ml-linux-aarch64-cuda/spack.yaml | 2 + .../stacks/ml-linux-x86_64-cpu/spack.yaml | 2 + .../stacks/ml-linux-x86_64-cuda/spack.yaml | 2 + .../stacks/ml-linux-x86_64-rocm/spack.yaml | 2 + .../stacks/radiuss-aws-aarch64/spack.yaml | 2 + .../stacks/radiuss-aws/spack.yaml | 2 + .../cloud_pipelines/stacks/radiuss/spack.yaml | 2 + .../stacks/tutorial/spack.yaml | 2 + .../stacks/windows-vis/spack.yaml | 2 + 29 files changed, 343 insertions(+), 109 deletions(-) diff --git a/lib/spack/docs/conf.py b/lib/spack/docs/conf.py index 87e6f6b227b..e3f0df9a4ba 100644 --- a/lib/spack/docs/conf.py +++ b/lib/spack/docs/conf.py @@ -223,6 +223,10 @@ def setup(sphinx): ("py:class", "spack.compiler.CompilerCache"), # TypeVar that is not handled correctly ("py:class", "llnl.util.lang.T"), + ("py:class", "llnl.util.lang.KT"), + ("py:class", "llnl.util.lang.VT"), + ("py:obj", "llnl.util.lang.KT"), + ("py:obj", "llnl.util.lang.VT"), ] # The reST default role (used for this markup: `text`) to use for all documents. diff --git a/lib/spack/llnl/util/lang.py b/lib/spack/llnl/util/lang.py index 922d515c65c..3e21e882e36 100644 --- a/lib/spack/llnl/util/lang.py +++ b/lib/spack/llnl/util/lang.py @@ -14,7 +14,7 @@ import typing import warnings from datetime import datetime, timedelta -from typing import Callable, Dict, Iterable, List, Tuple, TypeVar +from typing import Callable, Dict, Iterable, List, Mapping, Optional, Tuple, TypeVar # Ignore emacs backups when listing modules ignore_modules = r"^\.#|~$" @@ -1080,3 +1080,88 @@ def __set__(self, instance, value): def factory(self, instance, owner): raise NotImplementedError("must be implemented by derived classes") + + +KT = TypeVar("KT") +VT = TypeVar("VT") + + +class PriorityOrderedMapping(Mapping[KT, VT]): + """Mapping that iterates over key according to an integer priority. If the priority is + the same for two keys, insertion order is what matters. + + The priority is set when the key/value pair is added. If not set, the highest current priority + is used. + """ + + _data: Dict[KT, VT] + _priorities: List[Tuple[int, KT]] + + def __init__(self) -> None: + self._data = {} + # Tuple of (priority, key) + self._priorities = [] + + def __getitem__(self, key: KT) -> VT: + return self._data[key] + + def __len__(self) -> int: + return len(self._data) + + def __iter__(self): + yield from (key for _, key in self._priorities) + + def __reversed__(self): + yield from (key for _, key in reversed(self._priorities)) + + def reversed_keys(self): + """Iterates over keys from the highest priority, to the lowest.""" + return reversed(self) + + def reversed_values(self): + """Iterates over values from the highest priority, to the lowest.""" + yield from (self._data[key] for _, key in reversed(self._priorities)) + + def _highest_priority(self) -> int: + if not self._priorities: + return 0 + result, _ = self._priorities[-1] + return result + + def add(self, key: KT, *, value: VT, priority: Optional[int] = None) -> None: + """Adds a key/value pair to the mapping, with a specific priority. + + If the priority is None, then it is assumed to be the highest priority value currently + in the container. + + Raises: + ValueError: when the same priority is already in the mapping + """ + if priority is None: + priority = self._highest_priority() + + if key in self._data: + self.remove(key) + + self._priorities.append((priority, key)) + # We rely on sort being stable + self._priorities.sort(key=lambda x: x[0]) + self._data[key] = value + assert len(self._data) == len(self._priorities) + + def remove(self, key: KT) -> VT: + """Removes a key from the mapping. + + Returns: + The value associated with the key being removed + + Raises: + KeyError: if the key is not in the mapping + """ + if key not in self._data: + raise KeyError(f"cannot find {key}") + + popped_item = self._data.pop(key) + self._priorities = [(p, k) for p, k in self._priorities if k != key] + assert len(self._data) == len(self._priorities) + return popped_item diff --git a/lib/spack/spack/cmd/common/arguments.py b/lib/spack/spack/cmd/common/arguments.py index 41a7024d285..bd6dfbbb467 100644 --- 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 index b23b7e4ae92..79e7bd997c7 100644 --- a/lib/spack/spack/config.py +++ b/lib/spack/spack/config.py @@ -66,6 +66,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, @@ -408,26 +410,18 @@ 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: Dict[str, ConfigScope] + scopes: lang.PriorityOrderedMapping[str, ConfigScope] - def __init__(self, *scopes: ConfigScope) -> None: - """Initialize a configuration with an initial list of scopes. - - Args: - scopes: list of scopes to add to this - Configuration, ordered from lowest to highest precedence - - """ - self.scopes = collections.OrderedDict() - for scope in scopes: - self.push_scope(scope) + def __init__(self) -> None: + self.scopes = lang.PriorityOrderedMapping() self.format_updates: Dict[str, List[ConfigScope]] = collections.defaultdict(list) def ensure_unwrapped(self) -> "Configuration": @@ -435,36 +429,31 @@ def ensure_unwrapped(self) -> "Configuration": return self def highest(self) -> ConfigScope: - """Scope with highest precedence""" - return next(reversed(self.scopes.values())) # type: ignore + """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["command_line"] = self.remove_scope("command_line") + def push_scope(self, scope: ConfigScope, priority: Optional[int] = None) -> None: + """Adds a scope to the Configuration, at a given priority. - @_config_mutator - def push_scope(self, scope: ConfigScope) -> None: - """Add a higher precedence scope to the Configuration.""" - tty.debug(f"[CONFIGURATION: PUSH SCOPE]: {str(scope)}", level=2) - self.scopes[scope.name] = scope + If a priority is not given, it is assumed to be the current highest priority. - @_config_mutator - def pop_scope(self) -> ConfigScope: - """Remove the highest precedence scope and return it.""" - name, scope = self.scopes.popitem(last=True) # type: ignore[call-arg] - tty.debug(f"[CONFIGURATION: POP SCOPE]: {str(scope)}", level=2) - return scope + 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 def remove_scope(self, scope_name: str) -> Optional[ConfigScope]: - """Remove scope by name; has no effect when ``scope_name`` does not exist""" - scope = self.scopes.pop(scope_name, None) - tty.debug(f"[CONFIGURATION: POP SCOPE]: {str(scope)}", level=2) + """Removes a scope by name, and returns it. If the scope does not exist, returns None.""" + try: + scope = self.scopes.remove(scope_name) + tty.debug(f"[CONFIGURATION: POP SCOPE]: {str(scope)}", level=2) + except KeyError as e: + tty.debug(f"[CONFIGURATION: POP SCOPE]: {e}", level=2) + return None return scope @property @@ -473,15 +462,13 @@ def writable_scopes(self) -> Generator[ConfigScope, None, None]: return (s for s in self.scopes.values() if s.writable) def highest_precedence_scope(self) -> ConfigScope: - """Writable scope with highest precedence.""" - return next(s for s in reversed(self.scopes.values()) if s.writable) # type: ignore + """Writable scope with the highest precedence.""" + return next(s for s in self.scopes.reversed_values() if s.writable) def highest_precedence_non_platform_scope(self) -> ConfigScope: - """Writable non-platform scope with highest precedence""" + """Writable non-platform scope with the highest precedence""" return next( - s - for s in reversed(self.scopes.values()) # type: ignore - if s.writable and not s.is_platform_dependent + s for s in self.scopes.reversed_values() if s.writable and not s.is_platform_dependent ) def matching_scopes(self, reg_expr) -> List[ConfigScope]: @@ -748,7 +735,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 @@ -762,7 +749,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: @@ -772,13 +759,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]]: @@ -813,11 +802,10 @@ def create() -> Configuration: it. It is bundled inside a function so that configuration can be initialized lazily. """ - cfg = Configuration() - # first do the builtin, hardcoded defaults - builtin = InternalConfigScope("_builtin", CONFIG_DEFAULTS) - cfg.push_scope(builtin) + cfg = create_from( + (ConfigScopePriority.BUILTIN, InternalConfigScope("_builtin", CONFIG_DEFAULTS)) + ) # Builtin paths to configuration files in Spack configuration_paths = [ @@ -847,10 +835,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 @@ -955,7 +942,7 @@ def set(path: str, value: Any, scope: Optional[str] = None) -> None: return CONFIG.set(path, value, scope) -def scopes() -> Dict[str, ConfigScope]: +def scopes() -> lang.PriorityOrderedMapping[str, ConfigScope]: """Convenience function to get list of configuration scopes.""" return CONFIG.scopes @@ -1409,7 +1396,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. @@ -1424,7 +1411,7 @@ def use_configuration( global CONFIG # Normalize input and construct a Configuration object - configuration = _config_from(scopes_or_paths) + configuration = create_from(*scopes_or_paths) CONFIG.clear_caches(), configuration.clear_caches() saved_config, CONFIG = CONFIG, configuration @@ -1435,23 +1422,44 @@ 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 +def create_from(*scopes_or_paths: Union[ScopeWithOptionalPriority, str]) -> Configuration: + """Creates a configuration object from the scopes passed in input. - # 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)) + Args: + *scopes_or_paths: either a tuple of (priority, ConfigScope), or a ConfigScope, or a string + If priority is not given, it is assumed to be ConfigScopePriority.CONFIG_FILES. If a + string is given, a DirectoryConfigScope is created from it. - configuration = Configuration(*scopes) - return configuration + Examples: + + >>> builtin_scope = InternalConfigScope("_builtin", {"config": {"build_jobs": 1}}) + >>> cl_scope = InternalConfigScope("command_line", {"config": {"build_jobs": 10}}) + >>> cfg = create_from( + ... (ConfigScopePriority.COMMAND_LINE, cl_scope), + ... (ConfigScopePriority.BUILTIN, builtin_scope) + ... ) + """ + scopes_with_priority = [_normalize_input(x) for x in scopes_or_paths] + result = Configuration() + for priority, scope in scopes_with_priority: + result.push_scope(scope, priority=priority) + return result def raw_github_gitlab_url(url: str) -> str: diff --git a/lib/spack/spack/enums.py b/lib/spack/spack/enums.py index 47a48b06c33..3a71d90fa88 100644 --- a/lib/spack/spack/enums.py +++ b/lib/spack/spack/enums.py @@ -12,3 +12,13 @@ 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 diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index 92bdc6871fe..bceae352371 100644 --- 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 @@ -3067,14 +3069,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 index 8907022009c..414d467d7a7 100644 --- a/lib/spack/spack/main.py +++ b/lib/spack/spack/main.py @@ -47,6 +47,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 @@ -872,14 +874,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): @@ -952,7 +959,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/build_environment.py b/lib/spack/spack/test/build_environment.py index 14a09e85dcf..86c6b5a120f 100644 --- a/lib/spack/spack/test/build_environment.py +++ b/lib/spack/spack/test/build_environment.py @@ -542,7 +542,7 @@ def test_build_jobs_sequential_is_sequential(): spack.config.determine_number_of_jobs( parallel=False, max_cpus=8, - config=spack.config.Configuration( + config=spack.config.create_from( spack.config.InternalConfigScope("command_line", {"config": {"build_jobs": 8}}), spack.config.InternalConfigScope("defaults", {"config": {"build_jobs": 8}}), ), @@ -556,7 +556,7 @@ def test_build_jobs_command_line_overrides(): spack.config.determine_number_of_jobs( parallel=True, max_cpus=1, - config=spack.config.Configuration( + config=spack.config.create_from( spack.config.InternalConfigScope("command_line", {"config": {"build_jobs": 10}}), spack.config.InternalConfigScope("defaults", {"config": {"build_jobs": 1}}), ), @@ -567,7 +567,7 @@ def test_build_jobs_command_line_overrides(): spack.config.determine_number_of_jobs( parallel=True, max_cpus=100, - config=spack.config.Configuration( + config=spack.config.create_from( spack.config.InternalConfigScope("command_line", {"config": {"build_jobs": 10}}), spack.config.InternalConfigScope("defaults", {"config": {"build_jobs": 100}}), ), @@ -581,7 +581,7 @@ def test_build_jobs_defaults(): spack.config.determine_number_of_jobs( parallel=True, max_cpus=10, - config=spack.config.Configuration( + config=spack.config.create_from( spack.config.InternalConfigScope("defaults", {"config": {"build_jobs": 1}}) ), ) @@ -591,7 +591,7 @@ def test_build_jobs_defaults(): spack.config.determine_number_of_jobs( parallel=True, max_cpus=10, - config=spack.config.Configuration( + config=spack.config.create_from( spack.config.InternalConfigScope("defaults", {"config": {"build_jobs": 100}}) ), ) diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py index 5682135ee26..a8d79e13272 100644 --- 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": { @@ -710,10 +712,7 @@ def assert_marked(obj): def test_internal_config_from_data(): - config = spack.config.Configuration() - - # add an internal config initialized from an inline dict - config.push_scope( + config = spack.config.create_from( spack.config.InternalConfigScope( "_builtin", {"config": {"verify_ssl": False, "build_jobs": 6}} ) @@ -1445,7 +1444,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 +1468,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 index 2193e84e70c..64ed92ae0b0 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -66,6 +66,8 @@ from spack.main import SpackCommand from spack.util.pattern import Bunch +from ..enums import ConfigScopePriority + mirror_cmd = SpackCommand("mirror") @@ -723,11 +725,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")), ] @@ -794,13 +808,11 @@ def mock_wsdk_externals(monkeypatch_session): def concretize_scope(mutable_config, tmpdir): """Adds a scope for concretization preferences""" tmpdir.ensure_dir("concretize") - mutable_config.push_scope( + with spack.config.override( spack.config.DirectoryConfigScope("concretize", str(tmpdir.join("concretize"))) - ) + ): + yield str(tmpdir.join("concretize")) - yield str(tmpdir.join("concretize")) - - mutable_config.pop_scope() spack.repo.PATH._provider_index = None diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py index 16c78bb5a9d..84c6046ae77 100644 --- a/lib/spack/spack/test/env.py +++ b/lib/spack/spack/test/env.py @@ -519,7 +519,9 @@ def test_error_message_when_using_too_new_lockfile(tmp_path): ("when_possible", True), ], ) -def test_environment_concretizer_scheme_used(tmp_path, unify_in_lower_scope, unify_in_spack_yaml): +def test_environment_concretizer_scheme_used( + tmp_path, mutable_config, unify_in_lower_scope, unify_in_spack_yaml +): """Tests that "unify" settings in spack.yaml always take precedence over settings in lower configuration scopes. """ @@ -533,10 +535,11 @@ def test_environment_concretizer_scheme_used(tmp_path, unify_in_lower_scope, uni unify: {str(unify_in_spack_yaml).lower()} """ ) - - with spack.config.override("concretizer:unify", unify_in_lower_scope): - with ev.Environment(manifest.parent) as e: - assert e.unify == unify_in_spack_yaml + mutable_config.set("concretizer:unify", unify_in_lower_scope) + assert mutable_config.get("concretizer:unify") == unify_in_lower_scope + with ev.Environment(manifest.parent) as e: + assert mutable_config.get("concretizer:unify") == unify_in_spack_yaml + assert e.unify == unify_in_spack_yaml @pytest.mark.parametrize("unify_in_config", [True, False, "when_possible"]) diff --git a/lib/spack/spack/test/llnl/util/lang.py b/lib/spack/spack/test/llnl/util/lang.py index 728119e31aa..dbdcb1dea84 100644 --- a/lib/spack/spack/test/llnl/util/lang.py +++ b/lib/spack/spack/test/llnl/util/lang.py @@ -364,3 +364,44 @@ def test_fnmatch_multiple(): assert not regex.match("libbar.so.1") assert not regex.match("libfoo.solibbar.so") assert not regex.match("libbaz.so") + + +class TestPriorityOrderedMapping: + @pytest.mark.parametrize( + "elements,expected", + [ + # Push out-of-order with explicit, and different, priorities + ([("b", 2), ("a", 1), ("d", 4), ("c", 3)], ["a", "b", "c", "d"]), + # Push in-order with priority=None + ([("a", None), ("b", None), ("c", None), ("d", None)], ["a", "b", "c", "d"]), + # Mix explicit and implicit priorities + ([("b", 2), ("c", None), ("a", 1), ("d", None)], ["a", "b", "c", "d"]), + ([("b", 10), ("c", None), ("a", -20), ("d", None)], ["a", "b", "c", "d"]), + ([("b", 10), ("c", None), ("a", 20), ("d", None)], ["b", "c", "a", "d"]), + # Adding the same key twice with different priorities + ([("b", 10), ("c", None), ("a", 20), ("d", None), ("a", -20)], ["a", "b", "c", "d"]), + # Adding the same key twice, no priorities + ([("b", None), ("a", None), ("b", None)], ["a", "b"]), + ], + ) + def test_iteration_order(self, elements, expected): + """Tests that the iteration order respects priorities, no matter the insertion order.""" + m = llnl.util.lang.PriorityOrderedMapping() + for key, priority in elements: + m.add(key, value=None, priority=priority) + assert list(m) == expected + + def test_reverse_iteration(self): + """Tests that we can conveniently use reverse iteration""" + m = llnl.util.lang.PriorityOrderedMapping() + for key, value in [("a", 1), ("b", 2), ("c", 3)]: + m.add(key, value=value) + + assert list(m) == ["a", "b", "c"] + assert list(reversed(m)) == ["c", "b", "a"] + + assert list(m.keys()) == ["a", "b", "c"] + assert list(m.reversed_keys()) == ["c", "b", "a"] + + assert list(m.values()) == [1, 2, 3] + assert list(m.reversed_values()) == [3, 2, 1] diff --git a/share/spack/gitlab/cloud_pipelines/configs/concretizer.yaml b/share/spack/gitlab/cloud_pipelines/configs/concretizer.yaml index 9006e2ec56a..25b643327a5 100644 --- a/share/spack/gitlab/cloud_pipelines/configs/concretizer.yaml +++ b/share/spack/gitlab/cloud_pipelines/configs/concretizer.yaml @@ -1,3 +1,2 @@ concretizer: reuse: false - unify: false diff --git a/share/spack/gitlab/cloud_pipelines/stacks/aws-pcluster-neoverse_v1/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/aws-pcluster-neoverse_v1/spack.yaml index e91665c398a..64435384f04 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/aws-pcluster-neoverse_v1/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/aws-pcluster-neoverse_v1/spack.yaml @@ -1,5 +1,8 @@ spack: view: false + concretizer: + unify: false + definitions: - apps: - gromacs diff --git a/share/spack/gitlab/cloud_pipelines/stacks/aws-pcluster-x86_64_v4/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/aws-pcluster-x86_64_v4/spack.yaml index fd2d4f2138c..f9fdd4969b0 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/aws-pcluster-x86_64_v4/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/aws-pcluster-x86_64_v4/spack.yaml @@ -1,5 +1,8 @@ spack: view: false + concretizer: + unify: false + definitions: - apps: - gromacs %oneapi diff --git a/share/spack/gitlab/cloud_pipelines/stacks/bootstrap-aarch64-darwin/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/bootstrap-aarch64-darwin/spack.yaml index 2c63d6fc4ce..7888fe3f7a3 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/bootstrap-aarch64-darwin/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/bootstrap-aarch64-darwin/spack.yaml @@ -1,6 +1,9 @@ spack: view: false + concretizer: + unify: false + packages: all: require: target=aarch64 diff --git a/share/spack/gitlab/cloud_pipelines/stacks/bootstrap-x86_64-linux-gnu/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/bootstrap-x86_64-linux-gnu/spack.yaml index 67d5b20fa32..4d8d316961c 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/bootstrap-x86_64-linux-gnu/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/bootstrap-x86_64-linux-gnu/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: diff --git a/share/spack/gitlab/cloud_pipelines/stacks/build_systems/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/build_systems/spack.yaml index 3d0419e7318..7f25097fbd1 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/build_systems/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/build_systems/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: require: target=x86_64_v3 diff --git a/share/spack/gitlab/cloud_pipelines/stacks/ml-darwin-aarch64-mps/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/ml-darwin-aarch64-mps/spack.yaml index 1ccedc4b553..eff7543a190 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/ml-darwin-aarch64-mps/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/ml-darwin-aarch64-mps/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: diff --git a/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-aarch64-cpu/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-aarch64-cpu/spack.yaml index 23ed6aa665e..c932c723404 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-aarch64-cpu/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-aarch64-cpu/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: require: diff --git a/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-aarch64-cuda/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-aarch64-cuda/spack.yaml index d37f8ec7d27..32070049a05 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-aarch64-cuda/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-aarch64-cuda/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: require: diff --git a/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-cpu/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-cpu/spack.yaml index 31ca52dd394..e3e11577867 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-cpu/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-cpu/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: require: diff --git a/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-cuda/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-cuda/spack.yaml index 7b508debd37..cf10cbba4d6 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-cuda/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-cuda/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: require: diff --git a/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-rocm/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-rocm/spack.yaml index f3bdf578fa1..7dc7437133a 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-rocm/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/ml-linux-x86_64-rocm/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: require: diff --git a/share/spack/gitlab/cloud_pipelines/stacks/radiuss-aws-aarch64/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/radiuss-aws-aarch64/spack.yaml index c5a1259ba62..dd8d74f5768 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/radiuss-aws-aarch64/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/radiuss-aws-aarch64/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: providers: diff --git a/share/spack/gitlab/cloud_pipelines/stacks/radiuss-aws/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/radiuss-aws/spack.yaml index ca7de563c44..e8ddea19869 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/radiuss-aws/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/radiuss-aws/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: providers: diff --git a/share/spack/gitlab/cloud_pipelines/stacks/radiuss/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/radiuss/spack.yaml index 3aea6fc48a4..6fcbbcc7980 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/radiuss/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/radiuss/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: require: target=x86_64_v3 diff --git a/share/spack/gitlab/cloud_pipelines/stacks/tutorial/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/tutorial/spack.yaml index 58695a07175..a58c60376e1 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/tutorial/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/tutorial/spack.yaml @@ -1,5 +1,7 @@ spack: view: false + concretizer: + unify: false packages: all: require: target=x86_64_v3 diff --git a/share/spack/gitlab/cloud_pipelines/stacks/windows-vis/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/windows-vis/spack.yaml index 1d2546b6980..38cd91cb9e7 100644 --- a/share/spack/gitlab/cloud_pipelines/stacks/windows-vis/spack.yaml +++ b/share/spack/gitlab/cloud_pipelines/stacks/windows-vis/spack.yaml @@ -5,6 +5,8 @@ spack: view: false + concretizer: + unify: false specs: - vtk~mpi