From 17a9198c78a3ef014242e351ce2ba31e3dccfee7 Mon Sep 17 00:00:00 2001 From: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com> Date: Mon, 6 Nov 2023 09:48:28 -0800 Subject: [PATCH] Environments: remove environments created with SpackYAMLErrors (#40878) --- lib/spack/spack/cmd/env.py | 4 +- lib/spack/spack/environment/__init__.py | 2 + lib/spack/spack/environment/environment.py | 14 ++++- lib/spack/spack/test/cmd/env.py | 69 ++++++++++++++++------ 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 @@ from .environment import ( TOP_LEVEL_KEY, Environment, + SpackEnvironmentConfigError, SpackEnvironmentError, SpackEnvironmentViewError, activate, @@ -372,6 +373,7 @@ __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 @@ def test_env_with_included_configs_precedence(tmp_path): 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)