Factor YAML manifest manipulation out of the Environment class (#36927)

Change the signature of the Environment.__init__ method to have
a single argument, i.e. the directory where the environment manifest 
is located. Initializing that directory is now delegated to a function 
taking care of all the error handling upfront. Environment objects 
require a "spack.yaml" to be available to be constructed.

Add a class to manage the environment manifest file. The environment 
now delegates to an attribute of that class the responsibility of keeping
track of changes modifying the manifest. This allows simplifying the 
updates of the manifest file, and helps keeping in sync the spec lists in
memory with the spack.yaml on disk.

lib/spack/spack/ci.py

@@ -750,7 +750,7 @@ def generate_gitlab_ci_yaml(
             env.concretize()
             env.write()
 
-    yaml_root = ev.config_dict(env.yaml)
+    yaml_root = ev.config_dict(env.manifest)
 
     # Get the joined "ci" config with all of the current scopes resolved
     ci_config = cfg.get("ci")

lib/spack/spack/cmd/ci.py

@@ -227,7 +227,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.yaml)
+    yaml_root = ev.config_dict(env.manifest)
 
     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")

lib/spack/spack/cmd/env.py

@@ -163,7 +163,7 @@ def env_activate(args):
         env = create_temp_env_directory()
         env_path = os.path.abspath(env)
         short_name = os.path.basename(env_path)
-        ev.Environment(env).write(regenerate=False)
+        ev.create_in_dir(env).write(regenerate=False)
 
     # Managed environment
     elif ev.exists(env_name_or_dir) and not args.dir:
@@ -301,16 +301,17 @@ def env_create(args):
         # object could choose to enable a view by default. False means that
         # the environment should not include a view.
         with_view = None
-    if args.envfile:
+
-        with open(args.envfile) as f:
+    _env_create(
-            _env_create(
+        args.create_env,
-                args.create_env, f, args.dir, with_view=with_view, keep_relative=args.keep_relative
+        init_file=args.envfile,
-            )
+        dir=args.dir,
-    else:
+        with_view=with_view,
-        _env_create(args.create_env, None, args.dir, with_view=with_view)
+        keep_relative=args.keep_relative,
+    )
 
 
-def _env_create(name_or_path, init_file=None, dir=False, with_view=None, keep_relative=False):
+def _env_create(name_or_path, *, init_file=None, dir=False, with_view=None, keep_relative=False):
     """Create a new environment, with an optional yaml description.
 
     Arguments:
@@ -323,18 +324,21 @@ def _env_create(name_or_path, init_file=None, dir=False, with_view=None, keep_re
             the new environment file, otherwise they may be made absolute if the
             new environment is in a different location
     """
-    if dir:
+    if not dir:
-        env = ev.Environment(name_or_path, init_file, with_view, keep_relative)
+        env = ev.create(
-        env.write()
+            name_or_path, init_file=init_file, with_view=with_view, keep_relative=keep_relative
-        tty.msg("Created environment in %s" % env.path)
+        )
-        tty.msg("You can activate this environment with:")
-        tty.msg("  spack env activate %s" % env.path)
-    else:
-        env = ev.create(name_or_path, init_file, with_view, keep_relative)
-        env.write()
         tty.msg("Created environment '%s' in %s" % (name_or_path, env.path))
         tty.msg("You can activate this environment with:")
         tty.msg("  spack env activate %s" % (name_or_path))
+        return env
+
+    env = ev.create_in_dir(
+        name_or_path, init_file=init_file, with_view=with_view, keep_relative=keep_relative
+    )
+    tty.msg("Created environment in %s" % env.path)
+    tty.msg("You can activate this environment with:")
+    tty.msg("  spack env activate %s" % env.path)
     return env
 
 
@@ -431,21 +435,22 @@ def env_view_setup_parser(subparser):
 def env_view(args):
     env = ev.active_environment()
 
-    if env:
+    if not env:
-        if args.action == ViewAction.regenerate:
-            env.regenerate_views()
-        elif args.action == ViewAction.enable:
-            if args.view_path:
-                view_path = args.view_path
-            else:
-                view_path = env.view_path_default
-            env.update_default_view(view_path)
-            env.write()
-        elif args.action == ViewAction.disable:
-            env.update_default_view(None)
-            env.write()
-    else:
         tty.msg("No active environment")
+        return
+
+    if args.action == ViewAction.regenerate:
+        env.regenerate_views()
+    elif args.action == ViewAction.enable:
+        if args.view_path:
+            view_path = args.view_path
+        else:
+            view_path = env.view_path_default
+        env.update_default_view(view_path)
+        env.write()
+    elif args.action == ViewAction.disable:
+        env.update_default_view(path_or_bool=False)
+        env.write()
 
 
 #

lib/spack/spack/cmd/remove.py

@@ -38,6 +38,6 @@ def remove(parser, args):
             env.clear()
         else:
             for spec in spack.cmd.parse_specs(args.specs):
-                tty.msg("Removing %s from environment %s" % (spec, env.name))
                 env.remove(spec, args.list_name, force=args.force)
+                tty.msg(f"{spec} has been removed from {env.manifest}")
         env.write()

lib/spack/spack/environment/__init__.py

@@ -340,11 +340,14 @@
     all_environments,
     config_dict,
     create,
+    create_in_dir,
     deactivate,
     default_manifest_yaml,
     default_view_name,
     display_specs,
+    environment_dir_from_name,
     exists,
+    initialize_environment_dir,
     installed_specs,
     is_env_dir,
     is_latest_format,
@@ -369,11 +372,14 @@
     "all_environments",
     "config_dict",
     "create",
+    "create_in_dir",
     "deactivate",
     "default_manifest_yaml",
     "default_view_name",
     "display_specs",
+    "environment_dir_from_name",
     "exists",
+    "initialize_environment_dir",
     "installed_specs",
     "is_env_dir",
     "is_latest_format",

lib/spack/spack/schema/ci.py

@@ -204,7 +204,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.yaml)
+        env_config = ev.config_dict(ci_env.manifest)
         if "gitlab-ci" in env_config:
             tty.die("Error: `gitlab-ci` section detected with `ci`, these are not compatible")
 

