From ddfc43be96521118f16878c5d0444917eef057f0 Mon Sep 17 00:00:00 2001
From: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Date: Thu, 8 Jun 2023 20:34:17 +0200
Subject: Forbid using `env:` as a top level environment attribute (#38199)

* Remove "env" from environment schema

* Remove spack.env.schema.keys

* Remove spack.environment.config_dict
---
 lib/spack/spack/ci.py                         |  2 +-
 lib/spack/spack/cmd/ci.py                     |  2 +-
 lib/spack/spack/config.py                     | 18 ++---
 lib/spack/spack/container/__init__.py         |  2 +-
 lib/spack/spack/container/writers/__init__.py |  4 +-
 lib/spack/spack/environment/__init__.py       |  4 +-
 lib/spack/spack/environment/environment.py    | 96 ++++++++++++---------------
 lib/spack/spack/schema/ci.py                  |  2 +-
 lib/spack/spack/schema/env.py                 |  8 +--
 lib/spack/spack/test/cmd/develop.py           |  2 +-
 10 files changed, 60 insertions(+), 80 deletions(-)

diff --git a/lib/spack/spack/ci.py b/lib/spack/spack/ci.py
index 6739eb9813..3fe7f5f8d0 100644
--- a/lib/spack/spack/ci.py
+++ b/lib/spack/spack/ci.py
@@ -751,7 +751,7 @@ def generate_gitlab_ci_yaml(
             env.concretize()
             env.write()
 
-    yaml_root = ev.config_dict(env.manifest)
+    yaml_root = env.manifest[ev.TOP_LEVEL_KEY]
 
     # Get the joined "ci" config with all of the current scopes resolved
     ci_config = cfg.get("ci")
diff --git a/lib/spack/spack/cmd/ci.py b/lib/spack/spack/cmd/ci.py
index 42d61d438a..b004c1373b 100644
--- a/lib/spack/spack/cmd/ci.py
+++ b/lib/spack/spack/cmd/ci.py
@@ -228,7 +228,7 @@ def ci_reindex(args):
     Use the active, gitlab-enabled environment to rebuild the buildcache
     index for the associated mirror."""
     env = spack.cmd.require_active_env(cmd_name="ci rebuild-index")
-    yaml_root = ev.config_dict(env.manifest)
+    yaml_root = env.manifest[ev.TOP_LEVEL_KEY]
 
     if "mirrors" not in yaml_root or len(yaml_root["mirrors"].values()) < 1:
         tty.die("spack ci rebuild-index requires an env containing a mirror")
diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py
index 1bc5b9b9a4..81842cde15 100644
--- a/lib/spack/spack/config.py
+++ b/lib/spack/spack/config.py
@@ -81,7 +81,7 @@ section_schemas = {
 # Same as above, but including keys for environments
 # this allows us to unify config reading between configs and environments
 all_schemas = copy.deepcopy(section_schemas)
-all_schemas.update(dict((key, spack.schema.env.schema) for key in spack.schema.env.keys))
+all_schemas.update({spack.schema.env.TOP_LEVEL_KEY: spack.schema.env.schema})
 
 #: Path to the default configuration
 configuration_defaults_path = ("defaults", os.path.join(spack.paths.etc_path, "defaults"))
@@ -111,14 +111,6 @@ scopes_metavar = "{defaults,system,site,user}[/PLATFORM] or env:ENVIRONMENT"
 overrides_base_name = "overrides-"
 
 
-def first_existing(dictionary, keys):
-    """Get the value of the first key in keys that is in the dictionary."""
-    try:
-        return next(k for k in keys if k in dictionary)
-    except StopIteration:
-        raise KeyError("None of %s is in dict!" % str(keys))
-
-
 class ConfigScope(object):
     """This class represents a configuration scope.
 
@@ -838,12 +830,10 @@ config: Union[Configuration, llnl.util.lang.Singleton] = llnl.util.lang.Singleto
 
 def add_from_file(filename, scope=None):
     """Add updates to a config from a filename"""
-    import spack.environment as ev
-
-    # Get file as config dict
+    # Extract internal attributes, if we are dealing with an environment
     data = read_config_file(filename)
-    if any(k in data for k in spack.schema.env.keys):
-        data = ev.config_dict(data)
+    if spack.schema.env.TOP_LEVEL_KEY in data:
+        data = data[spack.schema.env.TOP_LEVEL_KEY]
 
     # update all sections from config dict
     # We have to iterate on keys to keep overrides from the file
diff --git a/lib/spack/spack/container/__init__.py b/lib/spack/spack/container/__init__.py
index e7d487cbb4..988849d5f2 100644
--- a/lib/spack/spack/container/__init__.py
+++ b/lib/spack/spack/container/__init__.py
@@ -37,7 +37,7 @@ def validate(configuration_file):
         config = syaml.load(f)
 
     # Ensure we have a "container" attribute with sensible defaults set
-    env_dict = ev.config_dict(config)
+    env_dict = config[ev.TOP_LEVEL_KEY]
     env_dict.setdefault(
         "container", {"format": "docker", "images": {"os": "ubuntu:22.04", "spack": "develop"}}
     )
diff --git a/lib/spack/spack/container/writers/__init__.py b/lib/spack/spack/container/writers/__init__.py
index 69a4b7f443..710021cb06 100644
--- a/lib/spack/spack/container/writers/__init__.py
+++ b/lib/spack/spack/container/writers/__init__.py
@@ -50,7 +50,7 @@ def create(configuration, last_phase=None):
         configuration (dict): how to generate the current recipe
         last_phase (str): last phase to be printed or None to print them all
     """
-    name = ev.config_dict(configuration)["container"]["format"]
+    name = configuration[ev.TOP_LEVEL_KEY]["container"]["format"]
     return _writer_factory[name](configuration, last_phase)
 
 
@@ -138,7 +138,7 @@ class PathContext(tengine.Context):
     template_name: Optional[str] = None
 
     def __init__(self, config, last_phase):
-        self.config = ev.config_dict(config)
+        self.config = config[ev.TOP_LEVEL_KEY]
         self.container_config = self.config["container"]
 
         # Operating system tag as written in the configuration file
diff --git a/lib/spack/spack/environment/__init__.py b/lib/spack/spack/environment/__init__.py
index d87355656b..227b48670c 100644
--- a/lib/spack/spack/environment/__init__.py
+++ b/lib/spack/spack/environment/__init__.py
@@ -337,6 +337,7 @@ the commit or version.
 """
 
 from .environment import (
+    TOP_LEVEL_KEY,
     Environment,
     SpackEnvironmentError,
     SpackEnvironmentViewError,
@@ -345,7 +346,6 @@ from .environment import (
     active_environment,
     all_environment_names,
     all_environments,
-    config_dict,
     create,
     create_in_dir,
     deactivate,
@@ -369,6 +369,7 @@ from .environment import (
 )
 
 __all__ = [
+    "TOP_LEVEL_KEY",
     "Environment",
     "SpackEnvironmentError",
     "SpackEnvironmentViewError",
@@ -377,7 +378,6 @@ __all__ = [
     "active_environment",
     "all_environment_names",
     "all_environments",
-    "config_dict",
     "create",
     "create_in_dir",
     "deactivate",
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index d2a025443b..82810e3b7d 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -53,6 +53,7 @@ import spack.util.url
 import spack.version
 from spack.filesystem_view import SimpleFilesystemView, inverse_view_func_parser, view_func_parser
 from spack.installer import PackageInstaller
+from spack.schema.env import TOP_LEVEL_KEY
 from spack.spec import Spec
 from spack.spec_list import InvalidSpecConstraintError, SpecList
 from spack.util.path import substitute_path_variables
@@ -361,19 +362,6 @@ def ensure_env_root_path_exists():
         fs.mkdirp(env_root_path())
 
 
-def config_dict(yaml_data):
-    """Get the configuration scope section out of an spack.yaml"""
-    # TODO (env:): Remove env: as a possible top level keyword in v0.21
-    key = spack.config.first_existing(yaml_data, spack.schema.env.keys)
-    if key == "env":
-        msg = (
-            "using 'env:' as a top-level attribute of a Spack environment is deprecated and "
-            "will be removed in Spack v0.21. Please use 'spack:' instead."
-        )
-        warnings.warn(msg)
-    return yaml_data[key]
-
-
 def all_environment_names():
     """List the names of environments that currently exist."""
     # just return empty if the env path does not exist.  A read-only
@@ -821,8 +809,8 @@ class Environment:
     def _construct_state_from_manifest(self):
         """Read manifest file and set up user specs."""
         self.spec_lists = collections.OrderedDict()
-
-        for item in config_dict(self.manifest).get("definitions", []):
+        env_configuration = self.manifest[TOP_LEVEL_KEY]
+        for item in env_configuration.get("definitions", []):
             entry = copy.deepcopy(item)
             when = _eval_conditional(entry.pop("when", "True"))
             assert len(entry) == 1
@@ -834,13 +822,13 @@ class Environment:
                 else:
                     self.spec_lists[name] = user_specs
 
-        spec_list = config_dict(self.manifest).get(user_speclist_name, [])
+        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()
         )
         self.spec_lists[user_speclist_name] = user_specs
 
-        enable_view = config_dict(self.manifest).get("view")
+        enable_view = env_configuration.get("view")
         # enable_view can be boolean, string, or None
         if enable_view is True or enable_view is None:
             self.views = {default_view_name: ViewDescriptor(self.path, self.view_path_default)}
@@ -855,14 +843,11 @@ class Environment:
         else:
             self.views = {}
 
-        # Retrieve the current concretization strategy
-        configuration = config_dict(self.manifest)
-
         # Retrieve unification scheme for the concretizer
         self.unify = spack.config.get("concretizer:unify", False)
 
         # Retrieve dev-build packages:
-        self.dev_specs = copy.deepcopy(configuration.get("develop", {}))
+        self.dev_specs = copy.deepcopy(env_configuration.get("develop", {}))
         for name, entry in self.dev_specs.items():
             # spec must include a concrete version
             assert Spec(entry["spec"]).versions.concrete_range_as_version
@@ -982,7 +967,7 @@ class Environment:
 
         # load config scopes added via 'include:', in reverse so that
         # highest-precedence scopes are last.
-        includes = config_dict(self.manifest).get("include", [])
+        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.
@@ -1075,10 +1060,7 @@ class Environment:
         """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,
-            [spack.config.first_existing(self.manifest, spack.schema.env.keys)],
+            config_name, self.manifest_path, spack.schema.env.schema, [TOP_LEVEL_KEY]
         )
 
     def config_scopes(self):
@@ -2684,8 +2666,8 @@ class EnvironmentManifestFile(collections.abc.Mapping):
         Args:
             user_spec: user spec to be appended
         """
-        config_dict(self.pristine_yaml_content).setdefault("specs", []).append(user_spec)
-        config_dict(self.yaml_content).setdefault("specs", []).append(user_spec)
+        self.pristine_configuration.setdefault("specs", []).append(user_spec)
+        self.configuration.setdefault("specs", []).append(user_spec)
         self.changed = True
 
     def remove_user_spec(self, user_spec: str) -> None:
@@ -2698,8 +2680,8 @@ class EnvironmentManifestFile(collections.abc.Mapping):
             SpackEnvironmentError: when the user spec is not in the list
         """
         try:
-            config_dict(self.pristine_yaml_content)["specs"].remove(user_spec)
-            config_dict(self.yaml_content)["specs"].remove(user_spec)
+            self.pristine_configuration["specs"].remove(user_spec)
+            self.configuration["specs"].remove(user_spec)
         except ValueError as e:
             msg = f"cannot remove {user_spec} from {self}, no such spec exists"
             raise SpackEnvironmentError(msg) from e
@@ -2716,8 +2698,8 @@ class EnvironmentManifestFile(collections.abc.Mapping):
             SpackEnvironmentError: when the user spec cannot be overridden
         """
         try:
-            config_dict(self.pristine_yaml_content)["specs"][idx] = user_spec
-            config_dict(self.yaml_content)["specs"][idx] = user_spec
+            self.pristine_configuration["specs"][idx] = user_spec
+            self.configuration["specs"][idx] = user_spec
         except ValueError as e:
             msg = f"cannot override {user_spec} from {self}"
             raise SpackEnvironmentError(msg) from e
@@ -2733,14 +2715,14 @@ class EnvironmentManifestFile(collections.abc.Mapping):
         Raises:
             SpackEnvironmentError: is no valid definition exists already
         """
-        defs = config_dict(self.pristine_yaml_content).get("definitions", [])
+        defs = self.pristine_configuration.get("definitions", [])
         msg = f"cannot add {user_spec} to the '{list_name}' definition, no valid list exists"
 
         for idx, item in self._iterate_on_definitions(defs, list_name=list_name, err_msg=msg):
             item[list_name].append(user_spec)
             break
 
-        config_dict(self.yaml_content)["definitions"][idx][list_name].append(user_spec)
+        self.configuration["definitions"][idx][list_name].append(user_spec)
         self.changed = True
 
     def remove_definition(self, user_spec: str, list_name: str) -> None:
@@ -2754,7 +2736,7 @@ class EnvironmentManifestFile(collections.abc.Mapping):
             SpackEnvironmentError: if the user spec cannot be removed from the list,
                 or the list does not exist
         """
-        defs = config_dict(self.pristine_yaml_content).get("definitions", [])
+        defs = self.pristine_configuration.get("definitions", [])
         msg = (
             f"cannot remove {user_spec} from the '{list_name}' definition, "
             f"no valid list exists"
@@ -2767,7 +2749,7 @@ class EnvironmentManifestFile(collections.abc.Mapping):
             except ValueError:
                 pass
 
-        config_dict(self.yaml_content)["definitions"][idx][list_name].remove(user_spec)
+        self.configuration["definitions"][idx][list_name].remove(user_spec)
         self.changed = True
 
     def override_definition(self, user_spec: str, *, override: str, list_name: str) -> None:
@@ -2782,7 +2764,7 @@ class EnvironmentManifestFile(collections.abc.Mapping):
         Raises:
             SpackEnvironmentError: if the user spec cannot be overridden
         """
-        defs = config_dict(self.pristine_yaml_content).get("definitions", [])
+        defs = self.pristine_configuration.get("definitions", [])
         msg = f"cannot override {user_spec} with {override} in the '{list_name}' definition"
 
         for idx, item in self._iterate_on_definitions(defs, list_name=list_name, err_msg=msg):
@@ -2793,7 +2775,7 @@ class EnvironmentManifestFile(collections.abc.Mapping):
             except ValueError:
                 pass
 
-        config_dict(self.yaml_content)["definitions"][idx][list_name][sub_index] = override
+        self.configuration["definitions"][idx][list_name][sub_index] = override
         self.changed = True
 
     def _iterate_on_definitions(self, definitions, *, list_name, err_msg):
@@ -2825,24 +2807,24 @@ class EnvironmentManifestFile(collections.abc.Mapping):
                 True the default view is used for the environment, if False there's no view.
         """
         if isinstance(view, dict):
-            config_dict(self.pristine_yaml_content)["view"][default_view_name].update(view)
-            config_dict(self.yaml_content)["view"][default_view_name].update(view)
+            self.pristine_configuration["view"][default_view_name].update(view)
+            self.configuration["view"][default_view_name].update(view)
             self.changed = True
             return
 
         if not isinstance(view, bool):
             view = str(view)
 
-        config_dict(self.pristine_yaml_content)["view"] = view
-        config_dict(self.yaml_content)["view"] = view
+        self.pristine_configuration["view"] = view
+        self.configuration["view"] = view
         self.changed = True
 
     def remove_default_view(self) -> None:
         """Removes the default view from the manifest file"""
-        view_data = config_dict(self.pristine_yaml_content).get("view")
+        view_data = self.pristine_configuration.get("view")
         if isinstance(view_data, collections.abc.Mapping):
-            config_dict(self.pristine_yaml_content)["view"].pop(default_view_name)
-            config_dict(self.yaml_content)["view"].pop(default_view_name)
+            self.pristine_configuration["view"].pop(default_view_name)
+            self.configuration["view"].pop(default_view_name)
             self.changed = True
             return
 
@@ -2859,12 +2841,10 @@ class EnvironmentManifestFile(collections.abc.Mapping):
         if entry["path"] == pkg_name:
             entry.pop("path")
 
-        config_dict(self.pristine_yaml_content).setdefault("develop", {}).setdefault(
-            pkg_name, {}
-        ).update(entry)
-        config_dict(self.yaml_content).setdefault("develop", {}).setdefault(pkg_name, {}).update(
+        self.pristine_configuration.setdefault("develop", {}).setdefault(pkg_name, {}).update(
             entry
         )
+        self.configuration.setdefault("develop", {}).setdefault(pkg_name, {}).update(entry)
         self.changed = True
 
     def remove_develop_spec(self, pkg_name: str) -> None:
@@ -2877,11 +2857,11 @@ class EnvironmentManifestFile(collections.abc.Mapping):
             SpackEnvironmentError: if there is nothing to remove
         """
         try:
-            del config_dict(self.pristine_yaml_content)["develop"][pkg_name]
+            del self.pristine_configuration["develop"][pkg_name]
         except KeyError as e:
             msg = f"cannot remove '{pkg_name}' from develop specs in {self}, entry does not exist"
             raise SpackEnvironmentError(msg) from e
-        del config_dict(self.yaml_content)["develop"][pkg_name]
+        del self.configuration["develop"][pkg_name]
         self.changed = True
 
     def absolutify_dev_paths(self, init_file_dir: Union[str, pathlib.Path]) -> None:
@@ -2892,11 +2872,11 @@ class EnvironmentManifestFile(collections.abc.Mapping):
             init_file_dir: directory with the "spack.yaml" used to initialize the environment.
         """
         init_file_dir = pathlib.Path(init_file_dir).absolute()
-        for _, entry in config_dict(self.pristine_yaml_content).get("develop", {}).items():
+        for _, entry in self.pristine_configuration.get("develop", {}).items():
             expanded_path = os.path.normpath(str(init_file_dir / entry["path"]))
             entry["path"] = str(expanded_path)
 
-        for _, entry in config_dict(self.yaml_content).get("develop", {}).items():
+        for _, entry in self.configuration.get("develop", {}).items():
             expanded_path = os.path.normpath(str(init_file_dir / entry["path"]))
             entry["path"] = str(expanded_path)
         self.changed = True
@@ -2910,6 +2890,16 @@ class EnvironmentManifestFile(collections.abc.Mapping):
             _write_yaml(self.pristine_yaml_content, f)
         self.changed = False
 
+    @property
+    def pristine_configuration(self):
+        """Return the dictionaries in the pristine YAML, without the top level attribute"""
+        return self.pristine_yaml_content[TOP_LEVEL_KEY]
+
+    @property
+    def configuration(self):
+        """Return the dictionaries in the YAML, without the top level attribute"""
+        return self.yaml_content[TOP_LEVEL_KEY]
+
     def __len__(self):
         return len(self.yaml_content)
 
diff --git a/lib/spack/spack/schema/ci.py b/lib/spack/spack/schema/ci.py
index 667960f372..04767d50fc 100644
--- a/lib/spack/spack/schema/ci.py
+++ b/lib/spack/spack/schema/ci.py
@@ -209,7 +209,7 @@ def update(data):
     # Warn if deprecated section is still in the environment
     ci_env = ev.active_environment()
     if ci_env:
-        env_config = ev.config_dict(ci_env.manifest)
+        env_config = ci_env.manifest[ev.TOP_LEVEL_KEY]
         if "gitlab-ci" in env_config:
             tty.die("Error: `gitlab-ci` section detected with `ci`, these are not compatible")
 
diff --git a/lib/spack/spack/schema/env.py b/lib/spack/spack/schema/env.py
index 93f0f9129f..6548ca4b2b 100644
--- a/lib/spack/spack/schema/env.py
+++ b/lib/spack/spack/schema/env.py
@@ -15,8 +15,8 @@ import spack.schema.merged
 import spack.schema.packages
 import spack.schema.projections
 
-#: legal first keys in the schema
-keys = ("spack", "env")
+#: Top level key in a manifest file
+TOP_LEVEL_KEY = "spack"
 
 spec_list_schema = {
     "type": "array",
@@ -47,8 +47,8 @@ schema = {
     "title": "Spack environment file schema",
     "type": "object",
     "additionalProperties": False,
-    "patternProperties": {
-        "^env|spack$": {
+    "properties": {
+        "spack": {
             "type": "object",
             "default": {},
             "additionalProperties": False,
diff --git a/lib/spack/spack/test/cmd/develop.py b/lib/spack/spack/test/cmd/develop.py
index 1f77bbfc63..7c0d8f5034 100644
--- a/lib/spack/spack/test/cmd/develop.py
+++ b/lib/spack/spack/test/cmd/develop.py
@@ -32,7 +32,7 @@ class TestDevelop(object):
         assert dev_specs_entry["spec"] == str(spec)
 
         # check yaml representation
-        yaml = ev.config_dict(env.manifest)
+        yaml = env.manifest[ev.TOP_LEVEL_KEY]
         assert spec.name in yaml["develop"]
         yaml_entry = yaml["develop"][spec.name]
         assert yaml_entry["spec"] == str(spec)
-- 
cgit v1.2.3-60-g2f50