Environments: Fix environment configuration (#42058)

* Environments: fix environment config
* Don't change the lockfile manifest path
* Update activate's comments to tie the manifest to the configuration
* Add spec_list override method
* Remove type checking from 'activate' since already have built-in check
* Refactor global methods tied to the manifest to be in its class
* Restore Environment's 'env_subdir_path' and pass its config_stage_dir to EnvironmentManifestFile
* Restore global env_subdir_path for use by Environment and EnvironmentManifestFile

5 files changed, 220 insertions, 195 deletions

diff --git a/lib/spack/spack/cmd/config.py b/lib/spack/spack/cmd/config.py
index ee0bad2bae..61b27bcdfe 100644
--- a/lib/spack/spack/cmd/config.py
+++ b/lib/spack/spack/cmd/config.py
@@ -122,7 +122,7 @@ def _get_scope_and_section(args):
     if not section and not scope:
         env = ev.active_environment()
         if env:
-            scope = env.env_file_config_scope_name()
+            scope = env.scope_name
 
     # set scope defaults
     elif not scope:
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 8566244a49..25e2360fdc 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -83,17 +83,30 @@ manifest_name = "spack.yaml"
 lockfile_name = "spack.lock"
 
 
-#: Name of the directory where environments store repos, logs, views
+#: Name of the directory where environments store repos, logs, views, configs
 env_subdir_name = ".spack-env"
 
 
-def env_root_path():
+def env_root_path() -> str:
     """Override default root path if the user specified it"""
     return spack.util.path.canonicalize_path(
         spack.config.get("config:environments_root", default=default_env_path)
     )
 
 
+def environment_name(path: Union[str, pathlib.Path]) -> str:
+    """Human-readable representation of the environment.
+
+    This is the path for directory environments, and just the name
+    for managed environments.
+    """
+    path_str = str(path)
+    if path_str.startswith(env_root_path()):
+        return os.path.basename(path_str)
+    else:
+        return path_str
+
+
 def check_disallowed_env_config_mods(scopes):
     for scope in scopes:
         with spack.config.use_configuration(scope):
@@ -179,9 +192,8 @@ def validate_env_name(name):
 def activate(env, use_env_repo=False):
     """Activate an environment.
 
-    To activate an environment, we add its configuration scope to the
-    existing Spack configuration, and we set active to the current
-    environment.
+    To activate an environment, we add its manifest's configuration scope to the
+    existing Spack configuration, and we set active to the current environment.
 
     Arguments:
         env (Environment): the environment to activate
@@ -198,7 +210,7 @@ def activate(env, use_env_repo=False):
     # below.
     install_tree_before = spack.config.get("config:install_tree")
     upstreams_before = spack.config.get("upstreams")
-    prepare_config_scope(env)
+    env.manifest.prepare_config_scope()
     install_tree_after = spack.config.get("config:install_tree")
     upstreams_after = spack.config.get("upstreams")
     if install_tree_before != install_tree_after or upstreams_before != upstreams_after:
@@ -226,7 +238,7 @@ def deactivate():
     if hasattr(_active_environment, "store_token"):
         spack.store.restore(_active_environment.store_token)
         delattr(_active_environment, "store_token")
-    deactivate_config_scope(_active_environment)
+    _active_environment.manifest.deactivate_config_scope()
 
     # use _repo so we only remove if a repo was actually constructed
     if _active_environment._repo:
@@ -363,7 +375,7 @@ def _rewrite_relative_dev_paths_on_relocation(env, init_file_dir):
     to store the environment in a different directory, we have to rewrite
     relative paths to absolute ones."""
     with env:
-        dev_specs = spack.config.get("develop", default={}, scope=env.env_file_config_scope_name())
+        dev_specs = spack.config.get("develop", default={}, scope=env.scope_name)
         if not dev_specs:
             return
         for name, entry in dev_specs.items():
@@ -378,7 +390,7 @@ def _rewrite_relative_dev_paths_on_relocation(env, init_file_dir):
 
             dev_specs[name]["path"] = expanded_path
 
-        spack.config.set("develop", dev_specs, scope=env.env_file_config_scope_name())
+        spack.config.set("develop", dev_specs, scope=env.scope_name)
 
         env._dev_specs = None
         # If we changed the environment's spack.yaml scope, that will not be reflected
@@ -769,6 +781,17 @@ def _create_environment(path):
     return Environment(path)
 
 
+def env_subdir_path(manifest_dir: Union[str, pathlib.Path]) -> str:
+    """Path to where the environment stores repos, logs, views, configs.
+
+    Args:
+        manifest_dir:  directory containing the environment manifest file
+
+    Returns:  directory the environment uses to manage its files
+    """
+    return os.path.join(str(manifest_dir), env_subdir_name)
+
+
 class Environment:
     """A Spack environment, which bundles together configuration and a list of specs."""
 
@@ -780,6 +803,8 @@ class Environment:
             manifest_dir: directory with the "spack.yaml" associated with the environment
         """
         self.path = os.path.abspath(str(manifest_dir))
+        self.name = environment_name(self.path)
+        self.env_subdir_path = env_subdir_path(self.path)
 
         self.txlock = lk.Lock(self._transaction_lock_path)
 
@@ -802,9 +827,15 @@ class Environment:
         self._previous_active = None
         self._dev_specs = None
 
+        # Load the manifest file contents into memory
+        self._load_manifest_file()
+
+    def _load_manifest_file(self):
+        """Instantiate and load the manifest file contents into memory."""
         with lk.ReadTransaction(self.txlock):
-            self.manifest = EnvironmentManifestFile(manifest_dir)
-            self._read()
+            self.manifest = EnvironmentManifestFile(self.path)
+            with self.manifest.use_config():
+                self._read()
 
     @property
     def unify(self):
@@ -822,19 +853,10 @@ class Environment:
     def _re_read(self):
         """Reinitialize the environment object."""
         self.clear(re_read=True)
-        self.manifest = EnvironmentManifestFile(self.path)
-        self._read(re_read=True)
-
-    def _read(self, re_read=False):
-        # If the manifest has included files, then some of the information
-        # (e.g., definitions) MAY be in those files. So we need to ensure
-        # the config is populated with any associated spec lists in order
-        # to fully construct the manifest state.
-        includes = self.manifest[TOP_LEVEL_KEY].get("include", [])
-        if includes and not re_read:
-            prepare_config_scope(self)
+        self._load_manifest_file()
 
-        self._construct_state_from_manifest(re_read)
+    def _read(self):
+        self._construct_state_from_manifest()
 
         if os.path.exists(self.lock_path):
             with open(self.lock_path) as f:
@@ -861,18 +883,14 @@ class Environment:
             else:
                 self.spec_lists[name] = user_specs
 
-    def _construct_state_from_manifest(self, re_read=False):
-        """Read manifest file and set up user specs."""
+    def _construct_state_from_manifest(self):
+        """Set up user specs and views from the manifest file."""
         self.spec_lists = collections.OrderedDict()
 
-        if not re_read:
-            for item in spack.config.get("definitions", []):
-                self._process_definition(item)
-
-        env_configuration = self.manifest[TOP_LEVEL_KEY]
-        for item in env_configuration.get("definitions", []):
+        for item in spack.config.get("definitions", []):
             self._process_definition(item)
 
+        env_configuration = self.manifest[TOP_LEVEL_KEY]
         spec_list = env_configuration.get(user_speclist_name, [])
         user_specs = SpecList(
             user_speclist_name, [s for s in spec_list if s], self.spec_lists.copy()
@@ -921,7 +939,7 @@ class Environment:
         """Clear the contents of the environment
 
         Arguments:
-            re_read (bool): If True, do not clear ``new_specs`` nor
+            re_read (bool): If ``True``, do not clear ``new_specs`` nor
                 ``new_installs`` values. These values cannot be read from
                 yaml, and need to be maintained when re-reading an existing
                 environment.
@@ -941,23 +959,6 @@ class Environment:
             self.new_installs = []  # write modules for these on write()
 
     @property
-    def internal(self):
-        """Whether this environment is managed by Spack."""
-        return self.path.startswith(env_root_path())
-
-    @property
-    def name(self):
-        """Human-readable representation of the environment.
-
-        This is the path for directory environments, and just the name
-        for managed environments.
-        """
-        if self.internal:
-            return os.path.basename(self.path)
-        else:
-            return self.path
-
-    @property
     def active(self):
         """True if this environment is currently active."""
         return _active_environment and self.path == _active_environment.path
@@ -985,18 +986,8 @@ class Environment:
         return self.lock_path + ".backup.v1"
 
     @property
-    def env_subdir_path(self):
-        """Path to directory where the env stores repos, logs, views."""
-        return os.path.join(self.path, env_subdir_name)
-
-    @property
     def repos_path(self):
-        return os.path.join(self.path, env_subdir_name, "repos")
-
-    @property
-    def config_stage_dir(self):
-        """Directory for any staged configuration file(s)."""
-        return os.path.join(self.env_subdir_path, "config")
+        return os.path.join(self.env_subdir_path, "repos")
 
     @property
     def view_path_default(self):
@@ -1009,122 +1000,10 @@ class Environment:
             self._repo = make_repo_path(self.repos_path)
         return self._repo
 
-    def included_config_scopes(self):
-        """List of included configuration scopes from the environment.
-
-        Scopes are listed in the YAML file in order from highest to
-        lowest precedence, so configuration from earlier scope will take
-        precedence over later ones.
-
-        This routine returns them in the order they should be pushed onto
-        the internal scope stack (so, in reverse, from lowest to highest).
-        """
-        scopes = []
-
-        # load config scopes added via 'include:', in reverse so that
-        # highest-precedence scopes are last.
-        includes = self.manifest[TOP_LEVEL_KEY].get("include", [])
-        missing = []
-        for i, config_path in enumerate(reversed(includes)):
-            # allow paths to contain spack config/environment variables, etc.
-            config_path = substitute_path_variables(config_path)
-
-            include_url = urllib.parse.urlparse(config_path)
-
-            # Transform file:// URLs to direct includes.
-            if include_url.scheme == "file":
-                config_path = urllib.request.url2pathname(include_url.path)
-
-            # Any other URL should be fetched.
-            elif include_url.scheme in ("http", "https", "ftp"):
-                # Stage any remote configuration file(s)
-                staged_configs = (
-                    os.listdir(self.config_stage_dir)
-                    if os.path.exists(self.config_stage_dir)
-                    else []
-                )
-                remote_path = urllib.request.url2pathname(include_url.path)
-                basename = os.path.basename(remote_path)
-                if basename in staged_configs:
-                    # Do NOT re-stage configuration files over existing
-                    # ones with the same name since there is a risk of
-                    # losing changes (e.g., from 'spack config update').
-                    tty.warn(
-                        "Will not re-stage configuration from {0} to avoid "
-                        "losing changes to the already staged file of the "
-                        "same name.".format(remote_path)
-                    )
-
-                    # Recognize the configuration stage directory
-                    # is flattened to ensure a single copy of each
-                    # configuration file.
-                    config_path = self.config_stage_dir
-                    if basename.endswith(".yaml"):
-                        config_path = os.path.join(config_path, basename)
-                else:
-                    staged_path = spack.config.fetch_remote_configs(
-                        config_path, self.config_stage_dir, skip_existing=True
-                    )
-                    if not staged_path:
-                        raise SpackEnvironmentError(
-                            "Unable to fetch remote configuration {0}".format(config_path)
-                        )
-                    config_path = staged_path
-
-            elif include_url.scheme:
-                raise ValueError(
-                    f"Unsupported URL scheme ({include_url.scheme}) for "
-                    f"environment include: {config_path}"
-                )
-
-            # treat relative paths as relative to the environment
-            if not os.path.isabs(config_path):
-                config_path = os.path.join(self.path, config_path)
-                config_path = os.path.normpath(os.path.realpath(config_path))
-
-            if os.path.isdir(config_path):
-                # directories are treated as regular ConfigScopes
-                config_name = "env:%s:%s" % (self.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)
-            elif os.path.exists(config_path):
-                # files are assumed to be SingleFileScopes
-                config_name = "env:%s:%s" % (self.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
-                )
-            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))
-            raise spack.config.ConfigFileError(msg)
-
-        return scopes
-
-    def env_file_config_scope_name(self):
+    @property
+    def scope_name(self):
         """Name of the config scope of this environment's manifest file."""
