env: make environment search more consistent; simplify code

- spack.yaml files in the current directory were picked up inconsistently
  -- make this a sure thing by moving that logic into find_environment()
  and moving find_environment() to main()

- simplify arguments to Spack command:
  - remove short args for infrequently used commands (--pdb/-D, -P, -s)
  - `spack -D` now forces an env with a directory

4 files changed, 105 insertions, 105 deletions

diff --git a/lib/spack/spack/cmd/env.py b/lib/spack/spack/cmd/env.py
index c478f3c412..b40790af71 100644
--- a/lib/spack/spack/cmd/env.py
+++ b/lib/spack/spack/cmd/env.py
@@ -62,9 +62,6 @@ def env_activate_setup_parser(subparser):
 
 
 def env_activate(args):
-    if not args.activate_env:
-        tty.die('spack env activate requires an environment name')
-
     env = args.activate_env
     if not args.shell:
         msg = [
@@ -93,6 +90,9 @@ def env_activate(args):
     else:
         tty.die("No such environment: '%s'" % env)
 
+    if spack_env == os.environ.get('SPACK_ENV'):
+        tty.die("Environment %s is already active" % args.activate_env)
+
     if args.shell == 'csh':
         # TODO: figure out how to make color work for csh
         sys.stdout.write('setenv SPACK_ENV %s;\n' % spack_env)
@@ -167,7 +167,8 @@ def env_deactivate(args):
 #
 def env_create_setup_parser(subparser):
     """create a new environment"""
-    subparser.add_argument('env', help='name of environment to create')
+    subparser.add_argument(
+        'create_env', metavar='ENV', help='name of environment to create')
     subparser.add_argument(
         '-d', '--dir', action='store_true',
         help='create an environment in a specific directory')
@@ -179,9 +180,9 @@ def env_create_setup_parser(subparser):
 def env_create(args):
     if args.envfile:
         with open(args.envfile) as f:
-            _env_create(args.env, f, args.dir)
+            _env_create(args.create_env, f, args.dir)
     else:
-        _env_create(args.env, None, args.dir)
+        _env_create(args.create_env, None, args.dir)
 
 
 def _env_create(name_or_path, init_file=None, dir=False):
@@ -211,33 +212,38 @@ def _env_create(name_or_path, init_file=None, dir=False):
 def env_remove_setup_parser(subparser):
     """remove an existing environment"""
     subparser.add_argument(
-        'env', nargs='+', help='environment(s) to remove')
+        'rm_env', metavar='ENV', nargs='+',
+        help='environment(s) to remove')
     arguments.add_common_arguments(subparser, ['yes_to_all'])
 
 
 def env_remove(args):
-    for env_name in args.env:
-        env = ev.disambiguate(env_name)
-        if not env:
-            tty.die('no such environment: %s' % env_name)
+    """Remove a *named* environment.
+
+    This removes an environment managed by Spack. Directory environments
+    and `spack.yaml` files embedded in repositories should be removed
+    manually.
+    """
+    read_envs = []
+    for env_name in args.rm_env:
+        env = ev.read(env_name)
+        read_envs.append(env)
 
     if not args.yes_to_all:
         answer = tty.get_yes_or_no(
             'Really remove %s %s?' % (
-                string.plural(len(args.env), 'environment', show_n=False),
-                string.comma_and(args.env)),
+                string.plural(len(args.rm_env), 'environment', show_n=False),
+                string.comma_and(args.rm_env)),
             default=False)
         if not answer:
             tty.die("Will not remove any environments")
 
-    for env_name in args.env:
-        env = ev.disambiguate(env_name)
-
+    for env in read_envs:
         if env.active:
             tty.die("Environment %s can't be removed while activated.")
 
         env.destroy()
-        tty.msg("Successfully removed environment '%s'" % env)
+        tty.msg("Successfully removed environment '%s'" % env.name)
 
 
 #
@@ -245,7 +251,6 @@ def env_remove(args):
 #
 def env_list_setup_parser(subparser):
     """list available environments"""
-    pass
 
 
 def env_list(args):
diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index 2a0a6c0137..cfb3cd2c28 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -131,73 +131,105 @@ def deactivate():
     active = None
 
 
-def get_env(args, cmd_name, required=True):
-    """Get target environment from args, environment variables, or spack.yaml.
+def find_environment(args):
+    """Find active environment from args, spack.yaml, or environment variable.
 
-    This is used by a number of commands for determining whether there is
-    an active environment.
+    This is called in ``spack.main`` to figure out which environment to
+    activate.
 
-    Check for an environment:
-        1. via spack -e ENV
+    Check for an environment in this order:
+        1. via ``spack -e ENV`` or ``spack -D DIR`` (arguments)
         2. as a spack.yaml file in the current directory, or
-        3. via the SPACK_ENV environment variable.
+        3. via a path in the SPACK_ENV environment variable.
 
-    If an environment is found, read it in.  If not, print an error
-    message referencing the calling command.
+    If an environment is found, read it in.  If not, return None.
 
     Arguments:
         args (Namespace): argparse namespace wtih command arguments
-        cmd_name (str): name of calling command
 
