Move shell aware env into spack.environment.shell (#25608)

Cherry-picked from #25564 so this is standalone.

With this PR we can activate an environment in Spack itself, without computing changes to environment variables only necessary for "shell aware" env activation.

1. Activating an environment:
    
    ```python
    spack.environment.activate(Environment(xyz)) -> None
    ```
    this basically just sets `_active_environment` and modifies some config scopes.

2. Activating an environment **and** getting environment variable modifications for the shell:

    ```python
    spack.environment.shell.activate(Environment(xyz)) -> EnvironmentModifications
    ```

This should make it easier/faster to do unit tests and scripting with spack, without the cli interface.

12 files changed, 346 insertions, 218 deletions

diff --git a/lib/spack/spack/bootstrap.py b/lib/spack/spack/bootstrap.py
index 8f4119a04d..76fc6f487f 100644
--- a/lib/spack/spack/bootstrap.py
+++ b/lib/spack/spack/bootstrap.py
@@ -539,7 +539,7 @@ def _add_externals_if_missing():
 def ensure_bootstrap_configuration():
     bootstrap_store_path = store_path()
     user_configuration = _read_and_sanitize_configuration()
-    with spack.environment.deactivate_environment():
+    with spack.environment.no_active_environment():
         with spack.architecture.use_platform(spack.architecture.real_platform()):
             with spack.repo.use_repositories(spack.paths.packages_path):
                 with spack.store.use_store(bootstrap_store_path):
diff --git a/lib/spack/spack/cmd/common/arguments.py b/lib/spack/spack/cmd/common/arguments.py
index 40a30a2618..1b173bf584 100644
--- a/lib/spack/spack/cmd/common/arguments.py
+++ b/lib/spack/spack/cmd/common/arguments.py
@@ -69,7 +69,7 @@ class ConstraintAction(argparse.Action):
 
         # If an environment is provided, we'll restrict the search to
         # only its installed packages.
-        env = ev._active_environment
+        env = ev.active_environment()
         if env:
             kwargs['hashes'] = set(env.all_hashes())
 
diff --git a/lib/spack/spack/cmd/env.py b/lib/spack/spack/cmd/env.py
index 5d436624bd..ae45e105de 100644
--- a/lib/spack/spack/cmd/env.py
+++ b/lib/spack/spack/cmd/env.py
@@ -19,6 +19,7 @@ import spack.cmd.modules
 import spack.cmd.uninstall
 import spack.config
 import spack.environment as ev
+import spack.environment.shell
 import spack.schema.env
 import spack.util.string as string
 
@@ -81,6 +82,7 @@ def env_activate_setup_parser(subparser):
 
 def env_activate(args):
     env = args.activate_env
+
     if not args.shell:
         spack.cmd.common.shell_init_instructions(
             "spack env activate",
@@ -110,10 +112,19 @@ def env_activate(args):
         tty.debug("Environment %s is already active" % args.activate_env)
         return
 
-    cmds = ev.activate(
-        ev.Environment(spack_env), add_view=args.with_view, shell=args.shell,
+    # Activate new environment
+    active_env = ev.Environment(spack_env)
+    cmds = spack.environment.shell.activate_header(
+        env=active_env,
+        shell=args.shell,
         prompt=env_prompt if args.prompt else None
     )
+    env_mods = spack.environment.shell.activate(
+        env=active_env,
+        add_view=args.with_view
+    )
+    cmds += env_mods.shell_modifications(args.shell)
+
     sys.stdout.write(cmds)
 
 
@@ -150,7 +161,9 @@ def env_deactivate(args):
     if 'SPACK_ENV' not in os.environ:
         tty.die('No environment is currently active.')
 
-    cmds = ev.deactivate(shell=args.shell)
+    cmds = spack.environment.shell.deactivate_header(args.shell)
+    env_mods = spack.environment.shell.deactivate()
+    cmds += env_mods.shell_modifications(args.shell)
     sys.stdout.write(cmds)
 
 
diff --git a/lib/spack/spack/environment/__init__.py b/lib/spack/spack/environment/__init__.py
new file mode 100644
index 0000000000..0f04162b35
--- /dev/null
+++ b/lib/spack/spack/environment/__init__.py
@@ -0,0 +1,57 @@
+# Copyright 2013-2021 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)
+from .environment import (
+    Environment,
+    SpackEnvironmentError,
+    activate,
+    active,
+    active_environment,
+    all_environment_names,
+    all_environments,
+    config_dict,
+    create,
+    deactivate,
+    default_manifest_yaml,
+    default_view_name,
+    display_specs,
+    exists,
+    is_env_dir,
+    is_latest_format,
+    lockfile_name,
+    manifest_file,
+    manifest_name,
+    no_active_environment,
+    read,
+    root,
+    spack_env_var,
+    update_yaml,
+)
+
+__all__ = [
+    'Environment',
+    'SpackEnvironmentError',
+    'activate',
+    'active',
+    'active_environment',
+    'all_environment_names',
+    'all_environments',
+    'config_dict',
+    'create',
+    'deactivate',
+    'default_manifest_yaml',
+    'default_view_name',
+    'display_specs',
+    'exists',
+    'is_env_dir',
+    'is_latest_format',
+    'lockfile_name',
+    'manifest_file',
+    'manifest_name',
+    'no_active_environment',
+    'read',
+    'root',
+    'spack_env_var',
+    'update_yaml',
+]
diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment/environment.py
index 330d90915e..88dd545398 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -16,13 +16,13 @@ from ordereddict_backport import OrderedDict
 
 import llnl.util.filesystem as fs
 import llnl.util.tty as tty
-from llnl.util.tty.color import colorize
 
 import spack.concretize
 import spack.config
 import spack.error
 import spack.hash_types as ht
 import spack.hooks
+import spack.paths
 import spack.repo
 import spack.schema.env
 import spack.spec
@@ -108,9 +108,7 @@ def validate_env_name(name):
     return name
 
 
-def activate(
-    env, use_env_repo=False, add_view=True, shell='sh', prompt=None
-):
+def activate(env, use_env_repo=False):
     """Activate an environment.
 
     To activate an environment, we add its configuration scope to the
@@ -121,96 +119,26 @@ def activate(
         env (Environment): the environment to activate
         use_env_repo (bool): use the packages exactly as they appear in the
             environment's repository
-        add_view (bool): generate commands to add view to path variables
-        shell (str): One of `sh`, `csh`, `fish`.
-        prompt (str): string to add to the users prompt, or None
-
-    Returns:
-        str: Shell commands to activate environment.
-
-    TODO: environment to use the activated spack environment.
     """
     global _active_environment
 
-    _active_environment = env
-    prepare_config_scope(_active_environment)
+    # Fail early to avoid ending in an invalid state
+    if not isinstance(env, Environment):
+        raise TypeError("`env` should be of type {0}".format(Environment.__name__))
+
+    prepare_config_scope(env)
+
     if use_env_repo:
-        spack.repo.path.put_first(_active_environment.repo)
-
-    tty.debug("Using environment '%s'" % _active_environment.name)
-
-    # Construct the commands to run
-    cmds = ''
-    if shell == 'csh':
-        # TODO: figure out how to make color work for csh
-        cmds += 'setenv SPACK_ENV %s;\n' % env.path
-        cmds += 'alias despacktivate "spack env deactivate";\n'
-        if prompt:
-            cmds += 'if (! $?SPACK_OLD_PROMPT ) '
-            cmds += 'setenv SPACK_OLD_PROMPT "${prompt}";\n'
-            cmds += 'set prompt="%s ${prompt}";\n' % prompt
-    elif shell == 'fish':
-        if os.getenv('TERM') and 'color' in os.getenv('TERM') and prompt:
-            prompt = colorize('@G{%s} ' % prompt, color=True)
-
-        cmds += 'set -gx SPACK_ENV %s;\n' % env.path
-        cmds += 'function despacktivate;\n'
-        cmds += '   spack env deactivate;\n'
-        cmds += 'end;\n'
-        #
-        # NOTE: We're not changing the fish_prompt function (which is fish's
-        # solution to the PS1 variable) here. This is a bit fiddly, and easy to
-        # screw up => spend time reasearching a solution. Feedback welcome.
-        #
-    else:
-        if os.getenv('TERM') and 'color' in os.getenv('TERM') and prompt:
-            prompt = colorize('@G{%s} ' % prompt, color=True)
-
-        cmds += 'export SPACK_ENV=%s;\n' % env.path
-        cmds += "alias despacktivate='spack env deactivate';\n"
-        if prompt:
-            cmds += 'if [ -z ${SPACK_OLD_PS1+x} ]; then\n'
-            cmds += '    if [ -z ${PS1+x} ]; then\n'
-            cmds += "        PS1='$$$$';\n"
-            cmds += '    fi;\n'
-            cmds += '    export SPACK_OLD_PS1="${PS1}";\n'
-            cmds += 'fi;\n'
-            cmds += 'export PS1="%s ${PS1}";\n' % prompt
-
-    #
-    # NOTE in the fish-shell: Path variables are a special kind of variable
-    # used to support colon-delimited path lists including PATH, CDPATH,
-    # MANPATH, PYTHONPATH, etc. All variables that end in PATH (case-sensitive)
-    # become PATH variables.
-    #
-    try:
-        if add_view and default_view_name in env.views:
-            with spack.store.db.read_transaction():
-                cmds += env.add_default_view_to_shell(shell)
-    except (spack.repo.UnknownPackageError,
-            spack.repo.UnknownNamespaceError) as e:
-        tty.error(e)
-        tty.die(
-            'Environment view is broken due to a missing package or repo.\n',
-            '  To activate without views enabled, activate with:\n',
-            '    spack env activate -V {0}\n'.format(env.name),
-            '  To remove it and resolve the issue, '
-            'force concretize with the command:\n',
-            '    spack -e {0} concretize --force'.format(env.name))
-
-    return cmds
-
-
-def deactivate(shell='sh'):
-    """Undo any configuration or repo settings modified by ``activate()``.
+        spack.repo.path.put_first(env.repo)
 
-    Arguments:
-        shell (str): One of `sh`, `csh`, `fish`. Shell style to use.
+    tty.debug("Using environment '%s'" % env.name)
 
-    Returns:
-        str: shell commands for `shell` to undo environment variables
+    # Do this last, because setting up the config must succeed first.
+    _active_environment = env
 
-    """
+
+def deactivate():
+    """Undo any configuration or repo settings modified by ``activate()``."""
     global _active_environment
 
     if not _active_environment:
@@ -222,47 +150,9 @@ def deactivate(shell='sh'):
     if _active_environment._repo:
         spack.repo.path.remove(_active_environment._repo)
 
-    cmds = ''
-    if shell == 'csh':
-        cmds += 'unsetenv SPACK_ENV;\n'
-        cmds += 'if ( $?SPACK_OLD_PROMPT ) '
-        cmds += 'set prompt="$SPACK_OLD_PROMPT" && '
-        cmds += 'unsetenv SPACK_OLD_PROMPT;\n'
-        cmds += 'unalias despacktivate;\n'
-    elif shell == 'fish':
-        cmds += 'set -e SPACK_ENV;\n'
-        cmds += 'functions -e despacktivate;\n'
-        #
-        # NOTE: Not changing fish_prompt (above) => no need to restore it here.
-        #
-    else:
-        cmds += 'if [ ! -z ${SPACK_ENV+x} ]; then\n'
-        cmds += 'unset SPACK_ENV; export SPACK_ENV;\n'
-        cmds += 'fi;\n'
-        cmds += 'unalias despacktivate;\n'
-        cmds += 'if [ ! -z ${SPACK_OLD_PS1+x} ]; then\n'
-        cmds += '    if [ "$SPACK_OLD_PS1" = \'$$$$\' ]; then\n'
-        cmds += '        unset PS1; export PS1;\n'
-        cmds += '    else\n'
-        cmds += '        export PS1="$SPACK_OLD_PS1";\n'
-        cmds += '    fi;\n'
-        cmds += '    unset SPACK_OLD_PS1; export SPACK_OLD_PS1;\n'
-        cmds += 'fi;\n'
-
-    try:
-        if default_view_name in _active_environment.views:
-            with spack.store.db.read_transaction():
-                cmds += _active_environment.rm_default_view_from_shell(shell)
-    except (spack.repo.UnknownPackageError,
-            spack.repo.UnknownNamespaceError) as e:
-        tty.warn(e)
-        tty.warn('Could not fully deactivate view due to missing package '
-                 'or repo, shell environment may be corrupt.')
-
     tty.debug("Deactivated environment '%s'" % _active_environment.name)
-    _active_environment = None
 
-    return cmds
+    _active_environment = None
 
 
 def active_environment():
@@ -1345,12 +1235,18 @@ class Environment(object):
 
         return all_mods, errors
 
-    def add_default_view_to_shell(self, shell):
-        env_mod = spack.util.environment.EnvironmentModifications()
+    def add_default_view_to_env(self, env_mod):
+        """
+        Collect the environment modifications to activate an environment using the
+        default view. Removes duplicate paths.
 
+        Args:
+            env_mod (spack.util.environment.EnvironmentModifications): the environment
+                modifications object that is modified.
+        """
         if default_view_name not in self.views:
             # No default view to add to shell
-            return env_mod.shell_modifications(shell)
+            return env_mod
 
         env_mod.extend(uenv.unconditional_environment_modifications(
             self.default_view))
@@ -1365,14 +1261,20 @@ class Environment(object):
         for env_var in env_mod.group_by_name():
             env_mod.prune_duplicate_paths(env_var)
 
-        return env_mod.shell_modifications(shell)
+        return env_mod
 
-    def rm_default_view_from_shell(self, shell):
-        env_mod = spack.util.environment.EnvironmentModifications()
+    def rm_default_view_from_env(self, env_mod):
+        """
+        Collect the environment modifications to deactivate an environment using the
+        default view. Reverses the action of ``add_default_view_to_env``.
 
+        Args:
+            env_mod (spack.util.environment.EnvironmentModifications): the environment
+                modifications object that is modified.
+        """
         if default_view_name not in self.views:
             # No default view to add to shell
-            return env_mod.shell_modifications(shell)
+            return env_mod
 
         env_mod.extend(uenv.unconditional_environment_modifications(
             self.default_view).reversed())
@@ -1380,7 +1282,7 @@ class Environment(object):
         mods, _ = self._env_modifications_for_default_view(reverse=True)
         env_mod.extend(mods)
 
-        return env_mod.shell_modifications(shell)
+        return env_mod
 
     def _add_concrete_spec(self, spec, concrete, new=True):
         """Called when a new concretized spec is added to the environment.
@@ -2165,14 +2067,17 @@ def is_latest_format(manifest):
 
 
 @contextlib.contextmanager
-def deactivate_environment():
-    """Deactivate an active environment for the duration of the context."""
-    global _active_environment
-    current, _active_environment = _active_environment, None
+def no_active_environment():
+    """Deactivate the active environment for the duration of the context. Has no
+       effect when there is no active environment."""
+    env = active_environment()
     try:
+        deactivate()
         yield
     finally:
-        _active_environment = current
+        # TODO: we don't handle `use_env_repo` here.
+        if env:
+            activate(env)
 
 
 class SpackEnvironmentError(spack.error.SpackError):
diff --git a/lib/spack/spack/environment/shell.py b/lib/spack/spack/environment/shell.py
new file mode 100644
index 0000000000..1ffd6cd171
--- /dev/null
+++ b/lib/spack/spack/environment/shell.py
@@ -0,0 +1,161 @@
+# Copyright 2013-2021 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 llnl.util.tty as tty
+from llnl.util.tty.color import colorize
+
+import spack.environment as ev
+import spack.repo
+import spack.store
+from spack.util.environment import EnvironmentModifications
+
+
+def activate_header(env, shell, prompt=None):
+    # Construct the commands to run
+    cmds = ''
+    if shell == 'csh':
+        # TODO: figure out how to make color work for csh
+        cmds += 'setenv SPACK_ENV %s;\n' % env.path
+        cmds += 'alias despacktivate "spack env deactivate";\n'
+        if prompt:
+            cmds += 'if (! $?SPACK_OLD_PROMPT ) '
+            cmds += 'setenv SPACK_OLD_PROMPT "${prompt}";\n'
+            cmds += 'set prompt="%s ${prompt}";\n' % prompt
+    elif shell == 'fish':
+        if 'color' in os.getenv('TERM', '') and prompt:
+            prompt = colorize('@G{%s} ' % prompt, color=True)
+
+        cmds += 'set -gx SPACK_ENV %s;\n' % env.path
+        cmds += 'function despacktivate;\n'
+        cmds += '   spack env deactivate;\n'
+        cmds += 'end;\n'
+        #
+        # NOTE: We're not changing the fish_prompt function (which is fish's
+        # solution to the PS1 variable) here. This is a bit fiddly, and easy to
+        # screw up => spend time reasearching a solution. Feedback welcome.
+        #
+    else:
+        if 'color' in os.getenv('TERM', '') and prompt:
+            prompt = colorize('@G{%s} ' % prompt, color=True)
+
+        cmds += 'export SPACK_ENV=%s;\n' % env.path
+        cmds += "alias despacktivate='spack env deactivate';\n"
+        if prompt:
+            cmds += 'if [ -z ${SPACK_OLD_PS1+x} ]; then\n'
+            cmds += '    if [ -z ${PS1+x} ]; then\n'
+            cmds += "        PS1='$$$$';\n"
+            cmds += '    fi;\n'
+            cmds += '    export SPACK_OLD_PS1="${PS1}";\n'
+            cmds += 'fi;\n'
+            cmds += 'export PS1="%s ${PS1}";\n' % prompt
+
+    return cmds
+
+
+def deactivate_header(shell):
+    cmds = ''
+    if shell == 'csh':
+        cmds += 'unsetenv SPACK_ENV;\n'
+        cmds += 'if ( $?SPACK_OLD_PROMPT ) '
+        cmds += 'set prompt="$SPACK_OLD_PROMPT" && '
+        cmds += 'unsetenv SPACK_OLD_PROMPT;\n'
+        cmds += 'unalias despacktivate;\n'
+    elif shell == 'fish':
+        cmds += 'set -e SPACK_ENV;\n'
+        cmds += 'functions -e despacktivate;\n'
+        #
+        # NOTE: Not changing fish_prompt (above) => no need to restore it here.
+        #
+    else:
+        cmds += 'if [ ! -z ${SPACK_ENV+x} ]; then\n'
+        cmds += 'unset SPACK_ENV; export SPACK_ENV;\n'
+        cmds += 'fi;\n'
+        cmds += 'unalias despacktivate;\n'
+        cmds += 'if [ ! -z ${SPACK_OLD_PS1+x} ]; then\n'
+        cmds += '    if [ "$SPACK_OLD_PS1" = \'$$$$\' ]; then\n'
+        cmds += '        unset PS1; export PS1;\n'
+        cmds += '    else\n'
+        cmds += '        export PS1="$SPACK_OLD_PS1";\n'
+        cmds += '    fi;\n'
+        cmds += '    unset SPACK_OLD_PS1; export SPACK_OLD_PS1;\n'
+        cmds += 'fi;\n'
+
+    return cmds
+
+
+def activate(env, use_env_repo=False, add_view=True):
+    """
+    Activate an environment and append environment modifications
+
+    To activate an environment, we add its configuration scope to the
+    existing Spack configuration, and we set active to the current
+    environment.
+
+    Arguments:
+        env (spack.environment.Environment): the environment to activate
+        use_env_repo (bool): use the packages exactly as they appear in the
+            environment's repository
+        add_view (bool): generate commands to add view to path variables
+
+    Returns:
+        spack.util.environment.EnvironmentModifications: Environment variables
+        modifications to activate environment.
+    """
+    ev.activate(env, use_env_repo=use_env_repo)
+
+    env_mods = EnvironmentModifications()
+
+    #
+    # NOTE in the fish-shell: Path variables are a special kind of variable
+    # used to support colon-delimited path lists including PATH, CDPATH,
+    # MANPATH, PYTHONPATH, etc. All variables that end in PATH (case-sensitive)
+    # become PATH variables.
+    #
+    try:
+        if add_view and ev.default_view_name in env.views:
+            with spack.store.db.read_transaction():
+                env.add_default_view_to_env(env_mods)
+    except (spack.repo.UnknownPackageError,
+            spack.repo.UnknownNamespaceError) as e:
+        tty.error(e)
+        tty.die(
+            'Environment view is broken due to a missing package or repo.\n',
+            '  To activate without views enabled, activate with:\n',
+            '    spack env activate -V {0}\n'.format(env.name),
+            '  To remove it and resolve the issue, '
+            'force concretize with the command:\n',
+            '    spack -e {0} concretize --force'.format(env.name))
+
+    return env_mods
+
+
+def deactivate():
+    """
+    Deactivate an environment and collect corresponding environment modifications
+
+    Returns:
+        spack.util.environment.EnvironmentModifications: Environment variables
+        modifications to activate environment.
+    """
+    env_mods = EnvironmentModifications()
+    active = ev.active_environment()
+
+    if active is None:
+        return env_mods
+
+    if ev.default_view_name in active.views:
+        try:
+            with spack.store.db.read_transaction():
+                active.rm_default_view_from_env(env_mods)
+        except (spack.repo.UnknownPackageError,
+                spack.repo.UnknownNamespaceError) as e:
+            tty.warn(e)
+            tty.warn('Could not fully deactivate view due to missing package '
+                     'or repo, shell environment may be corrupt.')
+
+    ev.deactivate()
+
+    return env_mods
diff --git a/lib/spack/spack/main.py b/lib/spack/spack/main.py
index 8ead8c8f4e..b0c665df0d 100644
--- a/lib/spack/spack/main.py
+++ b/lib/spack/spack/main.py
@@ -745,7 +745,7 @@ def main(argv=None):
     if not args.no_env:
         env = spack.cmd.find_environment(args)
         if env:
-            ev.activate(env, args.use_env_repo, add_view=False)
+            ev.activate(env, args.use_env_repo)
 
     if args.print_shell_vars:
         print_setup_info(*args.print_shell_vars.split(','))
diff --git a/lib/spack/spack/subprocess_context.py b/lib/spack/spack/subprocess_context.py
index bfba6b0acc..9e3e3581f6 100644
--- a/lib/spack/spack/subprocess_context.py
+++ b/lib/spack/spack/subprocess_context.py
@@ -22,6 +22,10 @@ from types import ModuleType
 
 import spack.architecture
 import spack.config
+import spack.environment
+import spack.main
+import spack.repo
+import spack.store
 
 _serialize = sys.version_info >= (3, 8) and sys.platform == 'darwin'
 
@@ -63,26 +67,23 @@ class PackageInstallContext(object):
     needs to be transmitted to a child process.
     """
     def __init__(self, pkg):
-        import spack.environment as ev  # break import cycle
         if _serialize:
             self.serialized_pkg = serialize(pkg)
-            self.serialized_env = serialize(ev._active_environment)
+            self.serialized_env = serialize(spack.environment.active_environment())
         else:
             self.pkg = pkg
-            self.env = ev._active_environment
+            self.env = spack.environment.active_environment()
         self.spack_working_dir = spack.main.spack_working_dir
         self.test_state = TestState()
 
     def restore(self):
-        import spack.environment as ev  # break import cycle
         self.test_state.restore()
         spack.main.spack_working_dir = self.spack_working_dir
-        if _serialize:
-            ev._active_environment = pickle.load(self.serialized_env)
-            return pickle.load(self.serialized_pkg)
-        else:
-            ev._active_environment = self.env
-            return self.pkg
+        env = pickle.load(self.serialized_env) if _serialize else self.env
+        pkg = pickle.load(self.serialized_pkg) if _serialize else self.pkg
+        if env:
+            spack.environment.activate(env)
+        return pkg
 
 
 class TestState(object):
diff --git a/lib/spack/spack/test/cmd/ci.py b/lib/spack/spack/test/cmd/ci.py
index d596e71401..3feef4443f 100644
--- a/lib/spack/spack/test/cmd/ci.py
+++ b/lib/spack/spack/test/cmd/ci.py
@@ -54,13 +54,6 @@ def project_dir_env():
         os.environ.pop('CI_PROJECT_DIR')
 
 
-@pytest.fixture()
-def env_deactivate():
-    yield
-    ev._active_environment = None
-    os.environ.pop('SPACK_ENV', None)
-
-
 def set_env_var(key, val):
     os.environ[key] = val
 
@@ -124,7 +117,7 @@ and then 'd', 'b', and 'a' to be put in the next three stages, respectively.
         assert (spec_a_label in stages[3])
 
 
-def test_ci_generate_with_env(tmpdir, mutable_mock_env_path, env_deactivate,
+def test_ci_generate_with_env(tmpdir, mutable_mock_env_path,
                               install_mockery, mock_packages, project_dir_env):
     """Make sure we can get a .gitlab-ci.yml from an environment file
        which has the gitlab-ci, cdash, and mirrors sections."""
@@ -220,7 +213,7 @@ def _validate_needs_graph(yaml_contents, needs_graph, artifacts):
 
 
 def test_ci_generate_bootstrap_gcc(tmpdir, mutable_mock_env_path,
-                                   env_deactivate, install_mockery,
+                                   install_mockery,
                                    mock_packages, project_dir_env):
     """Test that we can bootstrap a compiler and use it as the
     compiler for a spec in the environment"""
@@ -283,7 +276,6 @@ spack:
 
 def test_ci_generate_bootstrap_artifacts_buildcache(tmpdir,
                                                     mutable_mock_env_path,
-                                                    env_deactivate,
                                                     install_mockery,
                                                     mock_packages,
                                                     project_dir_env):
@@ -350,7 +342,7 @@ spack:
 
 
 def test_ci_generate_with_env_missing_section(tmpdir, mutable_mock_env_path,
-                                              env_deactivate, install_mockery,
+                                              install_mockery,
                                               mock_packages, project_dir_env):
     """Make sure we get a reasonable message if we omit gitlab-ci section"""
     project_dir_env(tmpdir.strpath)
@@ -375,7 +367,7 @@ spack:
 
 
 def test_ci_generate_with_cdash_token(tmpdir, mutable_mock_env_path,
-                                      env_deactivate, install_mockery,
+                                      install_mockery,
                                       mock_packages, project_dir_env):
     """Make sure we it doesn't break if we configure cdash"""
     project_dir_env(tmpdir.strpath)
@@ -430,7 +422,7 @@ spack:
 
 
 def test_ci_generate_with_custom_scripts(tmpdir, mutable_mock_env_path,
-                                         env_deactivate, install_mockery,
+                                         install_mockery,
                                          mock_packages, monkeypatch,
                                          project_dir_env):
     """Test use of user-provided scripts"""
@@ -521,7 +513,7 @@ spack:
 
 
 def test_ci_generate_pkg_with_deps(tmpdir, mutable_mock_env_path,
-                                   env_deactivate, install_mockery,
+                                   install_mockery,
                                    mock_packages, project_dir_env):
     """Test pipeline generation for a package w/ dependencies"""
     project_dir_env(tmpdir.strpath)
@@ -575,7 +567,7 @@ spack:
 
 
 def test_ci_generate_for_pr_pipeline(tmpdir, mutable_mock_env_path,
-                                     env_deactivate, install_mockery,
+                                     install_mockery,
                                      mock_packages, monkeypatch,
                                      project_dir_env):
     """Test that PR pipelines do not include a final stage job for
@@ -639,7 +631,7 @@ spack:
 
 
 def test_ci_generate_with_external_pkg(tmpdir, mutable_mock_env_path,
-                                       env_deactivate, install_mockery,
+                                       install_mockery,
                                        mock_packages, monkeypatch,
                                        project_dir_env):
     """Make sure we do not generate jobs for external pkgs"""
@@ -682,7 +674,7 @@ spack:
         assert not any('externaltool' in key for key in yaml_contents)
 
 
-def test_ci_rebuild(tmpdir, mutable_mock_env_path, env_deactivate,
+def test_ci_rebuild(tmpdir, mutable_mock_env_path,
                     install_mockery, mock_packages, monkeypatch,
                     mock_gnupghome, mock_fetch, project_dir_env):
     project_dir_env(tmpdir.strpath)
@@ -840,7 +832,7 @@ spack:
         env_cmd('deactivate')
 
 
-def test_ci_nothing_to_rebuild(tmpdir, mutable_mock_env_path, env_deactivate,
+def test_ci_nothing_to_rebuild(tmpdir, mutable_mock_env_path,
                                install_mockery, mock_packages, monkeypatch,
                                mock_fetch, project_dir_env):
     project_dir_env(tmpdir.strpath)
@@ -915,7 +907,7 @@ spack:
 
 
 @pytest.mark.disable_clean_stage_check
-def test_push_mirror_contents(tmpdir, mutable_mock_env_path, env_deactivate,
+def test_push_mirror_contents(tmpdir, mutable_mock_env_path,
                               install_mockery_mutable_config, mock_packages,
                               mock_fetch, mock_stage, mock_gnupghome,
                               project_dir_env):
@@ -1086,7 +1078,7 @@ def test_push_mirror_contents_exceptions(monkeypatch, capsys):
 
 
 def test_ci_generate_override_runner_attrs(tmpdir, mutable_mock_env_path,
-                                           env_deactivate, install_mockery,
+                                           install_mockery,
                                            mock_packages, monkeypatch,
                                            project_dir_env):
     """Test that we get the behavior we want with respect to the provision
@@ -1231,7 +1223,7 @@ spack:
 
 
 def test_ci_generate_with_workarounds(tmpdir, mutable_mock_env_path,
-                                      env_deactivate, install_mockery,
+                                      install_mockery,
                                       mock_packages, monkeypatch,
                                       project_dir_env):
     """Make sure the post-processing cli workarounds do what they should"""
@@ -1282,7 +1274,7 @@ spack:
 
 
 @pytest.mark.disable_clean_stage_check
-def test_ci_rebuild_index(tmpdir, mutable_mock_env_path, env_deactivate,
+def test_ci_rebuild_index(tmpdir, mutable_mock_env_path,
                           install_mockery, mock_packages, mock_fetch,
                           mock_stage):
     working_dir = tmpdir.join('working_dir')
@@ -1336,7 +1328,7 @@ spack:
 def test_ci_generate_bootstrap_prune_dag(
         install_mockery_mutable_config, mock_packages, mock_fetch,
         mock_archive, mutable_config, monkeypatch, tmpdir,
-        mutable_mock_env_path, env_deactivate, project_dir_env):
+        mutable_mock_env_path, project_dir_env):
     """Test compiler bootstrapping with DAG pruning.  Specifically, make
        sure that if we detect the bootstrapped compiler needs to be rebuilt,
        we ensure the spec we want to build with that compiler is scheduled
@@ -1468,7 +1460,7 @@ spack:
 
 
 def test_ci_subcommands_without_mirror(tmpdir, mutable_mock_env_path,
-                                       env_deactivate, mock_packages,
+                                       mock_packages,
                                        install_mockery, project_dir_env):
     """Make sure we catch if there is not a mirror and report an error"""
     project_dir_env(tmpdir.strpath)
@@ -1546,7 +1538,7 @@ def test_ensure_only_one_temporary_storage():
 
 
 def test_ci_generate_temp_storage_url(tmpdir, mutable_mock_env_path,
-                                      env_deactivate, install_mockery,
+                                      install_mockery,
                                       mock_packages, monkeypatch,
                                       project_dir_env, mock_binary_index):
     """Verify correct behavior when using temporary-storage-url-prefix"""
@@ -1602,7 +1594,7 @@ spack:
 
 
 def test_ci_generate_read_broken_specs_url(tmpdir, mutable_mock_env_path,
-                                           env_deactivate, install_mockery,
+                                           install_mockery,
                                            mock_packages, monkeypatch,
                                            project_dir_env):
     """Verify that `broken-specs-url` works as intended"""
@@ -1658,7 +1650,7 @@ spack:
             assert(ex not in output)
 
 
-def test_ci_reproduce(tmpdir, mutable_mock_env_path, env_deactivate,
+def test_ci_reproduce(tmpdir, mutable_mock_env_path,
                       install_mockery, mock_packages, monkeypatch,
                       last_two_git_commits, project_dir_env, mock_binary_index):
     project_dir_env(tmpdir.strpath)
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 10b9dee981..3d1725a3f0 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -14,8 +14,10 @@ import llnl.util.link_tree
 
 import spack.cmd.env
 import spack.environment as ev
+import spack.environment.shell
 import spack.hash_types as ht
 import spack.modules
+import spack.repo
 import spack.util.spack_json as sjson
 from spack.cmd.env import _env_create
 from spack.main import SpackCommand, SpackCommandError
@@ -52,13 +54,6 @@ def check_viewdir_removal(viewdir):
             os.listdir(str(viewdir.join('.spack'))) == ['projections.yaml'])
 
 
-@pytest.fixture()
-def env_deactivate():
-    yield
-    ev._active_environment = None
-    os.environ.pop('SPACK_ENV', None)
-
-
 def test_add():
     e = ev.create('test')
     e.add('mpileaks')
@@ -213,8 +208,8 @@ def test_env_modifications_error_on_activate(
 
     pkg = spack.repo.path.get_pkg_class("cmake-client")
     monkeypatch.setattr(pkg, "setup_run_environment", setup_error)
-    with e:
-        pass
+
+    spack.environment.shell.activate(e)
 
     _, err = capfd.readouterr()
     assert "cmake-client had issues!" in err
@@ -230,8 +225,9 @@ def test_activate_adds_transitive_run_deps_to_path(
     with e:
         install('depends-on-run-env')
 
-    cmds = ev.activate(e)
-    assert 'DEPENDENCY_ENV_VAR=1' in cmds
+    env_variables = {}
+    spack.environment.shell.activate(e).apply_modifications(env_variables)
+    assert env_variables['DEPENDENCY_ENV_VAR'] == '1'
 
 
 def test_env_install_same_spec_twice(install_mockery, mock_fetch):
@@ -574,15 +570,11 @@ packages:
         e.install_all()
         e.write()
 
-        env_modifications = e.add_default_view_to_shell('sh')
-        individual_modifications = env_modifications.split('\n')
-
-        def path_includes_fake_prefix(cmd):
-            return 'export PATH' in cmd and str(fake_bin) in cmd
-
-        assert any(
-            path_includes_fake_prefix(cmd) for cmd in individual_modifications
-        )
+        env_mod = spack.util.environment.EnvironmentModifications()
+        e.add_default_view_to_env(env_mod)
+        env_variables = {}
+        env_mod.apply_modifications(env_variables)
+        assert str(fake_bin) in env_variables['PATH']
 
 
 def test_init_with_file_and_remove(tmpdir):
@@ -630,7 +622,7 @@ env:
                for x in e._get_environment_specs())
 
 
-def test_with_config_bad_include(env_deactivate, capfd):
+def test_with_config_bad_include(capfd):
     env_name = 'test_bad_include'
     test_config = """\
 spack:
@@ -650,6 +642,7 @@ spack:
     assert 'missing include' in err
     assert '/no/such/directory' in err
     assert 'no/such/file.yaml' in err
+    assert ev.active_environment() is None
 
 
 def test_env_with_include_config_files_same_basename():
@@ -1288,7 +1281,7 @@ def test_env_updates_view_force_remove(
 
 
 def test_env_activate_view_fails(
-        tmpdir, mock_stage, mock_fetch, install_mockery, env_deactivate):
+        tmpdir, mock_stage, mock_fetch, install_mockery):
     """Sanity check on env activate to make sure it requires shell support"""
     out = env('activate', 'test')
     assert "To set up shell support" in out
@@ -2039,8 +2032,7 @@ env:
 
 
 def test_stack_view_activate_from_default(tmpdir, mock_fetch, mock_packages,
-                                          mock_archive, install_mockery,
-                                          env_deactivate):
+                                          mock_archive, install_mockery):
     filename = str(tmpdir.join('spack.yaml'))
     viewdir = str(tmpdir.join('view'))
     with open(filename, 'w') as f:
@@ -2072,8 +2064,7 @@ env:
 
 def test_stack_view_no_activate_without_default(tmpdir, mock_fetch,
                                                 mock_packages, mock_archive,
-                                                install_mockery,
-                                                env_deactivate):
+                                                install_mockery):
     filename = str(tmpdir.join('spack.yaml'))
     viewdir = str(tmpdir.join('view'))
     with open(filename, 'w') as f:
@@ -2102,8 +2093,7 @@ env:
 
 
 def test_stack_view_multiple_views(tmpdir, mock_fetch, mock_packages,
-                                   mock_archive, install_mockery,
-                                   env_deactivate):
+                                   mock_archive, install_mockery):
     filename = str(tmpdir.join('spack.yaml'))
     default_viewdir = str(tmpdir.join('default-view'))
     combin_viewdir = str(tmpdir.join('combinatorial-view'))
@@ -2150,7 +2140,7 @@ env:
 
 
 def test_env_activate_sh_prints_shell_output(
-        tmpdir, mock_stage, mock_fetch, install_mockery, env_deactivate
+        tmpdir, mock_stage, mock_fetch, install_mockery
 ):
     """Check the shell commands output by ``spack env activate --sh``.
 
@@ -2171,7 +2161,7 @@ def test_env_activate_sh_prints_shell_output(
 
 
 def test_env_activate_csh_prints_shell_output(
-        tmpdir, mock_stage, mock_fetch, install_mockery, env_deactivate
+        tmpdir, mock_stage, mock_fetch, install_mockery
 ):
     """Check the shell commands output by ``spack env activate --csh``."""
     env('create', 'test', add_view=True)
@@ -2188,8 +2178,7 @@ def test_env_activate_csh_prints_shell_output(
 
 
 @pytest.mark.regression('12719')
-def test_env_activate_default_view_root_unconditional(env_deactivate,
-                                                      mutable_mock_env_path):
+def test_env_activate_default_view_root_unconditional(mutable_mock_env_path):
     """Check that the root of the default view in the environment is added
     to the shell unconditionally."""
     env('create', 'test', add_view=True)
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index 7ef3429383..f071d19b06 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -195,7 +195,7 @@ def no_path_access(monkeypatch):
 @pytest.fixture(scope='session', autouse=True)
 def clean_user_environment():
     spack_env_value = os.environ.pop(ev.spack_env_var, None)
-    with ev.deactivate_environment():
+    with ev.no_active_environment():
         yield
     if spack_env_value:
         os.environ[ev.spack_env_var] = spack_env_value
@@ -1386,11 +1386,11 @@ def mock_svn_repository(tmpdir_factory):
 @pytest.fixture()
 def mutable_mock_env_path(tmpdir_factory):
     """Fixture for mocking the internal spack environments directory."""
-    saved_path = ev.env_path
+    saved_path = ev.environment.env_path
     mock_path = tmpdir_factory.mktemp('mock-env-path')
-    ev.env_path = str(mock_path)
+    ev.environment.env_path = str(mock_path)
     yield mock_path
-    ev.env_path = saved_path
+    ev.environment.env_path = saved_path
 
 
 @pytest.fixture()
diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py
index 023c621445..3dfe6e90ce 100644
--- a/lib/spack/spack/test/env.py
+++ b/lib/spack/spack/test/env.py
@@ -5,6 +5,8 @@
 
 """Test environment internals without CLI"""
 
+import pytest
+
 import spack.environment as ev
 import spack.spec
 
@@ -36,3 +38,11 @@ def test_hash_change_no_rehash_concrete(tmpdir, mock_packages, config):
     assert read_in.concretized_order
     assert read_in.concretized_order[0] in read_in.specs_by_hash
     assert read_in.specs_by_hash[read_in.concretized_order[0]]._build_hash == new_hash
+
+
+def test_activate_should_require_an_env():
+    with pytest.raises(TypeError):
+        ev.activate(env='name')
+
+    with pytest.raises(TypeError):
+        ev.activate(env=None)