-        return "env:%s" % self.name
-
-    def env_file_config_scope(self):
-        """Get the configuration scope for the environment's manifest file."""
-        config_name = self.env_file_config_scope_name()
-        return spack.config.SingleFileScope(
-            config_name, self.manifest_path, spack.schema.env.schema, [TOP_LEVEL_KEY]
-        )
-
-    def config_scopes(self):
-        """A list of all configuration scopes for this environment."""
-        return check_disallowed_env_config_mods(
-            self.included_config_scopes() + [self.env_file_config_scope()]
-        )
+        return self.manifest.scope_name
 
     def destroy(self):
         """Remove this environment from Spack entirely."""
@@ -1224,7 +1103,7 @@ class Environment:
 
         for idx, spec in matches:
             override_spec = Spec.override(spec, change_spec)
-            self.spec_lists[list_name].specs[idx] = override_spec
+            self.spec_lists[list_name].replace(idx, str(override_spec))
             if list_name == user_speclist_name:
                 self.manifest.override_user_spec(str(override_spec), idx=idx)
             else:
@@ -1232,7 +1111,6 @@ class Environment:
                     str(spec), override=str(override_spec), list_name=list_name
                 )
         self.update_stale_references(from_list=list_name)
-        self._construct_state_from_manifest()
 
     def remove(self, query_spec, list_name=user_speclist_name, force=False):
         """Remove specs from an environment that match a query_spec"""
