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

@@ -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(
+        platform_scope = spack.config.DirectoryConfigScope(
-            "/".join([name, platform]), os.path.join(path, platform)
+            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))

lib/spack/spack/ci.py

@@ -809,7 +809,8 @@ def ensure_expected_target_path(path):
         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)
         ]

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

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)
+    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(
+    updates: List[spack.config.ConfigScope] = [
-        filter(
+        x
-            lambda s: not isinstance(
+        for x in spack.config.CONFIG.format_updates[args.section]
-                s, (spack.config.InternalConfigScope, spack.config.ImmutableConfigScope)
+        if not isinstance(x, spack.config.InternalConfigScope) and x.writable
-            ),
+    ]
-            spack.config.CONFIG.format_updates[args.section],
-        )
-    )
 
     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"])

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:

lib/spack/spack/config.py

@@ -35,7 +35,7 @@
 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 @@
 
 
 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.
+    def get_section_filename(self, section: str) -> str:
-    Each file is a config "section" (e.g., mirrors, compilers, etc.).
+        raise NotImplementedError
-    """
 
-    def __init__(self, name, path) -> None:
+    def get_section(self, section: str) -> Optional[YamlConfigDict]:
-        self.name = name  # scope name.
+        raise NotImplementedError
-        self.path = path  # path to directory containing configs.
+
-        self.sections = syaml.syaml_dict()  # sections read from config files.
+    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 False
-        return os.sep in self.name
+
+    def clear(self) -> None:
+        """Empty cached config information."""
+        self.sections = syaml.syaml_dict()
+
+    def __repr__(self) -> str:
+        return f"<ConfigScope: {self.name}>"
+
+
+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 @@ def get_section(self, section: str) -> Optional[YamlConfigDict]:
         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 @@ def _write_section(self, section: str) -> None:
         except (syaml.SpackYAMLError, OSError) as e:
             raise ConfigFileError(f"cannot write to '{filename}'") from e
 
-    def clear(self) -> None:
+    @property
-        """Empty cached config information."""
+    def is_platform_dependent(self) -> bool:
-        self.sections = syaml.syaml_dict()
+        """Returns true if the scope name is platform specific"""
-
+        return "/" in self.name
-    def __repr__(self) -> str:
-        return f"<ConfigScope: {self.name}: {self.path}>"
 
 
 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 @@ def __init__(
                        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 @@ def get_section(self, section: str) -> Optional[YamlConfigDict]:
         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 @@ def __repr__(self) -> str:
         return f"<SingleFileScope: {self.name}: {self.path}>"
 
 
-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"<ImmutableConfigScope: {self.name}: {self.path}>"
-
-
 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 @@ def __init__(self, name: str, data: Optional[YamlConfigDict] = None) -> None:
                 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 @@ def remove_scope(self, scope_name: str) -> Optional[ConfigScope]:
         return scope
 
     @property
-    def file_scopes(self) -> List[ConfigScope]:
+    def writable_scopes(self) -> Generator[ConfigScope, None, None]:
-        """List of writable scopes with an associated file."""
+        """Generator of writable scopes with an associated file."""
-        return [
+        return (s for s in self.scopes.values() if s.writable)
-            s
-            for s in self.scopes.values()
-            if (type(s) is ConfigScope or type(s) is SingleFileScope)
-        ]
 
     def highest_precedence_scope(self) -> ConfigScope:
-        """Non-internal scope with highest precedence."""
+        """Writable scope with highest precedence."""
-        return next(reversed(self.file_scopes))
+        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
+        """Writable non-platform scope with highest precedence"""
-
+        return next(
-        Platform-specific scopes are of the form scope/platform"""
+            s
-        generator = reversed(self.file_scopes)
+            for s in reversed(self.scopes.values())  # type: ignore
-        highest = next(generator)
+            if s.writable and not s.is_platform_dependent
-        while highest and highest.is_platform_dependent:
+        )
-            highest = next(generator)
-        return highest
 
     def matching_scopes(self, reg_expr) -> List[ConfigScope]:
         """
@@ -755,13 +758,14 @@ def override(
 
 
 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)
+    scope = DirectoryConfigScope(
-    plat_path = os.path.join(path, platform)
+        f"{name}/{platform}", os.path.join(path, platform), writable=writable
-    cfg.push_scope(scope_type(plat_name, plat_path))
+    )
+    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))
+        cfg.push_scope(DirectoryConfigScope(name, path, writable=False))
-        _add_platform_scope(cfg, ImmutableConfigScope, name, path)
+        _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 of writable scopes. Higher-priority scopes come first in the
+    scopes = [x for x in CONFIG.scopes.values() if x.writable]
-    list.
+    scopes.reverse()
-    """
+    return scopes
-    return list(
-        reversed(
-            list(
-                x
-                for x in CONFIG.scopes.values()
-                if not isinstance(x, (InternalConfigScope, ImmutableConfigScope))
-            )
-        )
-    )
 
 
 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

lib/spack/spack/environment/environment.py

@@ -3028,7 +3028,7 @@ def included_config_scopes(self) -> List[spack.config.ConfigScope]:
             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 @@ def included_config_scopes(self) -> List[spack.config.ConfigScope]:
             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))
+                tty.debug(f"Creating DirectoryConfigScope {config_name} for '{config_path}'")
-                scope = spack.config.ConfigScope(config_name, 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(
+                tty.debug(f"Creating SingleFileScope {config_name} for '{config_path}'")
-                    "Creating SingleFileScope {0} for '{1}'".format(config_name, config_path)
+                scopes.append(
-                )
+                    spack.config.SingleFileScope(
-                scope = 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 @@ def env_config_scopes(self) -> List[spack.config.ConfigScope]:
         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)

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))
+            spack.config.DirectoryConfigScope(name, str(mutable_dir))
-            for name in ["site/%s" % platform.system().lower(), "site", "user"]
+            for name in [f"site/{platform.system().lower()}", "site", "user"]
         ]
     )
 

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 @@ def test_immutable_scope(tmpdir):
       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 @@ def test_immutable_scope(tmpdir):
 
 
 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 @@ def test_single_file_scope_section_override(tmpdir, config):
 """
         )
 
-    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 @@ def test_single_file_scope_section_override(tmpdir, config):
 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

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.DirectoryConfigScope("site", str(configuration_dir.join("site"))),
-        spack.config.ConfigScope("system", str(configuration_dir.join("system"))),
+        spack.config.DirectoryConfigScope("system", str(configuration_dir.join("system"))),
-        spack.config.ConfigScope("user", str(configuration_dir.join("user"))),
+        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():
+    for local_config in mutable_config.scopes.values():
-        if not local_config.path:  # skip internal scopes
+        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