From b8d15e816bf7388774e65ca59daf3831b4fa4180 Mon Sep 17 00:00:00 2001
From: psakievich <psakiev@sandia.gov>
Date: Tue, 21 Feb 2023 17:37:14 -0700
Subject: [PATCH] Allow users to specify root env dir (#32836)

* Allow users to specify root env dir

Environments managed by spack have some advantages over anonymous Environments
but they are tucked away inside spack's directory tree. This PR gives
users the ability to specify where the environments should live.

See #32823

This is also taken as an opportunity to ensure that all references are to "managed environments",
rather than "named environments". Prior to this PR some references to the latter persisted.

Co-authored-by: Tom Scogland <scogland1@llnl.gov>
Co-authored-by: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com>
Co-authored-by: Gregory Becker <becker33@llnl.gov>
---
 bin/spack-tmpconfig                        |  1 +
 etc/spack/defaults/config.yaml             |  4 ++
 lib/spack/docs/environments.rst            |  9 +++--
 lib/spack/spack/cmd/env.py                 |  2 +-
 lib/spack/spack/cmd/location.py            |  2 +-
 lib/spack/spack/environment/environment.py | 46 ++++++++++++++++------
 lib/spack/spack/main.py                    |  2 +-
 lib/spack/spack/schema/config.py           |  1 +
 lib/spack/spack/test/cmd/concretize.py     |  9 ++---
 lib/spack/spack/test/cmd/env.py            | 17 ++++++++
 lib/spack/spack/test/cmd/uninstall.py      |  5 ++-
 lib/spack/spack/test/conftest.py           | 10 ++---
 lib/spack/spack/test/env.py                | 18 +++++++++
 13 files changed, 97 insertions(+), 29 deletions(-)

diff --git a/bin/spack-tmpconfig b/bin/spack-tmpconfig
index a3d053d340..a477daa4d3 100755
--- a/bin/spack-tmpconfig
+++ b/bin/spack-tmpconfig
@@ -72,6 +72,7 @@ config:
     root: $TMP_DIR/install
   misc_cache: $$user_cache_path/cache
   source_cache: $$user_cache_path/source
+  environments_root: $TMP_DIR/envs
 EOF
 cat >"$SPACK_USER_CONFIG_PATH/bootstrap.yaml" <<EOF
 bootstrap:
diff --git a/etc/spack/defaults/config.yaml b/etc/spack/defaults/config.yaml
index 06d6a10909..43f8a98dff 100644
--- a/etc/spack/defaults/config.yaml
+++ b/etc/spack/defaults/config.yaml
@@ -81,6 +81,10 @@ config:
   source_cache: $spack/var/spack/cache
 
 
+  ## Directory where spack managed environments are created and stored
+  # environments_root: $spack/var/spack/environments
+
+
   # Cache directory for miscellaneous files, like the package index.
   # This can be purged with `spack clean --misc-cache`
   misc_cache: $user_cache_path/cache
diff --git a/lib/spack/docs/environments.rst b/lib/spack/docs/environments.rst
index 09d8f07af1..e2805e4f01 100644
--- a/lib/spack/docs/environments.rst
+++ b/lib/spack/docs/environments.rst
@@ -58,9 +58,9 @@ Using Environments
 Here we follow a typical use case of creating, concretizing,
 installing and loading an environment.
 
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-Creating a named Environment
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+Creating a managed Environment
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 An environment is created by:
 
@@ -72,7 +72,8 @@ Spack then creates the directory ``var/spack/environments/myenv``.
 
 .. note::
 
-   All named environments are stored in the ``var/spack/environments`` folder.
+   All managed environments by default are stored in the ``var/spack/environments`` folder.
+   This location can be changed by setting the ``environments_root`` variable in ``config.yaml``.
 
 In the ``var/spack/environments/myenv`` directory, Spack creates the
 file ``spack.yaml`` and the hidden directory ``.spack-env``.
diff --git a/lib/spack/spack/cmd/env.py b/lib/spack/spack/cmd/env.py
index fe7d1ad59a..085757ca0f 100644
--- a/lib/spack/spack/cmd/env.py
+++ b/lib/spack/spack/cmd/env.py
@@ -165,7 +165,7 @@ def env_activate(args):
         short_name = os.path.basename(env_path)
         ev.Environment(env).write(regenerate=False)
 
-    # Named environment
+    # Managed environment
     elif ev.exists(env_name_or_dir) and not args.dir:
         env_path = ev.root(env_name_or_dir)
         short_name = env_name_or_dir
diff --git a/lib/spack/spack/cmd/location.py b/lib/spack/spack/cmd/location.py
index 7cce744ca7..aba8264a28 100644
--- a/lib/spack/spack/cmd/location.py
+++ b/lib/spack/spack/cmd/location.py
@@ -95,7 +95,7 @@ def location(parser, args):
             spack.cmd.require_active_env("location -e")
             path = ev.active_environment().path
         else:
-            # Get named environment path
+            # Get path of requested environment
             if not ev.exists(args.location_env):
                 tty.die("no such environment: '%s'" % args.location_env)
             path = ev.root(args.location_env)
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 813ffca0b6..cabb255e23 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -62,8 +62,8 @@
 _active_environment = None
 
 
-#: path where environments are stored in the spack tree
-env_path = os.path.join(spack.paths.var_path, "environments")
+#: default path where environments are stored in the spack tree
+default_env_path = os.path.join(spack.paths.var_path, "environments")
 
 
 #: Name of the input yaml file for an environment
@@ -78,6 +78,26 @@
 env_subdir_name = ".spack-env"
 
 
+def env_root_path():
+    """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 check_disallowed_env_config_mods(scopes):
+    for scope in scopes:
+        with spack.config.use_configuration(scope):
+            if spack.config.get("config:environments_root"):
+                raise SpackEnvironmentError(
+                    "Spack environments are prohibited from modifying 'config:environments_root' "
+                    "because it can make the definition of the environment ill-posed. Please "
+                    "remove from your environment and place it in a permanent scope such as "
+                    "defaults, system, site, etc."
+                )
+    return scopes
+
+
 def default_manifest_yaml():
     """default spack.yaml file to put in new environments"""
     return """\
@@ -214,7 +234,7 @@ def active_environment():
 
 def _root(name):
     """Non-validating version of root(), to be used internally."""
-    return os.path.join(env_path, name)
+    return os.path.join(env_root_path(), name)
 
 
 def root(name):
@@ -249,10 +269,12 @@ def read(name):
 
 
 def create(name, init_file=None, with_view=None, keep_relative=False):
-    """Create a named environment in Spack."""
+    """Create a managed environment in Spack."""
+    if not os.path.isdir(env_root_path()):
+        fs.mkdirp(env_root_path())
     validate_env_name(name)
     if exists(name):
-        raise SpackEnvironmentError("'%s': environment already exists" % name)
+        raise SpackEnvironmentError("'%s': environment already exists at %s" % (name, root(name)))
     return Environment(root(name), init_file, with_view, keep_relative)
 
 
@@ -266,10 +288,10 @@ 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
     # operation like list should not try to create a directory.
-    if not os.path.exists(env_path):
+    if not os.path.exists(env_root_path()):
         return []
 
-    candidates = sorted(os.listdir(env_path))
+    candidates = sorted(os.listdir(env_root_path()))
     names = []
     for candidate in candidates:
         yaml_path = os.path.join(_root(candidate), manifest_name)
@@ -279,7 +301,7 @@ def all_environment_names():
 
 
 def all_environments():
-    """Generator for all named Environments."""
+    """Generator for all managed Environments."""
     for name in all_environment_names():
         yield read(name)
 
@@ -859,14 +881,14 @@ def clear(self, re_read=False):
     @property
     def internal(self):
         """Whether this environment is managed by Spack."""
-        return self.path.startswith(env_path)
+        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 named environments.
+        for managed environments.
         """
         if self.internal:
             return os.path.basename(self.path)
@@ -1044,7 +1066,9 @@ def env_file_config_scope(self):
 
     def config_scopes(self):
         """A list of all configuration scopes for this environment."""
-        return self.included_config_scopes() + [self.env_file_config_scope()]
+        return check_disallowed_env_config_mods(
+            self.included_config_scopes() + [self.env_file_config_scope()]
+        )
 
     def destroy(self):
         """Remove this environment from Spack entirely."""
diff --git a/lib/spack/spack/main.py b/lib/spack/spack/main.py
index 06183ac112..ae6bed7fd0 100644
--- a/lib/spack/spack/main.py
+++ b/lib/spack/spack/main.py
@@ -459,7 +459,7 @@ def make_argument_parser(**kwargs):
         dest="env_dir",
         metavar="DIR",
         action="store",
-        help="run with an environment directory (ignore named environments)",
+        help="run with an environment directory (ignore managed environments)",
     )
     env_group.add_argument(
         "-E",
diff --git a/lib/spack/spack/schema/config.py b/lib/spack/spack/schema/config.py
index 7ecd1ad627..abe207cac0 100644
--- a/lib/spack/spack/schema/config.py
+++ b/lib/spack/spack/schema/config.py
@@ -67,6 +67,7 @@
             "license_dir": {"type": "string"},
             "source_cache": {"type": "string"},
             "misc_cache": {"type": "string"},
+            "environments_root": {"type": "string"},
             "connect_timeout": {"type": "integer", "minimum": 0},
             "verify_ssl": {"type": "boolean"},
             "suppress_gpg_warnings": {"type": "boolean"},
diff --git a/lib/spack/spack/test/cmd/concretize.py b/lib/spack/spack/test/cmd/concretize.py
index 08a19162f9..868da6df61 100644
--- a/lib/spack/spack/test/cmd/concretize.py
+++ b/lib/spack/spack/test/cmd/concretize.py
@@ -9,8 +9,7 @@
 import spack.environment as ev
 from spack.main import SpackCommand
 
-# everything here uses the mock_env_path
-pytestmark = pytest.mark.usefixtures("mutable_mock_env_path", "config", "mutable_mock_repo")
+pytestmark = pytest.mark.usefixtures("config", "mutable_mock_repo")
 
 env = SpackCommand("env")
 add = SpackCommand("add")
@@ -21,7 +20,7 @@
 
 
 @pytest.mark.parametrize("unify", unification_strategies)
-def test_concretize_all_test_dependencies(unify):
+def test_concretize_all_test_dependencies(unify, mutable_mock_env_path):
     """Check all test dependencies are concretized."""
     env("create", "test")
 
@@ -33,7 +32,7 @@ def test_concretize_all_test_dependencies(unify):
 
 
 @pytest.mark.parametrize("unify", unification_strategies)
-def test_concretize_root_test_dependencies_not_recursive(unify):
+def test_concretize_root_test_dependencies_not_recursive(unify, mutable_mock_env_path):
     """Check that test dependencies are not concretized recursively."""
     env("create", "test")
 
@@ -45,7 +44,7 @@ def test_concretize_root_test_dependencies_not_recursive(unify):
 
 
 @pytest.mark.parametrize("unify", unification_strategies)
-def test_concretize_root_test_dependencies_are_concretized(unify):
+def test_concretize_root_test_dependencies_are_concretized(unify, mutable_mock_env_path):
     """Check that root test dependencies are concretized."""
     env("create", "test")
 
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index e757384b8f..ce7862cd12 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -3222,3 +3222,20 @@ def test_relative_view_path_on_command_line_is_made_absolute(tmpdir, config):
         env("create", "--with-view", "view", "--dir", "env")
         environment = ev.Environment(os.path.join(".", "env"))
         assert os.path.samefile("view", environment.default_view.root)
+
+
+def test_environment_created_in_users_location(mutable_config, tmpdir):
+    """Test that an environment is created in a location based on the config"""
+    spack.config.set("config:environments_root", str(tmpdir.join("envs")))
+    env_dir = spack.config.get("config:environments_root")
+
+    assert tmpdir.strpath in env_dir
+    assert not os.path.isdir(env_dir)
+
+    dir_name = "user_env"
+    env("create", dir_name)
+    out = env("list")
+
+    assert dir_name in out
+    assert env_dir in ev.root(dir_name)
+    assert os.path.isdir(os.path.join(env_dir, dir_name))
diff --git a/lib/spack/spack/test/cmd/uninstall.py b/lib/spack/spack/test/cmd/uninstall.py
index a4b6410ef8..50bdf63d46 100644
--- a/lib/spack/spack/test/cmd/uninstall.py
+++ b/lib/spack/spack/test/cmd/uninstall.py
@@ -225,7 +225,7 @@ class TestUninstallFromEnv(object):
     concretize = SpackCommand("concretize")
     find = SpackCommand("find")
 
-    @pytest.fixture
+    @pytest.fixture(scope="function")
     def environment_setup(
         self, mutable_mock_env_path, config, mock_packages, mutable_database, install_mockery
     ):
@@ -244,6 +244,9 @@ def environment_setup(
             TestUninstallFromEnv.add("diamond-link-bottom")
             TestUninstallFromEnv.concretize()
             install("--fake")
+        yield "environment_setup"
+        TestUninstallFromEnv.env("rm", "e1", "-y")
+        TestUninstallFromEnv.env("rm", "e2", "-y")
 
     def test_basic_env_sanity(self, environment_setup):
         for env_name in ["e1", "e2"]:
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index 9a7b3a6a9a..b32ea4b4f8 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -1535,14 +1535,14 @@ def get_rev():
     yield t
 
 
-@pytest.fixture()
-def mutable_mock_env_path(tmpdir_factory):
+@pytest.fixture(scope="function")
+def mutable_mock_env_path(tmpdir_factory, mutable_config):
     """Fixture for mocking the internal spack environments directory."""
-    saved_path = ev.environment.env_path
+    saved_path = ev.environment.default_env_path
     mock_path = tmpdir_factory.mktemp("mock-env-path")
-    ev.environment.env_path = str(mock_path)
+    ev.environment.default_env_path = str(mock_path)
     yield mock_path
-    ev.environment.env_path = saved_path
+    ev.environment.default_env_path = saved_path
 
 
 @pytest.fixture()
diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py
index 83538e7d71..8614a92813 100644
--- a/lib/spack/spack/test/env.py
+++ b/lib/spack/spack/test/env.py
@@ -143,3 +143,21 @@ def test_user_view_path_is_not_canonicalized_in_yaml(tmpdir, config):
         snd = ev.Environment(env_path)
         assert snd.yaml["spack"]["view"] == view
         assert os.path.samefile(snd.default_view.root, absolute_view)
+
+
+def test_environment_cant_modify_environments_root(tmpdir):
+    filename = str(tmpdir.join("spack.yaml"))
+    with open(filename, "w") as f:
+        f.write(
+            """\
+ spack:
+   config:
+     environments_root: /a/black/hole
+   view: false
+   specs: []
+ """
+        )
+    with tmpdir.as_cwd():
+        with pytest.raises(ev.SpackEnvironmentError):
+            e = ev.Environment(tmpdir.strpath)
+            ev.activate(e)