@@ -2401,18 +2279,6 @@ def make_repo_path(root):
     return path
 
 
-def prepare_config_scope(env):
-    """Add env's scope to the global configuration search path."""
-    for scope in env.config_scopes():
-        spack.config.CONFIG.push_scope(scope)
-
-
-def deactivate_config_scope(env):
-    """Remove any scopes from env from the global config path."""
-    for scope in env.config_scopes():
-        spack.config.CONFIG.remove_scope(scope.name)
-
-
 def manifest_file(env_name_or_dir):
     """Return the absolute path to a manifest file given the environment
     name or directory.
@@ -2591,8 +2457,9 @@ class EnvironmentManifestFile(collections.abc.Mapping):
         already existing in the directory.
 
         Args:
-             manifest_dir: directory where the lockfile is
+             manifest_dir: directory containing the manifest and lockfile
         """
+        # TBD: Should this be the abspath?
         manifest_dir = pathlib.Path(manifest_dir)
         lockfile = manifest_dir / lockfile_name
         with lockfile.open("r") as f:
@@ -2610,6 +2477,8 @@ class EnvironmentManifestFile(collections.abc.Mapping):
     def __init__(self, manifest_dir: Union[pathlib.Path, str]) -> None:
         self.manifest_dir = pathlib.Path(manifest_dir)
         self.manifest_file = self.manifest_dir / manifest_name
