From 44cb4eca933e53bd0af32e0715195c5c5797b689 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Fri, 19 Apr 2024 21:39:34 +0200 Subject: [PATCH] environment.py: fix excessive re-reads (#43746) --- lib/spack/spack/config.py | 5 ++- lib/spack/spack/environment/environment.py | 46 ++++++++++++---------- lib/spack/spack/test/cmd/env.py | 10 ++--- 3 files changed, 33 insertions(+), 28 deletions(-) diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py index f58c331edb..2a2f180f45 100644 --- a/lib/spack/spack/config.py +++ b/lib/spack/spack/config.py @@ -1562,8 +1562,9 @@ def ensure_latest_format_fn(section: str) -> Callable[[YamlConfigDict], bool]: def use_configuration( *scopes_or_paths: Union[ConfigScope, str] ) -> Generator[Configuration, None, None]: - """Use the configuration scopes passed as arguments within the - context manager. + """Use the configuration scopes passed as arguments within the context manager. + + This function invalidates caches, and is therefore very slow. Args: *scopes_or_paths: scope objects or paths to be used diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index da241417d6..f394efd7de 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -106,17 +106,16 @@ def environment_name(path: Union[str, pathlib.Path]) -> str: return path_str -def check_disallowed_env_config_mods(scopes): +def ensure_no_disallowed_env_config_mods(scopes: List[spack.config.ConfigScope]) -> None: for scope in scopes: - with spack.config.use_configuration(scope): - if spack.config.get("config:environments_root"): - raise SpackEnvironmentError( - "Spack environments are prohibited from modifying 'config:environments_root' " - "because it can make the definition of the environment ill-posed. Please " - "remove from your environment and place it in a permanent scope such as " - "defaults, system, site, etc." - ) - return scopes + config = scope.get_section("config") + if config and "environments_root" in config["config"]: + raise SpackEnvironmentError( + "Spack environments are prohibited from modifying 'config:environments_root' " + "because it can make the definition of the environment ill-posed. Please " + "remove from your environment and place it in a permanent scope such as " + "defaults, system, site, etc." + ) def default_manifest_yaml(): @@ -2463,6 +2462,10 @@ def __init__(self, manifest_dir: Union[pathlib.Path, str]) -> None: self.scope_name = f"env:{environment_name(self.manifest_dir)}" self.config_stage_dir = os.path.join(env_subdir_path(manifest_dir), "config") + #: Configuration scopes associated with this environment. Note that these are not + #: invalidated by a re-read of the manifest file. + self._config_scopes: Optional[List[spack.config.ConfigScope]] = None + if not self.manifest_file.exists(): msg = f"cannot find '{manifest_name}' in {self.manifest_dir}" raise SpackEnvironmentError(msg) @@ -2808,16 +2811,19 @@ def included_config_scopes(self) -> List[spack.config.ConfigScope]: @property def env_config_scopes(self) -> List[spack.config.ConfigScope]: - """A list of all configuration scopes for the environment manifest. - - Returns: All configuration scopes associated with the environment - """ - config_name = self.scope_name - env_scope = spack.config.SingleFileScope( - config_name, str(self.manifest_file), spack.schema.env.schema, [TOP_LEVEL_KEY] - ) - - return check_disallowed_env_config_mods(self.included_config_scopes + [env_scope]) + """A list of all configuration scopes for the environment manifest. On the first call this + instantiates all the scopes, on subsequent calls it returns the cached list.""" + if self._config_scopes is not None: + return self._config_scopes + 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] + ), + ] + ensure_no_disallowed_env_config_mods(scopes) + self._config_scopes = scopes + return scopes def prepare_config_scope(self) -> None: """Add the manifest's scopes to the global configuration search path.""" diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py index 5c23c61b61..264ea5de0d 100644 --- a/lib/spack/spack/test/cmd/env.py +++ b/lib/spack/spack/test/cmd/env.py @@ -858,8 +858,7 @@ def test_with_config_bad_include_activate(environment_from_manifest, tmpdir): """ ) - e = ev.Environment(env_root) - with e: + with ev.Environment(env_root) as e: e.concretize() # we've created an environment with some included config files (which do @@ -869,7 +868,7 @@ def test_with_config_bad_include_activate(environment_from_manifest, tmpdir): os.remove(abs_include_path) os.remove(include1) with pytest.raises(spack.config.ConfigFileError) as exc: - ev.activate(e) + ev.activate(ev.Environment(env_root)) err = exc.value.message assert "missing include" in err @@ -1063,8 +1062,7 @@ def test_config_change_new(mutable_mock_env_path, tmp_path, mock_packages, mutab """ ) - e = ev.Environment(tmp_path) - with e: + with ev.Environment(tmp_path): config("change", "packages:mpich:require:~debug") with pytest.raises(spack.solver.asp.UnsatisfiableSpecError): spack.spec.Spec("mpich+debug").concretized() @@ -1081,7 +1079,7 @@ def test_config_change_new(mutable_mock_env_path, tmp_path, mock_packages, mutab require: "@3.0.3" """ ) - with e: + with ev.Environment(tmp_path): assert spack.spec.Spec("mpich").concretized().satisfies("@3.0.3") with pytest.raises(spack.config.ConfigError, match="not a list"): config("change", "packages:mpich:require:~debug")