From 1d8bdcfc046fa4d7b920ce9e72bd330a404818a6 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Fri, 5 Jul 2024 12:41:13 +0200 Subject: config: fix class hierarchy (#45044) 1. Avoid that `self.path` is of type `Optional[str]` 2. Simplify immutable config with a property. --- lib/spack/spack/bootstrap/config.py | 6 +- lib/spack/spack/ci.py | 3 +- lib/spack/spack/cmd/bootstrap.py | 2 +- lib/spack/spack/cmd/config.py | 19 ++-- lib/spack/spack/compilers/__init__.py | 2 +- lib/spack/spack/config.py | 155 ++++++++++++++--------------- lib/spack/spack/environment/environment.py | 23 +++-- lib/spack/spack/test/bindist.py | 4 +- lib/spack/spack/test/config.py | 18 ++-- lib/spack/spack/test/conftest.py | 20 ++-- 10 files changed, 128 insertions(+), 124 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/bootstrap/config.py b/lib/spack/spack/bootstrap/config.py index 8cba750fc5..067e884b50 100644 --- a/lib/spack/spack/bootstrap/config.py +++ b/lib/spack/spack/bootstrap/config.py @@ -129,10 +129,10 @@ def _bootstrap_config_scopes() -> Sequence["spack.config.ConfigScope"]: configuration_paths = (spack.config.CONFIGURATION_DEFAULTS_PATH, ("bootstrap", _config_path())) for name, path in configuration_paths: platform = spack.platforms.host().name - platform_scope = spack.config.ConfigScope( - "/".join([name, platform]), os.path.join(path, platform) + platform_scope = spack.config.DirectoryConfigScope( + f"{name}/{platform}", os.path.join(path, platform) ) - generic_scope = spack.config.ConfigScope(name, path) + generic_scope = spack.config.DirectoryConfigScope(name, path) config_scopes.extend([generic_scope, platform_scope]) msg = "[BOOTSTRAP CONFIG SCOPE] name={0}, path={1}" tty.debug(msg.format(generic_scope.name, generic_scope.path)) diff --git a/lib/spack/spack/ci.py b/lib/spack/spack/ci.py index 956b19abdc..528fa45063 100644 --- a/lib/spack/spack/ci.py +++ b/lib/spack/spack/ci.py @@ -809,7 +809,8 @@ def generate_gitlab_ci_yaml( cli_scopes = [ os.path.relpath(s.path, concrete_env_dir) for s in cfg.scopes().values() - if isinstance(s, cfg.ImmutableConfigScope) + if not s.writable + and isinstance(s, (cfg.DirectoryConfigScope)) and s.path not in env_includes and os.path.exists(s.path) ] diff --git a/lib/spack/spack/cmd/bootstrap.py b/lib/spack/spack/cmd/bootstrap.py index 5221a980c7..c321b12130 100644 --- a/lib/spack/spack/cmd/bootstrap.py +++ b/lib/spack/spack/cmd/bootstrap.py @@ -165,7 +165,7 @@ def _reset(args): if not ok_to_continue: raise RuntimeError("Aborting") - for scope in spack.config.CONFIG.file_scopes: + for scope in spack.config.CONFIG.writable_scopes: # The default scope should stay untouched if scope.name == "defaults": continue diff --git a/lib/spack/spack/cmd/config.py b/lib/spack/spack/cmd/config.py index 61b27bcdfe..36e235351b 100644 --- a/lib/spack/spack/cmd/config.py +++ b/lib/spack/spack/cmd/config.py @@ -264,7 +264,9 @@ def config_remove(args): def _can_update_config_file(scope: spack.config.ConfigScope, cfg_file): if isinstance(scope, spack.config.SingleFileScope): return fs.can_access(cfg_file) - return fs.can_write_to_dir(scope.path) and fs.can_access(cfg_file) + elif isinstance(scope, spack.config.DirectoryConfigScope): + return fs.can_write_to_dir(scope.path) and fs.can_access(cfg_file) + return False def _config_change_requires_scope(path, spec, scope, match_spec=None): @@ -362,14 +364,11 @@ def config_change(args): def config_update(args): # Read the configuration files spack.config.CONFIG.get_config(args.section, scope=args.scope) - updates: List[spack.config.ConfigScope] = list( - filter( - lambda s: not isinstance( - s, (spack.config.InternalConfigScope, spack.config.ImmutableConfigScope) - ), - spack.config.CONFIG.format_updates[args.section], - ) - ) + updates: List[spack.config.ConfigScope] = [ + x + for x in spack.config.CONFIG.format_updates[args.section] + if not isinstance(x, spack.config.InternalConfigScope) and x.writable + ] cannot_overwrite, skip_system_scope = [], False for scope in updates: @@ -447,7 +446,7 @@ def _can_revert_update(scope_dir, cfg_file, bkp_file): def config_revert(args): - scopes = [args.scope] if args.scope else [x.name for x in spack.config.CONFIG.file_scopes] + scopes = [args.scope] if args.scope else [x.name for x in spack.config.CONFIG.writable_scopes] # Search for backup files in the configuration scopes Entry = collections.namedtuple("Entry", ["scope", "cfg", "bkp"]) diff --git a/lib/spack/spack/compilers/__init__.py b/lib/spack/spack/compilers/__init__.py index 88e5e7b998..3105616425 100644 --- a/lib/spack/spack/compilers/__init__.py +++ b/lib/spack/spack/compilers/__init__.py @@ -260,7 +260,7 @@ def _init_compiler_config( def compiler_config_files(): config_files = list() config = spack.config.CONFIG - for scope in config.file_scopes: + for scope in config.writable_scopes: name = scope.name compiler_config = config.get("compilers", scope=name) if compiler_config: diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py index 2a2f180f45..54038ade93 100644 --- a/lib/spack/spack/config.py +++ b/lib/spack/spack/config.py @@ -35,7 +35,7 @@ import functools import os import re import sys -from typing import Any, Callable, Dict, Generator, List, Optional, Tuple, Type, Union +from typing import Any, Callable, Dict, Generator, List, Optional, Tuple, Union from llnl.util import filesystem, lang, tty @@ -117,21 +117,39 @@ YamlConfigDict = Dict[str, Any] class ConfigScope: - """This class represents a configuration scope. + def __init__(self, name: str) -> None: + self.name = name + self.writable = False + self.sections = syaml.syaml_dict() - A scope is one directory containing named configuration files. - Each file is a config "section" (e.g., mirrors, compilers, etc.). - """ + def get_section_filename(self, section: str) -> str: + raise NotImplementedError - def __init__(self, name, path) -> None: - self.name = name # scope name. - self.path = path # path to directory containing configs. - self.sections = syaml.syaml_dict() # sections read from config files. + def get_section(self, section: str) -> Optional[YamlConfigDict]: + raise NotImplementedError + + def _write_section(self, section: str) -> None: + raise NotImplementedError @property def is_platform_dependent(self) -> bool: - """Returns true if the scope name is platform specific""" - return os.sep in self.name + return False + + def clear(self) -> None: + """Empty cached config information.""" + self.sections = syaml.syaml_dict() + + def __repr__(self) -> str: + return f"" + + +class DirectoryConfigScope(ConfigScope): + """Config scope backed by a directory containing one file per section.""" + + def __init__(self, name: str, path: str, *, writable: bool = True) -> None: + super().__init__(name) + self.path = path + self.writable = writable def get_section_filename(self, section: str) -> str: """Returns the filename associated with a given section""" @@ -148,6 +166,9 @@ class ConfigScope: return self.sections[section] def _write_section(self, section: str) -> None: + if not self.writable: + raise ConfigError(f"Cannot write to immutable scope {self}") + filename = self.get_section_filename(section) data = self.get_section(section) if data is None: @@ -164,19 +185,23 @@ class ConfigScope: except (syaml.SpackYAMLError, OSError) as e: raise ConfigFileError(f"cannot write to '{filename}'") from e - def clear(self) -> None: - """Empty cached config information.""" - self.sections = syaml.syaml_dict() - - def __repr__(self) -> str: - return f"" + @property + def is_platform_dependent(self) -> bool: + """Returns true if the scope name is platform specific""" + return "/" in self.name class SingleFileScope(ConfigScope): """This class represents a configuration scope in a single YAML file.""" def __init__( - self, name: str, path: str, schema: YamlConfigDict, yaml_path: Optional[List[str]] = None + self, + name: str, + path: str, + schema: YamlConfigDict, + *, + yaml_path: Optional[List[str]] = None, + writable: bool = True, ) -> None: """Similar to ``ConfigScope`` but can be embedded in another schema. @@ -195,15 +220,13 @@ class SingleFileScope(ConfigScope): config: install_tree: $spack/opt/spack """ - super().__init__(name, path) + super().__init__(name) self._raw_data: Optional[YamlConfigDict] = None self.schema = schema + self.path = path + self.writable = writable self.yaml_path = yaml_path or [] - @property - def is_platform_dependent(self) -> bool: - return False - def get_section_filename(self, section) -> str: return self.path @@ -257,6 +280,8 @@ class SingleFileScope(ConfigScope): return self.sections.get(section, None) def _write_section(self, section: str) -> None: + if not self.writable: + raise ConfigError(f"Cannot write to immutable scope {self}") data_to_write: Optional[YamlConfigDict] = self._raw_data # If there is no existing data, this section SingleFileScope has never @@ -301,19 +326,6 @@ class SingleFileScope(ConfigScope): return f"" -class ImmutableConfigScope(ConfigScope): - """A configuration scope that cannot be written to. - - This is used for ConfigScopes passed on the command line. - """ - - def _write_section(self, section) -> None: - raise ConfigError(f"Cannot write to immutable scope {self}") - - def __repr__(self) -> str: - return f"" - - class InternalConfigScope(ConfigScope): """An internal configuration scope that is not persisted to a file. @@ -323,7 +335,7 @@ class InternalConfigScope(ConfigScope): """ def __init__(self, name: str, data: Optional[YamlConfigDict] = None) -> None: - super().__init__(name, None) + super().__init__(name) self.sections = syaml.syaml_dict() if data is not None: @@ -333,9 +345,6 @@ class InternalConfigScope(ConfigScope): validate({section: dsec}, SECTION_SCHEMAS[section]) self.sections[section] = _mark_internal(syaml.syaml_dict({section: dsec}), name) - def get_section_filename(self, section: str) -> str: - raise NotImplementedError("Cannot get filename for InternalConfigScope.") - def get_section(self, section: str) -> Optional[YamlConfigDict]: """Just reads from an internal dictionary.""" if section not in self.sections: @@ -440,27 +449,21 @@ class Configuration: return scope @property - def file_scopes(self) -> List[ConfigScope]: - """List of writable scopes with an associated file.""" - return [ - s - for s in self.scopes.values() - if (type(s) is ConfigScope or type(s) is SingleFileScope) - ] + def writable_scopes(self) -> Generator[ConfigScope, None, None]: + """Generator of writable scopes with an associated file.""" + return (s for s in self.scopes.values() if s.writable) def highest_precedence_scope(self) -> ConfigScope: - """Non-internal scope with highest precedence.""" - return next(reversed(self.file_scopes)) + """Writable scope with highest precedence.""" + return next(s for s in reversed(self.scopes.values()) if s.writable) # type: ignore def highest_precedence_non_platform_scope(self) -> ConfigScope: - """Non-internal non-platform scope with highest precedence - - Platform-specific scopes are of the form scope/platform""" - generator = reversed(self.file_scopes) - highest = next(generator) - while highest and highest.is_platform_dependent: - highest = next(generator) - return highest + """Writable non-platform scope with highest precedence""" + return next( + s + for s in reversed(self.scopes.values()) # type: ignore + if s.writable and not s.is_platform_dependent + ) def matching_scopes(self, reg_expr) -> List[ConfigScope]: """ @@ -755,13 +758,14 @@ COMMAND_LINE_SCOPES: List[str] = [] def _add_platform_scope( - cfg: Union[Configuration, lang.Singleton], scope_type: Type[ConfigScope], name: str, path: str + cfg: Union[Configuration, lang.Singleton], name: str, path: str, writable: bool = True ) -> None: """Add a platform-specific subdirectory for the current platform.""" platform = spack.platforms.host().name - plat_name = os.path.join(name, platform) - plat_path = os.path.join(path, platform) - cfg.push_scope(scope_type(plat_name, plat_path)) + scope = DirectoryConfigScope( + f"{name}/{platform}", os.path.join(path, platform), writable=writable + ) + cfg.push_scope(scope) def config_paths_from_entry_points() -> List[Tuple[str, str]]: @@ -806,8 +810,8 @@ def _add_command_line_scopes( # name based on order on the command line name = f"cmd_scope_{i:d}" - cfg.push_scope(ImmutableConfigScope(name, path)) - _add_platform_scope(cfg, ImmutableConfigScope, name, path) + cfg.push_scope(DirectoryConfigScope(name, path, writable=False)) + _add_platform_scope(cfg, name, path, writable=False) def create() -> Configuration: @@ -851,10 +855,10 @@ def create() -> Configuration: # add each scope and its platform-specific directory for name, path in configuration_paths: - cfg.push_scope(ConfigScope(name, path)) + cfg.push_scope(DirectoryConfigScope(name, path)) # Each scope can have per-platfom overrides in subdirectories - _add_platform_scope(cfg, ConfigScope, name, path) + _add_platform_scope(cfg, name, path) # add command-line scopes _add_command_line_scopes(cfg, COMMAND_LINE_SCOPES) @@ -969,7 +973,7 @@ def set(path: str, value: Any, scope: Optional[str] = None) -> None: def add_default_platform_scope(platform: str) -> None: plat_name = os.path.join("defaults", platform) plat_path = os.path.join(CONFIGURATION_DEFAULTS_PATH[1], platform) - CONFIG.push_scope(ConfigScope(plat_name, plat_path)) + CONFIG.push_scope(DirectoryConfigScope(plat_name, plat_path)) def scopes() -> Dict[str, ConfigScope]: @@ -978,19 +982,10 @@ def scopes() -> Dict[str, ConfigScope]: def writable_scopes() -> List[ConfigScope]: - """ - Return list of writable scopes. Higher-priority scopes come first in the - list. - """ - return list( - reversed( - list( - x - for x in CONFIG.scopes.values() - if not isinstance(x, (InternalConfigScope, ImmutableConfigScope)) - ) - ) - ) + """Return list of writable scopes. Higher-priority scopes come first in the list.""" + scopes = [x for x in CONFIG.scopes.values() if x.writable] + scopes.reverse() + return scopes def writable_scope_names() -> List[str]: @@ -1599,7 +1594,7 @@ def _config_from(scopes_or_paths: List[Union[ConfigScope, str]]) -> Configuratio 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(ConfigScope(name, path)) + scopes.append(DirectoryConfigScope(name, path)) configuration = Configuration(*scopes) return configuration diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index 7643579f16..501663322e 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -3028,7 +3028,7 @@ class EnvironmentManifestFile(collections.abc.Mapping): SpackEnvironmentError: if the manifest includes a remote file but no configuration stage directory has been identified """ - scopes = [] + scopes: List[spack.config.ConfigScope] = [] # load config scopes added via 'include:', in reverse so that # highest-precedence scopes are last. @@ -3097,23 +3097,21 @@ class EnvironmentManifestFile(collections.abc.Mapping): if os.path.isdir(config_path): # directories are treated as regular ConfigScopes config_name = "env:%s:%s" % (env_name, os.path.basename(config_path)) - tty.debug("Creating ConfigScope {0} for '{1}'".format(config_name, config_path)) - scope = spack.config.ConfigScope(config_name, config_path) + tty.debug(f"Creating DirectoryConfigScope {config_name} for '{config_path}'") + scopes.append(spack.config.DirectoryConfigScope(config_name, config_path)) elif os.path.exists(config_path): # files are assumed to be SingleFileScopes config_name = "env:%s:%s" % (env_name, config_path) - tty.debug( - "Creating SingleFileScope {0} for '{1}'".format(config_name, config_path) - ) - scope = spack.config.SingleFileScope( - config_name, config_path, spack.schema.merged.schema + tty.debug(f"Creating SingleFileScope {config_name} for '{config_path}'") + scopes.append( + spack.config.SingleFileScope( + config_name, config_path, spack.schema.merged.schema + ) ) else: missing.append(config_path) continue - scopes.append(scope) - if missing: msg = "Detected {0} missing include path(s):".format(len(missing)) msg += "\n {0}".format("\n ".join(missing)) @@ -3130,7 +3128,10 @@ class EnvironmentManifestFile(collections.abc.Mapping): scopes: List[spack.config.ConfigScope] = [ *self.included_config_scopes, spack.config.SingleFileScope( - self.scope_name, str(self.manifest_file), spack.schema.env.schema, [TOP_LEVEL_KEY] + self.scope_name, + str(self.manifest_file), + spack.schema.env.schema, + yaml_path=[TOP_LEVEL_KEY], ), ] ensure_no_disallowed_env_config_mods(scopes) diff --git a/lib/spack/spack/test/bindist.py b/lib/spack/spack/test/bindist.py index be88666b17..2489eaf294 100644 --- a/lib/spack/spack/test/bindist.py +++ b/lib/spack/spack/test/bindist.py @@ -115,8 +115,8 @@ def default_config(tmpdir, config_directory, monkeypatch, install_mockery_mutabl cfg = spack.config.Configuration( *[ - spack.config.ConfigScope(name, str(mutable_dir)) - for name in ["site/%s" % platform.system().lower(), "site", "user"] + spack.config.DirectoryConfigScope(name, str(mutable_dir)) + for name in [f"site/{platform.system().lower()}", "site", "user"] ] ) diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py index 14b76b06bf..26a0b37dba 100644 --- a/lib/spack/spack/test/config.py +++ b/lib/spack/spack/test/config.py @@ -774,7 +774,7 @@ def test_keys_are_ordered(configuration_dir): "./", ) - config_scope = spack.config.ConfigScope("modules", configuration_dir.join("site")) + config_scope = spack.config.DirectoryConfigScope("modules", configuration_dir.join("site")) data = config_scope.get_section("modules") @@ -956,7 +956,7 @@ config: root: dummy_tree_value """ ) - scope = spack.config.ImmutableConfigScope("test", str(tmpdir)) + scope = spack.config.DirectoryConfigScope("test", str(tmpdir), writable=False) data = scope.get_section("config") assert data["config"]["install_tree"] == {"root": "dummy_tree_value"} @@ -966,7 +966,9 @@ config: def test_single_file_scope(config, env_yaml): - scope = spack.config.SingleFileScope("env", env_yaml, spack.schema.env.schema, ["spack"]) + scope = spack.config.SingleFileScope( + "env", env_yaml, spack.schema.env.schema, yaml_path=["spack"] + ) with spack.config.override(scope): # from the single-file config @@ -1002,7 +1004,9 @@ spack: """ ) - scope = spack.config.SingleFileScope("env", env_yaml, spack.schema.env.schema, ["spack"]) + scope = spack.config.SingleFileScope( + "env", env_yaml, spack.schema.env.schema, yaml_path=["spack"] + ) with spack.config.override(scope): # from the single-file config @@ -1018,7 +1022,7 @@ spack: def test_write_empty_single_file_scope(tmpdir): env_schema = spack.schema.env.schema scope = spack.config.SingleFileScope( - "test", str(tmpdir.ensure("config.yaml")), env_schema, ["spack"] + "test", str(tmpdir.ensure("config.yaml")), env_schema, yaml_path=["spack"] ) scope._write_section("config") # confirm we can write empty config @@ -1217,7 +1221,9 @@ def test_license_dir_config(mutable_config, mock_packages): @pytest.mark.regression("22547") def test_single_file_scope_cache_clearing(env_yaml): - scope = spack.config.SingleFileScope("env", env_yaml, spack.schema.env.schema, ["spack"]) + scope = spack.config.SingleFileScope( + "env", env_yaml, spack.schema.env.schema, yaml_path=["spack"] + ) # Check that we can retrieve data from the single file scope before = scope.get_section("config") assert before diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index 94d0969d0c..1e3b336ad5 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -719,9 +719,9 @@ 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.ConfigScope("site", str(configuration_dir.join("site"))), - spack.config.ConfigScope("system", str(configuration_dir.join("system"))), - spack.config.ConfigScope("user", str(configuration_dir.join("user"))), + 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"), ] @@ -755,7 +755,7 @@ def mutable_empty_config(tmpdir_factory, configuration_dir): """Empty configuration that can be modified by the tests.""" mutable_dir = tmpdir_factory.mktemp("mutable_config").join("tmp") scopes = [ - spack.config.ConfigScope(name, str(mutable_dir.join(name))) + spack.config.DirectoryConfigScope(name, str(mutable_dir.join(name))) for name in ["site", "system", "user"] ] @@ -790,7 +790,7 @@ def concretize_scope(mutable_config, tmpdir): """Adds a scope for concretization preferences""" tmpdir.ensure_dir("concretize") mutable_config.push_scope( - spack.config.ConfigScope("concretize", str(tmpdir.join("concretize"))) + spack.config.DirectoryConfigScope("concretize", str(tmpdir.join("concretize"))) ) yield str(tmpdir.join("concretize")) @@ -802,10 +802,10 @@ def concretize_scope(mutable_config, tmpdir): @pytest.fixture def no_compilers_yaml(mutable_config): """Creates a temporary configuration without compilers.yaml""" - for scope, local_config in mutable_config.scopes.items(): - if not local_config.path: # skip internal scopes + for local_config in mutable_config.scopes.values(): + if not isinstance(local_config, spack.config.DirectoryConfigScope): continue - compilers_yaml = os.path.join(local_config.path, "compilers.yaml") + compilers_yaml = local_config.get_section_filename("compilers") if os.path.exists(compilers_yaml): os.remove(compilers_yaml) return mutable_config @@ -814,7 +814,9 @@ def no_compilers_yaml(mutable_config): @pytest.fixture() def mock_low_high_config(tmpdir): """Mocks two configuration scopes: 'low' and 'high'.""" - scopes = [spack.config.ConfigScope(name, str(tmpdir.join(name))) for name in ["low", "high"]] + scopes = [ + spack.config.DirectoryConfigScope(name, str(tmpdir.join(name))) for name in ["low", "high"] + ] with spack.config.use_configuration(*scopes) as config: yield config -- cgit v1.2.3-70-g09d2