+        self.scope_name = f"env:{environment_name(self.manifest_dir)}"
+        self.config_stage_dir = os.path.join(env_subdir_path(manifest_dir), "config")
 
         if not self.manifest_file.exists():
             msg = f"cannot find '{manifest_name}' in {self.manifest_dir}"
@@ -2846,6 +2715,145 @@ class EnvironmentManifestFile(collections.abc.Mapping):
     def __str__(self):
         return str(self.manifest_file)
 
+    @property
+    def included_config_scopes(self) -> List[spack.config.ConfigScope]:
+        """List of included configuration scopes from the manifest.
+
+        Scopes are listed in the YAML file in order from highest to
+        lowest precedence, so configuration from earlier scope will take
+        precedence over later ones.
+
+        This routine returns them in the order they should be pushed onto
+        the internal scope stack (so, in reverse, from lowest to highest).
+
+        Returns: Configuration scopes associated with the environment manifest
+
+        Raises:
+            SpackEnvironmentError: if the manifest includes a remote file but
+                no configuration stage directory has been identified
+        """
+        scopes = []
+
+        # load config scopes added via 'include:', in reverse so that
+        # highest-precedence scopes are last.
+        includes = self[TOP_LEVEL_KEY].get("include", [])
+        env_name = environment_name(self.manifest_dir)
+        missing = []
+        for i, config_path in enumerate(reversed(includes)):
+            # allow paths to contain spack config/environment variables, etc.
+            config_path = substitute_path_variables(config_path)
+
+            include_url = urllib.parse.urlparse(config_path)
+
+            # Transform file:// URLs to direct includes.
+            if include_url.scheme == "file":
+                config_path = urllib.request.url2pathname(include_url.path)
+
+            # Any other URL should be fetched.
+            elif include_url.scheme in ("http", "https", "ftp"):
+                # Stage any remote configuration file(s)
+                staged_configs = (
+                    os.listdir(self.config_stage_dir)
+                    if os.path.exists(self.config_stage_dir)
+                    else []
+                )
+                remote_path = urllib.request.url2pathname(include_url.path)
+                basename = os.path.basename(remote_path)
+                if basename in staged_configs:
+                    # Do NOT re-stage configuration files over existing
+                    # ones with the same name since there is a risk of
+                    # losing changes (e.g., from 'spack config update').
+                    tty.warn(
+                        "Will not re-stage configuration from {0} to avoid "
+                        "losing changes to the already staged file of the "
+                        "same name.".format(remote_path)
+                    )
+
+                    # Recognize the configuration stage directory
+                    # is flattened to ensure a single copy of each
+                    # configuration file.
+                    config_path = self.config_stage_dir
+                    if basename.endswith(".yaml"):
+                        config_path = os.path.join(config_path, basename)
+                else:
+                    staged_path = spack.config.fetch_remote_configs(
+                        config_path, str(self.config_stage_dir), skip_existing=True
+                    )
+                    if not staged_path:
+                        raise SpackEnvironmentError(
+                            "Unable to fetch remote configuration {0}".format(config_path)
+                        )
+                    config_path = staged_path
+
+            elif include_url.scheme:
+                raise ValueError(
+                    f"Unsupported URL scheme ({include_url.scheme}) for "
+                    f"environment include: {config_path}"
+                )
+
+            # treat relative paths as relative to the environment
+            if not os.path.isabs(config_path):
+                config_path = os.path.join(self.manifest_dir, config_path)
+                config_path = os.path.normpath(os.path.realpath(config_path))
+
+            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)
+            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
+                )
+            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))
+            raise spack.config.ConfigFileError(msg)
+
+        return scopes
+
+    @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])
+
+    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)
+
+    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)
+
+    @contextlib.contextmanager
+    def use_config(self):
+        """Ensure only the manifest's configuration scopes are global."""
+        with no_active_environment():
+            self.prepare_config_scope()
+            yield
+            self.deactivate_config_scope()
+
 
 class SpackEnvironmentError(spack.error.SpackError):
     """Superclass for all errors to do with Spack environments."""