lib/spack/spack/test/cmd/config.py

@@ -91,17 +91,10 @@ def test_config_edit(mutable_config, working_env):
 
 
 def test_config_get_gets_spack_yaml(mutable_mock_env_path):
-    env = ev.create("test")
-
     config("get", fail_on_error=False)
     assert config.returncode == 1
 
-    with env:
+    with ev.create("test") as env:
-        config("get", fail_on_error=False)
-        assert config.returncode == 1
-
-        env.write()
-
         assert "mpileaks" not in config("get")
 
         env.add("mpileaks")
@@ -671,4 +664,4 @@ def update_config(data):
         config("update", "-y", "config")
 
     with ev.Environment(str(tmpdir)) as e:
-        assert not e.raw_yaml["spack"]["config"]["ccache"]
+        assert not e.manifest.pristine_yaml_content["spack"]["config"]["ccache"]

lib/spack/spack/test/cmd/develop.py

@@ -32,7 +32,7 @@ def check_develop(self, env, spec, path=None):
         assert dev_specs_entry["spec"] == str(spec)
 
         # check yaml representation
-        yaml = ev.config_dict(env.yaml)
+        yaml = ev.config_dict(env.manifest)
         assert spec.name in yaml["develop"]
         yaml_entry = yaml["develop"][spec.name]
         assert yaml_entry["spec"] == str(spec)

lib/spack/spack/test/cmd/env.py

@@ -6,6 +6,7 @@
 import glob
 import io
 import os
+import pathlib
 import shutil
 import sys
 from argparse import Namespace
@@ -18,6 +19,7 @@
 import spack.cmd.env
 import spack.config
 import spack.environment as ev
+import spack.environment.environment
 import spack.environment.shell
 import spack.error
 import spack.modules
@@ -53,6 +55,18 @@
 sep = os.sep
 
 
+@pytest.fixture()
+def environment_from_manifest(tmp_path):
+    """Returns a new environment named 'test' from the content of a manifest file."""
+
+    def _create(content):
+        spack_yaml = tmp_path / ev.manifest_name
+        spack_yaml.write_text(content)
+        return _env_create("test", init_file=str(spack_yaml))
+
+    return _create
+
+
 def check_mpileaks_and_deps_in_view(viewdir):
     """Check that the expected install directories exist."""
     assert os.path.exists(str(viewdir.join(".spack", "mpileaks")))
@@ -427,11 +441,11 @@ def test_environment_status(capsys, tmpdir):
             with capsys.disabled():
                 assert "In environment test" in env("status")
 
-        with ev.Environment("local_dir"):
+        with ev.create_in_dir("local_dir"):
             with capsys.disabled():
                 assert os.path.join(os.getcwd(), "local_dir") in env("status")
 
-            e = ev.Environment("myproject")
+            e = ev.create_in_dir("myproject")
             e.write()
             with tmpdir.join("myproject").as_cwd():
                 with e:
@@ -445,21 +459,20 @@ def test_env_status_broken_view(
     mock_fetch,
     mock_custom_repository,
     install_mockery,
-    tmpdir,
+    tmp_path,
 ):
-    env_dir = str(tmpdir)
+    with ev.create_in_dir(tmp_path):
-    with ev.Environment(env_dir):
         install("--add", "trivial-install-test-package")
 
     # switch to a new repo that doesn't include the installed package
     # test that Spack detects the missing package and warns the user
     with spack.repo.use_repositories(mock_custom_repository):
-        with ev.Environment(env_dir):
+        with ev.Environment(tmp_path):
             output = env("status")
             assert "includes out of date packages or repos" in output
 
     # Test that the warning goes away when it's fixed
-    with ev.Environment(env_dir):
+    with ev.Environment(tmp_path):
         output = env("status")
         assert "includes out of date packages or repos" not in output
 
@@ -505,9 +518,9 @@ def test_env_repo():
     assert pkg_cls.namespace == "builtin.mock"
 
 
-def test_user_removed_spec():
+def test_user_removed_spec(environment_from_manifest):
     """Ensure a user can remove from any position in the spack.yaml file."""
