env: moved all `spack env install` functionality into `spack install`

- moved get_env from cmd/env.py to environment.py

- spack install will now install into the active environment when no
  arguments are provided.  It looks:
  1. at the command line
  2. for a local spack.yaml file
  3. for any currently activated environment

4 files changed, 110 insertions, 124 deletions

diff --git a/lib/spack/spack/cmd/env.py b/lib/spack/spack/cmd/env.py
index c0687a5603..478b991010 100644
--- a/lib/spack/spack/cmd/env.py
+++ b/lib/spack/spack/cmd/env.py
@@ -39,46 +39,10 @@ subcommands = [
     ['status', 'st'],
     'loads',
     'stage',
-    'install',
     'uninstall',
 ]
 
 
-def get_env(args, cmd_name, fail_on_error=True):
-    """Get target environment from args, or from environment variables.
-
-    This is used by a number of commands for handling the environment
-    argument.
-
-    Check whether an environment was passed via arguments, then whether
-    it was passed via SPACK_ENV.  If an environment is found, read it in.
-    If not, print an error message referencing the calling command.
-
-    Arguments:
-        args (Namespace): argparse namespace wtih command arguments
-        cmd_name (str): name of calling command
-
-    """
-    env = args.env
-    if not env:
-        if ev.active:
-            return ev.active
-        elif not fail_on_error:
-            return None
-        tty.die(
-            '`spack env %s` requires an environment' % cmd_name,
-            'activate an environment first:',
-            '    spack env activate ENV',
-            'or use:',
-            '    spack -e ENV %s ...' % cmd_name)
-
-    environment = ev.disambiguate(env)
-
-    if not environment:
-        tty.die('no such environment: %s' % env)
-    return environment
-
-
 #
 # env activate
 #
@@ -316,7 +280,7 @@ def env_add_setup_parser(subparser):
 
 
 def env_add(args):
-    env = get_env(args, 'add')
+    env = ev.get_env(args, 'env add')
 
     for spec in spack.cmd.parse_specs(args.specs):
         if not env.add(spec):
@@ -340,7 +304,7 @@ def env_remove_setup_parser(subparser):
 
 
 def env_remove(args):
-    env = get_env(args, 'remove <spec>')
+    env = ev.get_env(args, 'env remove <spec>')
 
     if args.all:
         env.clear()
@@ -364,41 +328,10 @@ def env_concretize_setup_parser(subparser):
 
 
 def env_concretize(args):
-    env = get_env(args, 'status')
-    _env_concretize(env, use_repo=args.use_env_repo, force=args.force)
-
-
-def _env_concretize(env, use_repo=False, force=False):
-    """Function body separated out to aid in testing."""
-    env.concretize(force=force)
+    env = ev.get_env(args, 'env concretize')
+    env.concretize(force=args.force)
     env.write()
 
-
-# REMOVE
-# env install
-#
-def env_install_setup_parser(subparser):
-    """concretize and install all specs in an environment"""
-    subparser.add_argument(
-        'env', nargs='?', help='install all packages in this environment')
-    subparser.add_argument(
-        '--only-concrete', action='store_true', default=False,
-        help='only install already concretized specs')
-    spack.cmd.install.add_common_arguments(subparser)
-
-
-def env_install(args):
-    env = get_env(args, 'status')
-
-    # concretize unless otherwise specified
-    if not args.only_concrete:
-        env.concretize()
-        env.write()
-
-    # install all specs in the environment
-    env.install_all(args)
-
-
 # REMOVE
 # env uninstall
 #
@@ -410,7 +343,7 @@ def env_uninstall_setup_parser(subparser):
 
 
 def env_uninstall(args):
-    env = get_env(args, 'uninstall')
+    env = ev.get_env(args, 'env uninstall')
     env.uninstall(args)
 
 
@@ -427,7 +360,7 @@ def env_status_setup_parser(subparser):
 
 
 def env_status(args):
-    env = get_env(args, 'status', fail_on_error=False)
+    env = ev.get_env(args, 'env status', required=False)
     if not env:
         tty.msg('No active environment')
         return
@@ -450,7 +383,7 @@ def env_stage_setup_parser(subparser):
 
 
 def env_stage(args):
-    env = get_env(args, 'stage')
+    env = ev.get_env(args, 'env stage')
     for spec in env.specs_by_hash.values():
         for dep in spec.traverse():
             dep.package.do_stage()
