Environments: remove environments created with SpackYAMLErrors (#40878)

4 files changed, 68 insertions, 21 deletions

diff --git a/lib/spack/spack/cmd/env.py b/lib/spack/spack/cmd/env.py
index bf1f29d558..bb1ad13ec2 100644
--- a/lib/spack/spack/cmd/env.py
+++ b/lib/spack/spack/cmd/env.py
@@ -402,7 +402,7 @@ def env_remove(args):
         try:
             env = ev.read(env_name)
             read_envs.append(env)
-        except spack.config.ConfigFormatError:
+        except (spack.config.ConfigFormatError, ev.SpackEnvironmentConfigError):
             bad_envs.append(env_name)
 
     if not args.yes_to_all:
@@ -570,8 +570,8 @@ def env_update_setup_parser(subparser):
 def env_update(args):
     manifest_file = ev.manifest_file(args.update_env)
     backup_file = manifest_file + ".bkp"
-    needs_update = not ev.is_latest_format(manifest_file)
 
+    needs_update = not ev.is_latest_format(manifest_file)
     if not needs_update:
         tty.msg('No update needed for the environment "{0}"'.format(args.update_env))
         return
diff --git a/lib/spack/spack/environment/__init__.py b/lib/spack/spack/environment/__init__.py
index ac598e8421..2f293d9eb8 100644
--- a/lib/spack/spack/environment/__init__.py
+++ b/lib/spack/spack/environment/__init__.py
@@ -339,6 +339,7 @@ the commit or version.
 from .environment import (
     TOP_LEVEL_KEY,
     Environment,
+    SpackEnvironmentConfigError,
     SpackEnvironmentError,
     SpackEnvironmentViewError,
     activate,
@@ -372,6 +373,7 @@ from .environment import (
 __all__ = [
     "TOP_LEVEL_KEY",
     "Environment",
+    "SpackEnvironmentConfigError",
     "SpackEnvironmentError",
     "SpackEnvironmentViewError",
     "activate",
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index ab6fef6fc0..8ddd7f8d3b 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -342,7 +342,7 @@ def create_in_dir(
 
         manifest.flush()
 
-    except spack.config.ConfigFormatError as e:
+    except (spack.config.ConfigFormatError, SpackEnvironmentConfigError) as e:
         shutil.rmtree(manifest_dir)
         raise e
 
@@ -396,7 +396,13 @@ def all_environments():
 
 def _read_yaml(str_or_file):
     """Read YAML from a file for round-trip parsing."""
-    data = syaml.load_config(str_or_file)
+    try:
+        data = syaml.load_config(str_or_file)
+    except syaml.SpackYAMLError as e:
+        raise SpackEnvironmentConfigError(
+            f"Invalid environment configuration detected: {e.message}"
+        )
+
     filename = getattr(str_or_file, "name", None)
     default_data = spack.config.validate(data, spack.schema.env.schema, filename)
     return data, default_data
@@ -2960,3 +2966,7 @@ class SpackEnvironmentError(spack.error.SpackError):
 
 class SpackEnvironmentViewError(SpackEnvironmentError):
     """Class for errors regarding view generation."""
+
+
+class SpackEnvironmentConfigError(SpackEnvironmentError):
+    """Class for Spack environment-specific configuration errors."""
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index e291432a0f..a06fdbd8cf 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -1006,21 +1006,7 @@ packages:
     assert any(x.satisfies("libelf@0.8.10") for x in specs)
 
 
-def test_bad_env_yaml_format(environment_from_manifest):
-    with pytest.raises(spack.config.ConfigFormatError) as e:
-        environment_from_manifest(
-            """\
-spack:
-  spacks:
-    - mpileaks
-"""
-        )
-
-        assert "'spacks' was unexpected" in str(e)
-
-    assert "test" not in env("list")
-
-
+@pytest.mark.regression("39248")
 def test_bad_env_yaml_format_remove(mutable_mock_env_path):
     badenv = "badenv"
     env("create", badenv)
@@ -1037,6 +1023,55 @@ def test_bad_env_yaml_format_remove(mutable_mock_env_path):
     assert badenv not in env("list")
 
 
+@pytest.mark.regression("39248")
+@pytest.mark.parametrize(
+    "error,message,contents",
+    [
+        (
+            spack.config.ConfigFormatError,
+            "not of type",
+            """\
+spack:
+  specs: mpi@2.0
+""",
+        ),
+        (
+            ev.SpackEnvironmentConfigError,
+            "duplicate key",
+            """\
+spack:
+  packages:
+    all:
+      providers:
+        mpi: [mvapich2]
+        mpi: [mpich]
+""",
+        ),
+        (
+            spack.config.ConfigFormatError,
+            "'specks' was unexpected",
+            """\
+spack:
+  specks:
+    - libdwarf
+""",
+        ),
+    ],
+)
+def test_bad_env_yaml_create_fails(tmp_path, mutable_mock_env_path, error, message, contents):
+    """Ensure creation with invalid yaml does NOT create or leave the environment."""
+    filename = tmp_path / ev.manifest_name
+    filename.write_text(contents)
+    env_name = "bad_env"
+    with pytest.raises(error, match=message):
+        env("create", env_name, str(filename))
+
+    assert env_name not in env("list")
+    manifest = mutable_mock_env_path / env_name / ev.manifest_name
+    assert not os.path.exists(str(manifest))
+
+
+@pytest.mark.regression("39248")
 @pytest.mark.parametrize("answer", ["-y", ""])
 def test_multi_env_remove(mutable_mock_env_path, monkeypatch, answer):
     """Test removal (or not) of a valid and invalid environment"""
@@ -1048,7 +1083,7 @@ def test_multi_env_remove(mutable_mock_env_path, monkeypatch, answer):
         env("create", e)
 
     # Ensure the bad environment contains invalid yaml
-    filename = mutable_mock_env_path / environments[1] / "spack.yaml"
+    filename = mutable_mock_env_path / environments[1] / ev.manifest_name
     filename.write_text(
         """\
     - libdwarf
@@ -1064,7 +1099,7 @@ def test_multi_env_remove(mutable_mock_env_path, monkeypatch, answer):
     if remove_environment is True:
         # Successfully removed (and reported removal) of *both* environments
         assert not all(e in env("list") for e in environments)
-        assert output.count("Successfully removed") == 2
+        assert output.count("Successfully removed") == len(environments)
     else:
         # Not removing any of the environments
         assert all(e in env("list") for e in environments)