-    initial_yaml = io.StringIO(
+    before = environment_from_manifest(
         """\
 env:
   specs:
@@ -516,8 +529,6 @@ def test_user_removed_spec():
   - libelf
 """
     )
-
-    before = ev.create("test", initial_yaml)
     before.concretize()
     before.write()
 
@@ -536,17 +547,16 @@ def test_user_removed_spec():
     after.concretize()
     after.write()
 
-    env_specs = after._get_environment_specs()
     read = ev.read("test")
     env_specs = read._get_environment_specs()
 
     assert not any(x.name == "hypre" for x in env_specs)
 
 
-def test_init_from_lockfile(tmpdir):
+def test_init_from_lockfile(environment_from_manifest):
     """Test that an environment can be instantiated from a lockfile."""
-    initial_yaml = io.StringIO(
+    e1 = environment_from_manifest(
-        """\
+        """
 env:
   specs:
   - mpileaks
@@ -554,11 +564,10 @@ def test_init_from_lockfile(tmpdir):
   - libelf
 """
     )
-    e1 = ev.create("test", initial_yaml)
     e1.concretize()
     e1.write()
 
-    e2 = ev.Environment(str(tmpdir), e1.lock_path)
+    e2 = _env_create("test2", init_file=e1.lock_path)
 
     for s1, s2 in zip(e1.user_specs, e2.user_specs):
         assert s1 == s2
@@ -571,10 +580,10 @@ def test_init_from_lockfile(tmpdir):
         assert s1 == s2
 
 
-def test_init_from_yaml(tmpdir):
+def test_init_from_yaml(environment_from_manifest):
     """Test that an environment can be instantiated from a lockfile."""
-    initial_yaml = io.StringIO(
+    e1 = environment_from_manifest(
-        """\
+        """
 env:
   specs:
   - mpileaks
@@ -582,11 +591,10 @@ def test_init_from_yaml(tmpdir):
   - libelf
 """
     )
-    e1 = ev.create("test", initial_yaml)
     e1.concretize()
     e1.write()
 
-    e2 = ev.Environment(str(tmpdir), e1.manifest_path)
+    e2 = _env_create("test2", init_file=e1.manifest_path)
 
     for s1, s2 in zip(e1.user_specs, e2.user_specs):
         assert s1 == s2
@@ -597,13 +605,16 @@ def test_init_from_yaml(tmpdir):
 
 
 @pytest.mark.usefixtures("config")
-def test_env_view_external_prefix(tmpdir_factory, mutable_database, mock_packages):
+def test_env_view_external_prefix(tmp_path, mutable_database, mock_packages):
-    fake_prefix = tmpdir_factory.mktemp("a-prefix")
+    fake_prefix = tmp_path / "a-prefix"
-    fake_bin = fake_prefix.join("bin")
+    fake_bin = fake_prefix / "bin"
-    fake_bin.ensure(dir=True)
+    fake_bin.mkdir(parents=True, exist_ok=False)
 
-    initial_yaml = io.StringIO(
+    manifest_dir = tmp_path / "environment"
-        """\
+    manifest_dir.mkdir(parents=True, exist_ok=False)
+    manifest_file = manifest_dir / ev.manifest_name
+    manifest_file.write_text(
+        """
 env:
   specs:
   - a
@@ -627,7 +638,7 @@ def test_env_view_external_prefix(tmpdir_factory, mutable_database, mock_package
 
     test_scope = spack.config.InternalConfigScope("env-external-test", data=external_config_dict)
     with spack.config.override(test_scope):
-        e = ev.create("test", initial_yaml)
+        e = ev.create("test", manifest_file)
         e.concretize()
         # Note: normally installing specs in a test environment requires doing
         # a fake install, but not for external specs since no actions are
@@ -672,8 +683,9 @@ def test_init_with_file_and_remove(tmpdir):
     assert "test" not in out
 
 
-def test_env_with_config():
+def test_env_with_config(environment_from_manifest):
-    test_config = """\
+    e = environment_from_manifest(
+        """
 env:
   specs:
   - mpileaks
@@ -681,26 +693,22 @@ def test_env_with_config():
     mpileaks:
       version: [2.2]
 """
-    _env_create("test", io.StringIO(test_config))
+    )
-
-    e = ev.read("test")
     with e:
         e.concretize()
 
     assert any(x.intersects("mpileaks@2.2") for x in e._get_environment_specs())
 
 
-def test_with_config_bad_include():
+def test_with_config_bad_include(environment_from_manifest):
-    env_name = "test_bad_include"
+    e = environment_from_manifest(
-    test_config = """\
+        """
 spack:
   include:
   - /no/such/directory
   - no/such/file.yaml
 """
-    _env_create(env_name, io.StringIO(test_config))
+    )
-
-    e = ev.read(env_name)
     with pytest.raises(spack.config.ConfigFileError) as exc:
         with e:
             e.concretize()
@@ -712,17 +720,19 @@ def test_with_config_bad_include():
     assert ev.active_environment() is None
 
 
-def test_env_with_include_config_files_same_basename():
+def test_env_with_include_config_files_same_basename(environment_from_manifest):
-    test_config = """\
+    e = environment_from_manifest(
-        env:
+        """
-            include:
+env:
-                - ./path/to/included-config.yaml
+  include:
-                - ./second/path/to/include-config.yaml
+  - ./path/to/included-config.yaml
-            specs:
+  - ./second/path/to/include-config.yaml
-                [libelf, mpileaks]
+  specs:
-            """
+  - libelf
+  - mpileaks
+"""
+    )
 
-    _env_create("test", io.StringIO(test_config))
     e = ev.read("test")
 
     fs.mkdirp(os.path.join(e.path, "path", "to"))
@@ -781,14 +791,20 @@ def mpileaks_env_config(include_path):
     )
 
 
-def test_env_with_included_config_file(packages_file):
+def test_env_with_included_config_file(environment_from_manifest, packages_file):
     """Test inclusion of a relative packages configuration file added to an
-    existing environment."""
+    existing environment.
+    """
     include_filename = "included-config.yaml"
-    test_config = mpileaks_env_config(os.path.join(".", include_filename))
+    e = environment_from_manifest(
-
+        f"""\
-    _env_create("test", io.StringIO(test_config))
+env:
-    e = ev.read("test")
+  include:
+  - {os.path.join(".", include_filename)}
+  specs:
+  - mpileaks
+"""
+    )
 
     included_path = os.path.join(e.path, include_filename)
     shutil.move(packages_file.strpath, included_path)
@@ -830,7 +846,7 @@ def test_env_with_included_config_missing_file(tmpdir, mutable_empty_config):
         ev.activate(env)
 
 
-def test_env_with_included_config_scope(tmpdir, packages_file):
+def test_env_with_included_config_scope(environment_from_manifest, packages_file):
     """Test inclusion of a package file from the environment's configuration
     stage directory. This test is intended to represent a case where a remote
     file has already been staged."""
@@ -838,15 +854,10 @@ def test_env_with_included_config_scope(tmpdir, packages_file):
 
     # Configure the environment to include file(s) from the environment's
     # remote configuration stage directory.
-    test_config = mpileaks_env_config(config_scope_path)
+    e = environment_from_manifest(mpileaks_env_config(config_scope_path))
-
-    # Create the environment
-    _env_create("test", io.StringIO(test_config))
-
-    e = ev.read("test")
 
     # Copy the packages.yaml file to the environment configuration
-    # directory so it is picked up during concretization. (Using
+    # directory, so it is picked up during concretization. (Using
     # copy instead of rename in case the fixture scope changes.)
     fs.mkdirp(config_scope_path)
     include_filename = os.path.basename(packages_file.strpath)
@@ -861,14 +872,11 @@ def test_env_with_included_config_scope(tmpdir, packages_file):
     assert any(x.satisfies("mpileaks@2.2") for x in e._get_environment_specs())
 
 
-def test_env_with_included_config_var_path(packages_file):
+def test_env_with_included_config_var_path(environment_from_manifest, packages_file):
     """Test inclusion of a package configuration file with path variables
     "staged" in the environment's configuration stage directory."""
     config_var_path = os.path.join("$tempdir", "included-config.yaml")
-    test_config = mpileaks_env_config(config_var_path)
+    e = environment_from_manifest(mpileaks_env_config(config_var_path))
-
-    _env_create("test", io.StringIO(test_config))
-    e = ev.read("test")
 
     config_real_path = substitute_path_variables(config_var_path)
     fs.mkdirp(os.path.dirname(config_real_path))
@@ -881,8 +889,9 @@ def test_env_with_included_config_var_path(packages_file):
     assert any(x.satisfies("mpileaks@2.2") for x in e._get_environment_specs())
 
 
-def test_env_config_precedence():
+def test_env_config_precedence(environment_from_manifest):
-    test_config = """\
+    e = environment_from_manifest(
+        """
 env:
   packages:
     libelf:
@@ -892,9 +901,7 @@ def test_env_config_precedence():
   specs:
   - mpileaks
 """
-    _env_create("test", io.StringIO(test_config))
+    )
-    e = ev.read("test")
-
     with open(os.path.join(e.path, "included-config.yaml"), "w") as f:
         f.write(
             """\
@@ -916,8 +923,9 @@ def test_env_config_precedence():
     assert any(x.satisfies("libelf@0.8.12") for x in e._get_environment_specs())
 
 
-def test_included_config_precedence():
+def test_included_config_precedence(environment_from_manifest):
-    test_config = """\
+    e = environment_from_manifest(
+        """
 env:
   include:
   - ./high-config.yaml  # this one should take precedence
@@ -925,8 +933,7 @@ def test_included_config_precedence():
   specs:
   - mpileaks
 """
-    _env_create("test", io.StringIO(test_config))
+    )
-    e = ev.read("test")
 
     with open(os.path.join(e.path, "high-config.yaml"), "w") as f:
         f.write(
@@ -970,7 +977,7 @@ def test_bad_env_yaml_format(tmpdir):
     with tmpdir.as_cwd():
         with pytest.raises(spack.config.ConfigFormatError) as e:
             env("create", "test", "./spack.yaml")
-        assert "./spack.yaml:2" in str(e)
+        assert "spack.yaml:2" in str(e)
         assert "'spacks' was unexpected" in str(e)
 
 
@@ -1255,14 +1262,17 @@ def test_env_without_view_install(tmpdir, mock_stage, mock_fetch, install_mocker
     check_mpileaks_and_deps_in_view(view_dir)
 
 
-def test_env_config_view_default(tmpdir, mock_stage, mock_fetch, install_mockery):
+def test_env_config_view_default(
+    environment_from_manifest, mock_stage, mock_fetch, install_mockery
+):
     # This config doesn't mention whether a view is enabled
-    test_config = """\
+    environment_from_manifest(
+        """
 env:
   specs:
   - mpileaks
 """
-    _env_create("test", io.StringIO(test_config))
+    )
 
     with ev.read("test"):
         install("--fake")
@@ -1879,7 +1889,9 @@ def test_stack_definition_conditional_add_write(tmpdir):
 
         test = ev.read("test")
 
-        packages_lists = list(filter(lambda x: "packages" in x, test.yaml["env"]["definitions"]))
+        packages_lists = list(
+            filter(lambda x: "packages" in x, test.manifest["env"]["definitions"])
+        )
 
         assert len(packages_lists) == 2
         assert "callpath" not in packages_lists[0]["packages"]
@@ -2557,7 +2569,9 @@ def test_lockfile_not_deleted_on_write_error(tmpdir, monkeypatch):
     def _write_helper_raise(self):
         raise RuntimeError("some error")
 
-    monkeypatch.setattr(ev.Environment, "update_manifest", _write_helper_raise)
+    monkeypatch.setattr(
+        spack.environment.environment.EnvironmentManifestFile, "flush", _write_helper_raise
+    )
     with ev.Environment(str(tmpdir)) as e:
         e.concretize(force=True)
         with pytest.raises(RuntimeError):
@@ -2615,25 +2629,6 @@ def test_rewrite_rel_dev_path_named_env(tmpdir):
         assert e.dev_specs["mypkg2"]["path"] == sep + os.path.join("some", "other", "path")
 
 
-def test_rewrite_rel_dev_path_original_dir(tmpdir):
-    """Relative devevelop paths should not be rewritten when initializing an
-    environment with root path set to the same directory"""
-    init_env, _, _, spack_yaml = _setup_develop_packages(tmpdir)
-    with ev.Environment(str(init_env), str(spack_yaml)) as e:
-        assert e.dev_specs["mypkg1"]["path"] == "../build_folder"
-        assert e.dev_specs["mypkg2"]["path"] == "/some/other/path"
-
-
-def test_rewrite_rel_dev_path_create_original_dir(tmpdir):
-    """Relative develop paths should not be rewritten when creating an
-    environment in the original directory"""
-    init_env, _, _, spack_yaml = _setup_develop_packages(tmpdir)
-    env("create", "-d", str(init_env), str(spack_yaml))
-    with ev.Environment(str(init_env)) as e:
-        assert e.dev_specs["mypkg1"]["path"] == "../build_folder"
-        assert e.dev_specs["mypkg2"]["path"] == "/some/other/path"
-
-
 def test_does_not_rewrite_rel_dev_path_when_keep_relative_is_set(tmpdir):
     """Relative develop paths should not be rewritten when --keep-relative is
     passed to create"""
@@ -2659,8 +2654,9 @@ def test_custom_version_concretize_together(tmpdir):
     assert any("hdf5@myversion" in spec for _, spec in e.concretized_specs())
 
 
-def test_modules_relative_to_views(tmpdir, install_mockery, mock_fetch):
+def test_modules_relative_to_views(environment_from_manifest, install_mockery, mock_fetch):
-    spack_yaml = """
+    environment_from_manifest(
+        """
 spack:
   specs:
   - trivial-install-test-package
@@ -2671,7 +2667,7 @@ def test_modules_relative_to_views(tmpdir, install_mockery, mock_fetch):
       roots:
         tcl: modules
 """
-    _env_create("test", io.StringIO(spack_yaml))
+    )
 
     with ev.read("test") as e:
         install()
@@ -2690,8 +2686,9 @@ def test_modules_relative_to_views(tmpdir, install_mockery, mock_fetch):
     assert spec.prefix not in contents
 
 
-def test_multiple_modules_post_env_hook(tmpdir, install_mockery, mock_fetch):
+def test_multiple_modules_post_env_hook(environment_from_manifest, install_mockery, mock_fetch):
-    spack_yaml = """
+    environment_from_manifest(
+        """
 spack:
   specs:
   - trivial-install-test-package
@@ -2706,7 +2703,7 @@ def test_multiple_modules_post_env_hook(tmpdir, install_mockery, mock_fetch):
       roots:
         tcl: full_modules
 """
-    _env_create("test", io.StringIO(spack_yaml))
+    )
 
     with ev.read("test") as e:
         install()
@@ -2818,17 +2815,17 @@ def test_env_view_fail_if_symlink_points_elsewhere(tmpdir, install_mockery, mock
     assert os.path.isdir(non_view_dir)
 
 
-def test_failed_view_cleanup(tmpdir, mock_stage, mock_fetch, install_mockery):
+def test_failed_view_cleanup(tmp_path, mock_stage, mock_fetch, install_mockery):
     """Tests whether Spack cleans up after itself when a view fails to create"""
-    view = str(tmpdir.join("view"))
+    view_dir = tmp_path / "view"
-    with ev.create("env", with_view=view):
+    with ev.create("env", with_view=str(view_dir)):
         add("libelf")
         install("--fake")
 
     # Save the current view directory.
-    resolved_view = os.path.realpath(view)
+    resolved_view = view_dir.resolve(strict=True)
-    all_views = os.path.dirname(resolved_view)
+    all_views = resolved_view.parent
-    views_before = os.listdir(all_views)
+    views_before = list(all_views.iterdir())
 
     # Add a spec that results in view clash when creating a view
     with ev.read("env"):
@@ -2838,9 +2835,9 @@ def test_failed_view_cleanup(tmpdir, mock_stage, mock_fetch, install_mockery):
 
     # Make sure there is no broken view in the views directory, and the current
     # view is the original view from before the failed regenerate attempt.
-    views_after = os.listdir(all_views)
+    views_after = list(all_views.iterdir())
     assert views_before == views_after
-    assert os.path.samefile(resolved_view, view)
+    assert view_dir.samefile(resolved_view), view_dir
 
 
 def test_environment_view_target_already_exists(tmpdir, mock_stage, mock_fetch, install_mockery):
@@ -2941,9 +2938,9 @@ def test_read_old_lock_and_write_new(config, tmpdir, lockfile):
                 shadowed_hash = dag_hash
 
     # make an env out of the old lockfile -- env should be able to read v1/v2/v3
-    test_lockfile_path = str(tmpdir.join("test.lock"))
+    test_lockfile_path = str(tmpdir.join("spack.lock"))
     shutil.copy(lockfile_path, test_lockfile_path)
-    _env_create("test", test_lockfile_path, with_view=False)
+    _env_create("test", init_file=test_lockfile_path, with_view=False)
 
     # re-read the old env as a new lockfile
     e = ev.read("test")
@@ -2958,24 +2955,24 @@ def test_read_old_lock_and_write_new(config, tmpdir, lockfile):
     assert old_hashes == hashes
 
 
-def test_read_v1_lock_creates_backup(config, tmpdir):
+def test_read_v1_lock_creates_backup(config, tmp_path):
     """When reading a version-1 lockfile, make sure that a backup of that file
     is created.
     """
-    # read in the JSON from a legacy v1 lockfile
+    v1_lockfile_path = pathlib.Path(spack.paths.test_path) / "data" / "legacy_env" / "v1.lock"
-    v1_lockfile_path = os.path.join(spack.paths.test_path, "data", "legacy_env", "v1.lock")
+    test_lockfile_path = tmp_path / "init" / ev.lockfile_name
-
+    test_lockfile_path.parent.mkdir(parents=True, exist_ok=False)
-    # make an env out of the old lockfile
-    test_lockfile_path = str(tmpdir.join(ev.lockfile_name))
     shutil.copy(v1_lockfile_path, test_lockfile_path)
 
-    e = ev.Environment(str(tmpdir))
+    e = ev.create_in_dir(tmp_path, init_file=test_lockfile_path)
     assert os.path.exists(e._lock_backup_v1_path)
     assert filecmp.cmp(e._lock_backup_v1_path, v1_lockfile_path)
 
 
 @pytest.mark.parametrize("lockfile", ["v1", "v2", "v3"])
-def test_read_legacy_lockfile_and_reconcretize(mock_stage, mock_fetch, install_mockery, lockfile):
+def test_read_legacy_lockfile_and_reconcretize(
+    mock_stage, mock_fetch, install_mockery, lockfile, tmp_path
+):
     # In legacy lockfiles v2 and v3 (keyed by build hash), there may be multiple
     # versions of the same spec with different build dependencies, which means
     # they will have different build hashes but the same DAG hash.
@@ -2985,9 +2982,10 @@ def test_read_legacy_lockfile_and_reconcretize(mock_stage, mock_fetch, install_m
     # After reconcretization with the *new*, finer-grained DAG hash, there should no
     # longer be conflicts, and the previously conflicting specs can coexist in the
     # same environment.
-    legacy_lockfile_path = os.path.join(
+    test_path = pathlib.Path(spack.paths.test_path)
-        spack.paths.test_path, "data", "legacy_env", "%s.lock" % lockfile
+    lockfile_content = test_path / "data" / "legacy_env" / f"{lockfile}.lock"
-    )
+    legacy_lockfile_path = tmp_path / ev.lockfile_name
+    shutil.copy(lockfile_content, legacy_lockfile_path)
 
     # The order of the root specs in this environment is:
     #     [
@@ -2997,7 +2995,7 @@ def test_read_legacy_lockfile_and_reconcretize(mock_stage, mock_fetch, install_m
     # So in v2 and v3 lockfiles we have two versions of dttop with the same DAG
     # hash but different build hashes.
 
-    env("create", "test", legacy_lockfile_path)
+    env("create", "test", str(legacy_lockfile_path))
     test = ev.read("test")
     assert len(test.specs_by_hash) == 1
 
@@ -3154,7 +3152,7 @@ def test_depfile_phony_convenience_targets(
     each package if "--make-prefix" is absent."""
     make = Executable("make")
     with fs.working_dir(str(tmpdir)):
-        with ev.Environment("."):
+        with ev.create_in_dir("."):
             add("dttop")
             concretize()
 
@@ -3277,10 +3275,11 @@ def test_env_include_packages_url(
         assert "openmpi" in cfg["all"]["providers"]["mpi"]
 
 
-def test_relative_view_path_on_command_line_is_made_absolute(tmpdir, config):
+def test_relative_view_path_on_command_line_is_made_absolute(tmp_path, config):
-    with fs.working_dir(str(tmpdir)):
+    with fs.working_dir(str(tmp_path)):
         env("create", "--with-view", "view", "--dir", "env")
         environment = ev.Environment(os.path.join(".", "env"))
+        environment.regenerate_views()
         assert os.path.samefile("view", environment.default_view.root)
 
 

lib/spack/spack/test/cmd/install.py

@@ -799,6 +799,7 @@ def test_install_no_add_in_env(tmpdir, mock_fetch, install_mockery, mutable_mock
     e.add("a")
     e.add("a ~bvv")
     e.concretize()
+    e.write()
     env_specs = e.all_specs()
 
     a_spec = None

lib/spack/spack/test/env.py

@@ -3,8 +3,8 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 """Test environment internals without CLI"""
-import io
 import os
+import pickle
 import sys
 
 import pytest
@@ -13,16 +13,28 @@
 
 import spack.environment as ev
 import spack.spec
+from spack.environment.environment import SpackEnvironmentViewError, _error_on_nonempty_view_dir
 
 pytestmark = pytest.mark.skipif(
     sys.platform == "win32", reason="Envs are not supported on windows"
 )
 
 
-def test_hash_change_no_rehash_concrete(tmpdir, mock_packages, config):
+class TestDirectoryInitialization:
+    def test_environment_dir_from_name(self, mutable_mock_env_path):
+        """Test the function mapping a managed environment name to its folder."""
+        env = ev.create("test")
+        environment_dir = ev.environment_dir_from_name("test")
+        assert env.path == environment_dir
+        with pytest.raises(ev.SpackEnvironmentError, match="environment already exists"):
+            ev.environment_dir_from_name("test", exists_ok=False)
+
+
+def test_hash_change_no_rehash_concrete(tmp_path, mock_packages, config):
     # create an environment
-    env_path = tmpdir.mkdir("env_dir").strpath
+    env_path = tmp_path / "env_dir"
-    env = ev.Environment(env_path)
+    env_path.mkdir(exist_ok=False)
+    env = ev.create_in_dir(env_path)
     env.write()
 
     # add a spec with a rewritten build hash
@@ -48,9 +60,10 @@ def test_hash_change_no_rehash_concrete(tmpdir, mock_packages, config):
     assert read_in.specs_by_hash[read_in.concretized_order[0]]._hash == new_hash
 
 
-def test_env_change_spec(tmpdir, mock_packages, config):
+def test_env_change_spec(tmp_path, mock_packages, config):
-    env_path = tmpdir.mkdir("env_dir").strpath
+    env_path = tmp_path / "env_dir"
-    env = ev.Environment(env_path)
+    env_path.mkdir(exist_ok=False)
+    env = ev.create_in_dir(env_path)
     env.write()
 
     spec = spack.spec.Spec("mpileaks@2.1~shared+debug")
@@ -80,9 +93,10 @@ def test_env_change_spec(tmpdir, mock_packages, config):
 """
 
 
-def test_env_change_spec_in_definition(tmpdir, mock_packages, config, mutable_mock_env_path):
+def test_env_change_spec_in_definition(tmp_path, mock_packages, config, mutable_mock_env_path):
-    initial_yaml = io.StringIO(_test_matrix_yaml)
+    manifest_file = tmp_path / ev.manifest_name
-    e = ev.create("test", initial_yaml)
+    manifest_file.write_text(_test_matrix_yaml)
+    e = ev.create("test", manifest_file)
     e.concretize()
     e.write()
 
@@ -96,10 +110,11 @@ def test_env_change_spec_in_definition(tmpdir, mock_packages, config, mutable_mo
 
 
 def test_env_change_spec_in_matrix_raises_error(
-    tmpdir, mock_packages, config, mutable_mock_env_path
+    tmp_path, mock_packages, config, mutable_mock_env_path
 ):
-    initial_yaml = io.StringIO(_test_matrix_yaml)
+    manifest_file = tmp_path / ev.manifest_name
-    e = ev.create("test", initial_yaml)
+    manifest_file.write_text(_test_matrix_yaml)
+    e = ev.create("test", manifest_file)
     e.concretize()
     e.write()
 
@@ -131,8 +146,8 @@ def test_user_view_path_is_not_canonicalized_in_yaml(tmpdir, config):
 
     # Serialize environment with relative view path
     with fs.working_dir(str(tmpdir)):
-        fst = ev.Environment(env_path, with_view=view)
+        fst = ev.create_in_dir(env_path, with_view=view)
-        fst.write()
+        fst.regenerate_views()
 
     # The view link should be created
     assert os.path.isdir(absolute_view)
@@ -141,7 +156,7 @@ def test_user_view_path_is_not_canonicalized_in_yaml(tmpdir, config):
     # and also check that the getter is pointing to the right dir.
     with fs.working_dir(str(tmpdir)):
         snd = ev.Environment(env_path)
-        assert snd.yaml["spack"]["view"] == view
+        assert snd.manifest["spack"]["view"] == view
         assert os.path.samefile(snd.default_view.root, absolute_view)
 
 
@@ -184,8 +199,167 @@ def test_roundtrip_spack_yaml_with_comments(original_content, mock_packages, con
     spack_yaml = tmp_path / "spack.yaml"
     spack_yaml.write_text(original_content)
 
-    e = ev.Environment(str(tmp_path))
+    e = ev.Environment(tmp_path)
-    e.update_manifest()
+    e.manifest.flush()
 
     content = spack_yaml.read_text()
     assert content == original_content
+
+
+def test_adding_anonymous_specs_to_env_fails(tmp_path):
+    """Tests that trying to add an anonymous spec to the 'specs' section of an environment
+    raises an exception
+    """
+    env = ev.create_in_dir(tmp_path)
+    with pytest.raises(ev.SpackEnvironmentError, match="cannot add anonymous"):
+        env.add("%gcc")
+
+
+def test_removing_from_non_existing_list_fails(tmp_path):
+    """Tests that trying to remove a spec from a non-existing definition fails."""
+    env = ev.create_in_dir(tmp_path)
+    with pytest.raises(ev.SpackEnvironmentError, match="'bar' does not exist"):
+        env.remove("%gcc", list_name="bar")
+
+
+@pytest.mark.parametrize(
+    "init_view,update_value",
+    [
+        (True, False),
+        (True, "./view"),
+        (False, True),
+        ("./view", True),
+        ("./view", False),
+        (True, True),
+        (False, False),
+    ],
+)
+def test_update_default_view(init_view, update_value, tmp_path, mock_packages, config):
+    """Tests updating the default view with different values."""
+    env = ev.create_in_dir(tmp_path, with_view=init_view)
+    env.update_default_view(update_value)
+    env.write(regenerate=True)
+    if not isinstance(update_value, bool):
+        assert env.default_view.raw_root == update_value
+
+    expected_value = update_value
+    if isinstance(init_view, str) and update_value is True:
+        expected_value = init_view
+
+    assert env.manifest.pristine_yaml_content["spack"]["view"] == expected_value
+
+
+@pytest.mark.parametrize(
+    "initial_content,update_value,expected_view",
+    [
+        (
+            """
+spack:
+  specs:
+  - mpileaks
+  view:
+    default:
+      root: ./view-gcc
+      select: ['%gcc']
+      link_type: symlink
+""",
+            "./another-view",
+            {"root": "./another-view", "select": ["%gcc"], "link_type": "symlink"},
+        ),
+        (
+            """
+spack:
+  specs:
+  - mpileaks
+  view:
+    default:
+      root: ./view-gcc
+      select: ['%gcc']
+      link_type: symlink
+""",
+            True,
+            {"root": "./view-gcc", "select": ["%gcc"], "link_type": "symlink"},
+        ),
+    ],
+)
+def test_update_default_complex_view(
+    initial_content, update_value, expected_view, tmp_path, mock_packages, config
+):
+    spack_yaml = tmp_path / "spack.yaml"
+    spack_yaml.write_text(initial_content)
+
+    env = ev.Environment(tmp_path)
+    env.update_default_view(update_value)
+    env.write(regenerate=True)
+
+    assert env.default_view.to_dict() == expected_view
+
+
+@pytest.mark.parametrize("filename", [ev.manifest_name, ev.lockfile_name])
+def test_cannot_initialize_in_dir_with_init_file(tmp_path, filename):
+    """Tests that initializing an environment in a directory with an already existing
+    spack.yaml or spack.lock raises an exception.
+    """
+    init_file = tmp_path / filename
+    init_file.touch()
+    with pytest.raises(ev.SpackEnvironmentError, match="cannot initialize"):
+        ev.create_in_dir(tmp_path)
+
+
+def test_cannot_initiliaze_if_dirname_exists_as_a_file(tmp_path):
+    """Tests that initializing an environment using as a location an existing file raises
+    an error.
+    """
+    dir_name = tmp_path / "dir"
+    dir_name.touch()
+    with pytest.raises(ev.SpackEnvironmentError, match="cannot initialize"):
+        ev.create_in_dir(dir_name)
+
+
+def test_cannot_initiliaze_if_init_file_does_not_exist(tmp_path):
+    """Tests that initializing an environment passing a non-existing init file raises an error."""
+    init_file = tmp_path / ev.manifest_name
+    with pytest.raises(ev.SpackEnvironmentError, match="cannot initialize"):
+        ev.create_in_dir(tmp_path, init_file=init_file)
+
+
+def test_cannot_initialize_from_random_file(tmp_path):
+    init_file = tmp_path / "foo.txt"
+    init_file.touch()
+    with pytest.raises(ev.SpackEnvironmentError, match="cannot initialize"):
+        ev.create_in_dir(tmp_path, init_file=init_file)
+
+
+def test_environment_pickle(tmp_path):
+    env1 = ev.create_in_dir(tmp_path)
+    obj = pickle.dumps(env1)
+    env2 = pickle.loads(obj)
+    assert isinstance(env2, ev.Environment)
+
+
+def test_error_on_nonempty_view_dir(tmpdir):
+    """Error when the target is not an empty dir"""
+    with tmpdir.as_cwd():
+        os.mkdir("empty_dir")
+        os.mkdir("nonempty_dir")
+        with open(os.path.join("nonempty_dir", "file"), "wb"):
+            pass
+        os.symlink("empty_dir", "symlinked_empty_dir")
+        os.symlink("does_not_exist", "broken_link")
+        os.symlink("broken_link", "file")
+
+        # This is OK.
+        _error_on_nonempty_view_dir("empty_dir")
+
+        # This is not OK.
+        with pytest.raises(SpackEnvironmentViewError):
+            _error_on_nonempty_view_dir("nonempty_dir")
+
+        with pytest.raises(SpackEnvironmentViewError):
+            _error_on_nonempty_view_dir("symlinked_empty_dir")
+
+        with pytest.raises(SpackEnvironmentViewError):
+            _error_on_nonempty_view_dir("broken_link")
+
+        with pytest.raises(SpackEnvironmentViewError):
+            _error_on_nonempty_view_dir("file")

lib/spack/spack/test/environment.py

@@ -1,47 +0,0 @@
-# Copyright 2013-2023 Lawrence Livermore National Security, LLC and other
-# Spack Project Developers. See the top-level COPYRIGHT file for details.
-#
-# SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-import os
-import pickle
-
-import pytest
-
-from spack.environment import Environment
-from spack.environment.environment import SpackEnvironmentViewError, _error_on_nonempty_view_dir
-
-
-def test_environment_pickle(tmpdir):
-    env1 = Environment(str(tmpdir))
-    obj = pickle.dumps(env1)
-    env2 = pickle.loads(obj)
-    assert isinstance(env2, Environment)
-
-
-def test_error_on_nonempty_view_dir(tmpdir):
-    """Error when the target is not an empty dir"""
-    with tmpdir.as_cwd():
-        os.mkdir("empty_dir")
-        os.mkdir("nonempty_dir")
-        with open(os.path.join("nonempty_dir", "file"), "wb"):
-            pass
-        os.symlink("empty_dir", "symlinked_empty_dir")
-        os.symlink("does_not_exist", "broken_link")
-        os.symlink("broken_link", "file")
-
-        # This is OK.
-        _error_on_nonempty_view_dir("empty_dir")
-
-        # This is not OK.
-        with pytest.raises(SpackEnvironmentViewError):
-            _error_on_nonempty_view_dir("nonempty_dir")
-
-        with pytest.raises(SpackEnvironmentViewError):
-            _error_on_nonempty_view_dir("symlinked_empty_dir")
-
-        with pytest.raises(SpackEnvironmentViewError):
-            _error_on_nonempty_view_dir("broken_link")
-
-        with pytest.raises(SpackEnvironmentViewError):
-            _error_on_nonempty_view_dir("file")

lib/spack/spack/test/modules/lmod.py

@@ -335,7 +335,7 @@ def test_projections_all(self, factory, module_configuration):
     def test_modules_relative_to_view(
         self, tmpdir, modulefile_content, module_configuration, install_mockery, mock_fetch
     ):
-        with ev.Environment(str(tmpdir), with_view=True) as e:
+        with ev.create_in_dir(str(tmpdir), with_view=True) as e:
             module_configuration("with_view")
             install("--add", "cmake")