@@ -470,7 +403,7 @@ def env_loads_setup_parser(subparser):
 
 
 def env_loads(args):
-    env = get_env(args, 'loads')
+    env = ev.get_env(args, 'env loads')
 
     # Set the module types that have been selected
     module_type = args.module_type
diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py
index a01c1a9e0a..d1aaf20f1b 100644
--- a/lib/spack/spack/cmd/install.py
+++ b/lib/spack/spack/cmd/install.py
@@ -26,41 +26,6 @@ section = "build"
 level = "short"
 
 
-def add_common_arguments(subparser):
-    arguments.add_common_arguments(subparser, ['jobs', 'install_status'])
-    subparser.add_argument(
-        '--overwrite', action='store_true',
-        help="reinstall an existing spec, even if it has dependents")
-    subparser.add_argument(
-        '--keep-prefix', action='store_true',
-        help="don't remove the install prefix if installation fails")
-    subparser.add_argument(
-        '--keep-stage', action='store_true',
-        help="don't remove the build stage if installation succeeds")
-    subparser.add_argument(
-        '--dont-restage', action='store_true',
-        help="if a partial install is detected, don't delete prior state")
-    subparser.add_argument(
-        '--use-cache', action='store_true', dest='use_cache',
-        help="check for pre-built Spack packages in mirrors")
-    subparser.add_argument(
-        '--show-log-on-error', action='store_true',
-        help="print full build log to stderr if build fails")
-    subparser.add_argument(
-        '--source', action='store_true', dest='install_source',
-        help="install source files in prefix")
-    arguments.add_common_arguments(subparser, ['no_checksum'])
-    subparser.add_argument(
-        '-v', '--verbose', action='store_true',
-        help="display verbose build output while installing")
-    subparser.add_argument(
-        '--fake', action='store_true',
-        help="fake install for debug purposes.")
-
-    cd_group = subparser.add_mutually_exclusive_group()
-    arguments.add_common_arguments(cd_group, ['clean', 'dirty'])
-
-
 def update_kwargs_from_args(args, kwargs):
     """Parse cli arguments and construct a dictionary
     that will be passed to Package.do_install API"""
@@ -86,7 +51,6 @@ def update_kwargs_from_args(args, kwargs):
 
 
 def setup_parser(subparser):