diff --git a/lib/spack/spack/spec_list.py b/lib/spack/spack/spec_list.py
index 7f9efdfa59..9779d40fe4 100644
--- a/lib/spack/spack/spec_list.py
+++ b/lib/spack/spack/spec_list.py
@@ -107,6 +107,20 @@ class SpecList:
         self._constraints = None
         self._specs = None
 
+    def replace(self, idx: int, spec: str):
+        """Replace the existing spec at the index with the new one.
+
+        Args:
+            idx: index of the spec to replace in the speclist
+            spec: new spec
+        """
+        self.yaml_list[idx] = spec
+
+        # invalidate cache variables when we change the list
+        self._expanded_list = None
+        self._constraints = None
+        self._specs = None
+
     def extend(self, other, copy_reference=True):
         self.yaml_list.extend(other.yaml_list)
         self._expanded_list = None
@@ -148,6 +162,7 @@ class SpecList:
                 if isinstance(item, str) and item.startswith("$"):
                     # replace the reference and apply the sigil if needed
                     name, sigil = self._parse_reference(item)
+
                     referent = [
                         _sigilify(item, sigil) for item in self._reference[name].specs_as_yaml_list
                     ]
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index f3da11d1d6..74ce4d31d1 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -974,8 +974,6 @@ def test_env_with_included_config_file_url(tmpdir, mutable_empty_config, package
 
     env = ev.Environment(tmpdir.strpath)
     ev.activate(env)
-    scopes = env.included_config_scopes()
-    assert len(scopes) == 1
 
     cfg = spack.config.get("packages")
     assert cfg["mpileaks"]["version"] == ["2.2"]
@@ -3670,8 +3668,6 @@ def test_env_include_packages_url(
     with spack.config.override("config:url_fetch_method", "curl"):
         env = ev.Environment(tmpdir.strpath)
         ev.activate(env)
-        scopes = env.included_config_scopes()
-        assert len(scopes) == 1
 
         cfg = spack.config.get("packages")
         assert "openmpi" in cfg["all"]["providers"]["mpi"]
diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py
index 0d1a4b08fb..54ac9a3732 100644
--- a/lib/spack/spack/test/env.py
+++ b/lib/spack/spack/test/env.py
@@ -108,6 +108,12 @@ def test_env_change_spec_in_definition(tmp_path, mock_packages, config, mutable_
     e.change_existing_spec(spack.spec.Spec("mpileaks@2.2"), list_name="desired_specs")
     e.write()
 
+    # Ensure changed specs are in memory
+    assert any(x.intersects("mpileaks@2.2%gcc") for x in e.user_specs)
+    assert not any(x.intersects("mpileaks@2.1%gcc") for x in e.user_specs)
+
+    # Now make sure the changes can be read from the modified config
+    e = ev.read("test")
     assert any(x.intersects("mpileaks@2.2%gcc") for x in e.user_specs)
     assert not any(x.intersects("mpileaks@2.1%gcc") for x in e.user_specs)