+    Returns:
+        (Environment): a found environment, or ``None``
     """
     # try arguments
     env = getattr(args, 'env', None)
 
-    # 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)
+    # treat env as a name
+    if env:
+        if exists(env):
+            return read(env)
+
+    else:
+        # if env was specified, see if it is a dirctory otherwise, look
+        # at env_dir (env and env_dir are mutually exclusive)
+        env = getattr(args, 'env_dir', None)
+
+        # if no argument, look for a manifest file
+        if not env:
+            if os.path.exists(manifest_name):
+                env = os.getcwd()
 
-    environment = disambiguate(env)
+            # if no env, env_dir, or manifest try the environment
+            if not env:
+                env = os.environ.get(spack_env_var)
 
-    if not environment:
-        tty.die('no such environment: %s' % env)
-    return environment
+                # nothing was set; there's no active environment
+                if not env:
+                    return None
+
+    # if we get here, env isn't the name of a spack environment; it has
+    # to be a path to an environment, or there is something wrong.
+    if is_env_dir(env):
+        return Environment(env)
+
+    raise SpackEnvironmentError('no environment in %s' % env)
+
+
+def get_env(args, cmd_name, required=True):
+    """Used by commands to get the active environment.
 
+    This first checks for an ``env`` argument, then looks at the
+    ``active`` environment.  We check args first because Spack's
+    subcommand arguments are parsed *after* the ``-e`` and ``-D``
+    arguments to ``spack``.  So there may be an ``env`` argument that is
+    *not* the active environment, and we give it precedence.
 
-def disambiguate(env, env_dir=None):
-    """Used to determine whether an environment is named or a directory."""
+    This is used by a number of commands for determining whether there is
+    an active environment.
+
+    If an environment is not found *and* is required, print an error
+    message that says the calling command *needs* an active environment.
+
+    Arguments:
+        args (Namespace): argparse namespace wtih command arguments
+        cmd_name (str): name of calling command
+        required (bool): if ``False``, return ``None`` if no environment
+            is found instead of raising an exception.
+
+    Returns:
+        (Environment): if there is an arg or active environment
+    """
+    # try argument first
+    env = getattr(args, 'env', None)
     if env:
         if exists(env):
-            # treat env as a name
             return read(env)
-        env_dir = env
-
-    if not env_dir:
-        env_dir = os.environ.get(spack_env_var)
-        if not env_dir:
-            return None
-
-    if os.path.isdir(env_dir):
-        if is_env_dir(env_dir):
-            return Environment(env_dir)
+        elif is_env_dir(env):
+            return Environment(env)
         else:
-            raise SpackEnvironmentError('no environment in %s' % env_dir)
-
-    return None
+            raise SpackEnvironmentError('no environment in %s' % env)
+
+    # try the active environment. This is set by find_environment() (above)
+    if active:
+        return active
+    elif not required:
+        return None
+    else:
+        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)
 
 
 def root(name):
diff --git a/lib/spack/spack/main.py b/lib/spack/spack/main.py
index 90ac30aaba..72fb30079b 100644
--- a/lib/spack/spack/main.py
+++ b/lib/spack/spack/main.py
@@ -585,31 +585,6 @@ def print_setup_info(*info):
             shell_set('_sp_module_prefix', 'not_installed')
 
 
-def activate_environment(env, env_dir, use_env_repo):
-    """Activate an environment from command line arguments or an env var."""
-
-    if env:
-        if ev.exists(env):
-            # treat env as a name
-            ev.activate(ev.read(env), use_env_repo)
-            return
-        env_dir = env
-
-    if not env_dir:
-        env_dir = os.environ.get(spack.environment.spack_env_var)
-        if not env_dir:
-            return
-
-    if os.path.isdir(env_dir):
-        if ev.is_env_dir(env_dir):
-            ev.activate(ev.Environment(env_dir), use_env_repo)
-        else:
-            tty.die('no environment in %s' % env_dir)
-        return
-
-    tty.die('no such environment: %s' % env_dir)
-
-
 def main(argv=None):
     """This is the entry point for the Spack command.
 
@@ -627,7 +602,9 @@ def main(argv=None):
 
     # activate an environment if one was specified on the command line
     if not args.no_env:
-        activate_environment(args.env, args.env_dir, args.use_env_repo)
+        env = ev.find_environment(args)
+        if env:
+            ev.activate(env, args.use_env_repo)
 
     # make spack.config aware of any command line configuration scopes
     if args.config_scopes:
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 843ea7d2e9..76210bc562 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -75,20 +75,6 @@ def test_env_remove(capfd):
     assert 'bar' not in out
 
 
-def test_remove_env_dir(capfd):
-    env('create', '-d', 'foo')
-    assert os.path.isdir('foo')
-
-    foo = ev.Environment('foo')
-    with foo:
-        with pytest.raises(spack.main.SpackCommandError):
-            with capfd.disabled():
-                env('remove', '-y', 'foo')
-
-    env('remove', '-y', './foo')
-    assert not os.path.isdir('foo')
-
-
 def test_concretize():
     e = ev.create('test')
     e.add('mpileaks')