-    add_common_arguments(subparser)
     subparser.add_argument(
         '--only',
         default='package,dependencies',
@@ -97,11 +61,46 @@ the default is to install the package along with all its dependencies.
 alternatively one can decide to install only the package or only
 the dependencies"""
     )
+    arguments.add_common_arguments(subparser, ['jobs', 'install_status'])
+    subparser.add_argument(
+        '--overwrite', action='store_true',
+        help="reinstall an existing spec, even if it has dependents")
+    subparser.add_argument(
+        '--keep-prefix', action='store_true',
+        help="don't remove the install prefix if installation fails")
+    subparser.add_argument(
+        '--keep-stage', action='store_true',
+        help="don't remove the build stage if installation succeeds")
+    subparser.add_argument(
+        '--dont-restage', action='store_true',
+        help="if a partial install is detected, don't delete prior state")
+    subparser.add_argument(
+        '--use-cache', action='store_true', dest='use_cache',
+        help="check for pre-built Spack packages in mirrors")
+    subparser.add_argument(
+        '--show-log-on-error', action='store_true',
+        help="print full build log to stderr if build fails")
+    subparser.add_argument(
+        '--source', action='store_true', dest='install_source',
+        help="install source files in prefix")
+    arguments.add_common_arguments(subparser, ['no_checksum'])
+    subparser.add_argument(
+        '-v', '--verbose', action='store_true',
+        help="display verbose build output while installing")
+    subparser.add_argument(
+        '--fake', action='store_true',
+        help="fake install for debug purposes.")
+    subparser.add_argument(
+        '--only-concrete', action='store_true', default=False,
+        help='(with environment) only install already concretized specs')
     subparser.add_argument(
         '-f', '--file', action='append', default=[],
         dest='specfiles', metavar='SPEC_YAML_FILE',
         help="install from file. Read specs to install from .yaml files")
 
+    cd_group = subparser.add_mutually_exclusive_group()
+    arguments.add_common_arguments(cd_group, ['clean', 'dirty'])
+
     subparser.add_argument(
         'package',
         nargs=argparse.REMAINDER,
@@ -155,7 +154,7 @@ def install_spec(cli_args, kwargs, spec):
 
     # handle active environment, if any
     def install(spec, kwargs):
-        env = spack.environment.active
+        env = ev.get_env(cli_args, 'install', required=False)
         if env:
             env.install(spec, kwargs)
             env.write()
@@ -187,12 +186,15 @@ def install_spec(cli_args, kwargs, spec):
 
 def install(parser, args, **kwargs):
     if not args.package and not args.specfiles:
-        # if there is a spack.yaml file, then install the packages in it.
-        if os.path.exists(ev.manifest_name):
-            env = ev.Environment(os.getcwd())
-            env.concretize()
-            env.write()
-            env.install_all()
+        # if there are no args but an active environment or spack.yaml file
+        # then install the packages from it.
+        env = ev.get_env(args, 'install', required=False)
+        if env:
+            if not args.only_concrete:
+                env.concretize()
+                env.write()
+            tty.msg("Installing environment %s" % env.name)
+            env.install_all(args)
             return
         else:
             tty.die("install requires a package argument or a spack.yaml file")
diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index 1326c382b9..8ba9ac976e 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -132,6 +132,53 @@ def deactivate():
     active = None
 
 
+def get_env(args, cmd_name, required=True):
+    """Get target environment from args, environment variables, or spack.yaml.
+
+    This is used by a number of commands for determining whether there is
+    an active environment.
+
+    Check for an environment:
+        1. via spack -e ENV
+        2. as a spack.yaml file in the current directory, or
+        3. via the SPACK_ENV environment variable.
+
+    If an environment is found, read it in.  If not, print an error
+    message referencing the calling command.
+
+    Arguments:
+        args (Namespace): argparse namespace wtih command arguments
+        cmd_name (str): name of calling command
+
+    """
+    # try arguments
+    env = args.env
+
+    # try a manifest file in the current directory
+    if not env:
+        if os.path.exists(manifest_name):
+            env = os.getcwd()
+
+    # try the active environment via SPACK_ENV environment variable
+    if not env:
+        if active:
+            return active
+        elif not required:
+            return None
+        tty.die(
+            '`spack %s` requires an environment' % cmd_name,
+            'activate an environment first:',
+            '    spack env activate ENV',
+            'or use:',
+            '    spack -e ENV %s ...' % cmd_name)
+
+    environment = disambiguate(env)
+
+    if not environment:
+        tty.die('no such environment: %s' % env)
+    return environment
+
+
 def disambiguate(env, env_dir=None):
     """Used to determine whether an environment is named or a directory."""
     if env:
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 1dc6695353..87ba5249ef 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -12,7 +12,7 @@ import llnl.util.filesystem as fs
 
 import spack.modules
 import spack.environment as ev
-from spack.cmd.env import _env_concretize, _env_create
+from spack.cmd.env import _env_create
 from spack.spec import Spec
 from spack.main import SpackCommand
 
@@ -23,6 +23,7 @@ pytestmark = pytest.mark.usefixtures(
 
 
 env = SpackCommand('env')
+install = SpackCommand('install')
 
 
 def test_add():
@@ -117,7 +118,6 @@ def test_env_install_single_spec(install_mockery, mock_fetch):
 
 def test_env_install_same_spec_twice(install_mockery, mock_fetch, capfd):
     env('create', 'test')
-    install = SpackCommand('install')
 
     e = ev.read('test')
     with capfd.disabled():
@@ -193,7 +193,9 @@ def test_to_lockfile_dict():
 def test_env_repo():
     e = ev.create('test')
     e.add('mpileaks')
-    _env_concretize(e)
+    e.write()
+
+    env('concretize', 'test')
 
     package = e.repo.get('mpileaks')
     assert package.name == 'mpileaks'
@@ -486,8 +488,12 @@ def test_env_loads(install_mockery, mock_fetch):
 
     with ev.read('test'):
         env('add', 'mpileaks')
+
     env('concretize', 'test')
-    env('install', '--fake', 'test')
+
+    with ev.read('test'):
+        install('--fake')
+
     env('loads', 'test')
 
     e = ev.read('test')
@@ -536,8 +542,6 @@ def test_env_commands_die_with_no_env_arg():
     with pytest.raises(spack.main.SpackCommandError):
         env('stage')
     with pytest.raises(spack.main.SpackCommandError):
-        env('install')
-    with pytest.raises(spack.main.SpackCommandError):
         env('uninstall')
     with pytest.raises(spack.main.SpackCommandError):
         env('add')