spack commands : refactoring of cli arguments and common utiities. Implemented suggestions on spack module loads

- Common cli arguments now are in their own module
 - Moved utilities that can be reused by different commands into spack.cmd.__init__.py
 - Modifications to `spack module loads`

lib/spack/spack/cmd/__init__.py

@@ -27,11 +27,12 @@
 import sys
 
 import llnl.util.tty as tty
-from llnl.util.lang import attr_setdefault
-
 import spack
-import spack.spec
 import spack.config
+import spack.spec
+from llnl.util.lang import *
+from llnl.util.tty.colify import *
+from llnl.util.tty.color import *
 
 #
 # Settings for commands that modify configuration
@@ -145,3 +146,97 @@ def disambiguate_spec(spec):
         tty.die(*args)
 
     return matching_specs[0]
+
+
+def ask_for_confirmation(message):
+    while True:
+        tty.msg(message + '[y/n]')
+        choice = raw_input().lower()
+        if choice == 'y':
+            break
+        elif choice == 'n':
+            raise SystemExit('Operation aborted')
+        tty.warn('Please reply either "y" or "n"')
+
+
+def gray_hash(spec, length):
+    return colorize('@K{%s}' % spec.dag_hash(length))
+
+
+def display_specs(specs, **kwargs):
+    mode = kwargs.get('mode', 'short')
+    hashes = kwargs.get('long', False)
+    namespace = kwargs.get('namespace', False)
+    flags = kwargs.get('show_flags', False)
+    variants = kwargs.get('variants', False)
+
+    hlen = 7
+    if kwargs.get('very_long', False):
+        hashes = True
+        hlen = None
+
+    nfmt = '.' if namespace else '_'
+    ffmt = '$%+' if flags else ''
+    vfmt = '$+' if variants else ''
+    format_string = '$%s$@%s%s' % (nfmt, ffmt, vfmt)
+
+    # Make a dict with specs keyed by architecture and compiler.
+    index = index_by(specs, ('architecture', 'compiler'))
+
+    # Traverse the index and print out each package
+    for i, (architecture, compiler) in enumerate(sorted(index)):
+        if i > 0:
+            print
+
+        header = "%s{%s} / %s{%s}" % (spack.spec.architecture_color,
+                                      architecture, spack.spec.compiler_color,
+                                      compiler)
+        tty.hline(colorize(header), char='-')
+
+        specs = index[(architecture, compiler)]
+        specs.sort()
+
+        abbreviated = [s.format(format_string, color=True) for s in specs]
+        if mode == 'paths':
+            # Print one spec per line along with prefix path
+            width = max(len(s) for s in abbreviated)
+            width += 2
+            format = "    %%-%ds%%s" % width
+
+            for abbrv, spec in zip(abbreviated, specs):
+                if hashes:
+                    print(gray_hash(spec, hlen), )
+                print(format % (abbrv, spec.prefix))
+
+        elif mode == 'deps':
+            for spec in specs:
+                print(spec.tree(
+                    format=format_string,
+                    color=True,
+                    indent=4,
+                    prefix=(lambda s: gray_hash(s, hlen)) if hashes else None))
+
+        elif mode == 'short':
+            # Print columns of output if not printing flags
+            if not flags:
+
+                def fmt(s):
+                    string = ""
+                    if hashes:
+                        string += gray_hash(s, hlen) + ' '
+                    string += s.format('$-%s$@%s' % (nfmt, vfmt), color=True)
+
+                    return string
+
+                colify(fmt(s) for s in specs)
+            # Print one entry per line if including flags
+            else:
+                for spec in specs:
+                    # Print the hash if necessary
+                    hsh = gray_hash(spec, hlen) + ' ' if hashes else ''
+                    print(hsh + spec.format(format_string, color=True) + '\n')
+
+        else:
+            raise ValueError(
+                "Invalid mode for display_specs: %s. Must be one of (paths,"
+                "deps, short)." % mode)  # NOQA: ignore=E501

lib/spack/spack/cmd/common/__init__.py

@@ -0,0 +1,24 @@
+##############################################################################
+# Copyright (c) 2013-2016, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/llnl/spack
+# Please also see the LICENSE file for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################

lib/spack/spack/cmd/common/arguments.py

@@ -0,0 +1,88 @@
+##############################################################################
+# Copyright (c) 2013-2016, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/llnl/spack
+# Please also see the LICENSE file for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+
+import argparse
+
+import spack.modules
+from spack.util.pattern import Bunch
+__all__ = ['add_common_arguments']
+
+_arguments = {}
+
+
+def add_common_arguments(parser, list_of_arguments):
+    for argument in list_of_arguments:
+        if argument not in _arguments:
+            message = 'Trying to add the non existing argument "{0}" to a command'
+            raise KeyError(message.format(argument))
+        x = _arguments[argument]
+        parser.add_argument(*x.flags, **x.kwargs)
+
+
+class ConstraintAction(argparse.Action):
+    """Constructs a list of specs based on a constraint given on the command line
+
+    An instance of this class is supposed to be used as an argument action in a parser.
+
+    It will read a constraint and will attach a list of matching specs to the namespace
+    """
+    qualifiers = {}
+
+    def __call__(self, parser, namespace, values, option_string=None):
+        # Query specs from command line
+        d = self.qualifiers.get(namespace.subparser_name, {})
+        specs = [s for s in spack.installed_db.query(**d)]
+        values = ' '.join(values)
+        if values:
+            specs = [x for x in specs if x.satisfies(values, strict=True)]
+        namespace.specs = specs
+
+_arguments['constraint'] = Bunch(flags=('constraint',),
+                                 kwargs={
+                                     'nargs': '*',
+                                     'help': 'Optional constraint to select a subset of installed packages',
+                                     'action': ConstraintAction
+                                 })
+
+_arguments['module_type'] = Bunch(flags=('-m', '--module-type'),
+                                  kwargs={
+                                      'help': 'Type of module files',
+                                      'default': 'tcl',
+                                      'choices': spack.modules.module_types
+                                  })
+
+_arguments['yes_to_all'] = Bunch(flags=('-y', '--yes-to-all'),
+                                 kwargs={
+                                     'action': 'store_true',
+                                     'dest': 'yes_to_all',
+                                     'help': 'Assume "yes" is the answer to every confirmation asked to the user.'
+                                 })
+
+_arguments['recurse_dependencies'] = Bunch(flags=('-r', '--dependencies'),
+                                           kwargs={
+                                               'action': 'store_true',
+                                               'dest': 'recurse_dependencies',
+                                               'help': 'Recursively traverse spec dependencies'
+                                           })

lib/spack/spack/cmd/find.py

@@ -31,7 +31,7 @@
 from llnl.util.lang import *
 from llnl.util.tty.colify import *
 from llnl.util.tty.color import *
-from llnl.util.lang import *
+from spack.cmd import display_specs
 
 description = "Find installed spack packages"
 
@@ -104,89 +104,6 @@ def setup_parser(subparser):
                            help='optional specs to filter results')
 
 
-def gray_hash(spec, length):
-    return colorize('@K{%s}' % spec.dag_hash(length))
-
-
-def display_specs(specs, **kwargs):
-    mode = kwargs.get('mode', 'short')
-    hashes = kwargs.get('long', False)
-    namespace = kwargs.get('namespace', False)
-    flags = kwargs.get('show_flags', False)
-    variants = kwargs.get('variants', False)
-
-    hlen = 7
-    if kwargs.get('very_long', False):
-        hashes = True
-        hlen = None
-
-    nfmt = '.' if namespace else '_'
-    ffmt = '$%+' if flags else ''
-    vfmt = '$+' if variants else ''
-    format_string = '$%s$@%s%s' % (nfmt, ffmt, vfmt)
-
-    # Make a dict with specs keyed by architecture and compiler.
-    index = index_by(specs, ('architecture', 'compiler'))
-
-    # Traverse the index and print out each package
-    for i, (architecture, compiler) in enumerate(sorted(index)):
-        if i > 0:
-            print
-
-        header = "%s{%s} / %s{%s}" % (spack.spec.architecture_color,
-                                      architecture, spack.spec.compiler_color,
-                                      compiler)
-        tty.hline(colorize(header), char='-')
-
-        specs = index[(architecture, compiler)]
-        specs.sort()
-
-        abbreviated = [s.format(format_string, color=True) for s in specs]
-        if mode == 'paths':
-            # Print one spec per line along with prefix path
-            width = max(len(s) for s in abbreviated)
-            width += 2
-            format = "    %%-%ds%%s" % width
-
-            for abbrv, spec in zip(abbreviated, specs):
-                if hashes:
-                    print(gray_hash(spec, hlen), )
-                print(format % (abbrv, spec.prefix))
-
-        elif mode == 'deps':
-            for spec in specs:
-                print(spec.tree(
-                    format=format_string,
-                    color=True,
-                    indent=4,
-                    prefix=(lambda s: gray_hash(s, hlen)) if hashes else None))
-
-        elif mode == 'short':
-            # Print columns of output if not printing flags
-            if not flags:
-
-                def fmt(s):
-                    string = ""
-                    if hashes:
-                        string += gray_hash(s, hlen) + ' '
-                    string += s.format('$-%s$@%s' % (nfmt, vfmt), color=True)
-
-                    return string
-
-                colify(fmt(s) for s in specs)
-            # Print one entry per line if including flags
-            else:
-                for spec in specs:
-                    # Print the hash if necessary
-                    hsh = gray_hash(spec, hlen) + ' ' if hashes else ''
-                    print(hsh + spec.format(format_string, color=True) + '\n')
-
-        else:
-            raise ValueError(
-                "Invalid mode for display_specs: %s. Must be one of (paths,"
-                "deps, short)." % mode)  # NOQA: ignore=E501
-
-
 def query_arguments(args):
     # Check arguments
     if args.explicit and args.implicit:

lib/spack/spack/cmd/module.py

@@ -24,102 +24,59 @@
 ##############################################################################
 from __future__ import print_function
 
+import collections
 import os
 import shutil
 import sys
-import collections
-import argparse
 
 import llnl.util.tty as tty
 import spack.cmd
+import spack.cmd.common.arguments as arguments
 from llnl.util.filesystem import mkdirp
 from spack.modules import module_types
-from spack.util.string import *
+#from spack.util.string import *
-
-from spack.cmd.uninstall import ask_for_confirmation
 
 description = "Manipulate module files"
 
-
+callbacks = {}
-# Qualifiers to be used when querying the db for specs
-constraint_qualifiers = {
-    'refresh': {
-        'installed': True,
-        'known': True
-    },
-    'find': {
-    },
-    'load-list':{
-    },
-    'rm': {
-    }
-}
 
 
-class ConstraintAction(argparse.Action):
+def subcommand(subparser_name):
-    qualifiers = {}
+    """Registers a function in the callbacks dictionary"""
-
+    def decorator(callback):
-    def __call__(self, parser, namespace, values, option_string=None):
+        callbacks[subparser_name] = callback
-        # Query specs from command line
+        return callback
-        d = self.qualifiers.get(namespace.subparser_name, {})
+    return decorator
-        specs = [s for s in spack.installed_db.query(**d)]
-        values = ' '.join(values)
-        if values:
-            specs = [x for x in specs if x.satisfies(values, strict=True)]
-        namespace.specs = specs
-
-# TODO : this needs better wrapping to be extracted
-ConstraintAction.qualifiers.update(constraint_qualifiers)
-
-
-def _add_common_arguments(subparser):
-    type_help = 'Type of module files'
-    subparser.add_argument('-m', '--module-type', help=type_help, default='tcl', choices=module_types)
-    constraint_help = 'Optional constraint to select a subset of installed packages'
-    subparser.add_argument('constraint', nargs='*', help=constraint_help, action=ConstraintAction)
 
 
 def setup_parser(subparser):
     sp = subparser.add_subparsers(metavar='SUBCOMMAND', dest='subparser_name')
     # spack module refresh
-    refresh_parser = sp.add_parser('refresh', help='Regenerate all module files.')
+    refresh_parser = sp.add_parser('refresh', help='Regenerate module files')
     refresh_parser.add_argument('--delete-tree', help='Delete the module file tree before refresh', action='store_true')
-    _add_common_arguments(refresh_parser)
+    arguments.add_common_arguments(refresh_parser, ['constraint', 'module_type', 'yes_to_all'])
-    refresh_parser.add_argument(
-        '-y', '--yes-to-all', action='store_true', dest='yes_to_all',
-        help='Assume "yes" is the answer to every confirmation asked to the user.'
-    )
 
     # spack module find
-    find_parser = sp.add_parser('find', help='Find module files for packages.')
+    find_parser = sp.add_parser('find', help='Find module files for packages')
-    _add_common_arguments(find_parser)
+    arguments.add_common_arguments(find_parser, ['constraint', 'module_type'])
 
     # spack module rm
-    rm_parser = sp.add_parser('rm', help='Find module files for packages.')
+    rm_parser = sp.add_parser('rm', help='Remove module files')
-    _add_common_arguments(rm_parser)
+    arguments.add_common_arguments(rm_parser, ['constraint', 'module_type', 'yes_to_all'])
-    rm_parser.add_argument(
+
-        '-y', '--yes-to-all', action='store_true', dest='yes_to_all',
+    # spack module loads
-        help='Assume "yes" is the answer to every confirmation asked to the user.'
+    loads_parser = sp.add_parser(
+        'loads',
+        help='Prompt the list of modules associated with packages that satisfy a constraint'
     )
+    loads_parser.add_argument(
+        '--input-only', action='store_false', dest='shell',
+        help='Generate input for module command (instead of a shell script)')
 
-    # spack module load-list
+    loads_parser.add_argument(
-    loadlist_parser = sp.add_parser(
-        'load-list',
-        help='Prompt the list of modules associated with packages that satisfy a contraint'
-    )
-    loadlist_parser.add_argument(
-        '-d', '--dependencies', action='store_true',
-        dest='recurse_dependencies',
-        help='Recursively traverse spec dependencies')
-
-    loadlist_parser.add_argument(
-        '-s', '--shell', action='store_true', dest='shell',
-        help='Generate shell script (instead of input for module command)')
-
-    loadlist_parser.add_argument(
         '-p', '--prefix', dest='prefix', default='',
         help='Prepend to module names when issuing module load commands')
-    _add_common_arguments(loadlist_parser)
+    arguments.add_common_arguments(loads_parser, ['constraint', 'module_type', 'recurse_dependencies'])
 
 
 class MultipleMatches(Exception):
@@ -129,8 +86,8 @@ class MultipleMatches(Exception):
 class NoMatch(Exception):
     pass
 
-
+@subcommand('loads')
-def load_list(mtype, specs, args):
+def loads(mtype, specs, args):
     # Get a comprehensive list of specs
     if args.recurse_dependencies:
         specs_from_user_constraint = specs[:]
@@ -166,7 +123,7 @@ def load_list(mtype, specs, args):
         d['name'] = mod
         print(prompt_template.format(**d))
 
-
+@subcommand('find')
 def find(mtype, specs, args):
     """
     Look at all installed packages and see if the spec provided
@@ -187,9 +144,11 @@ def find(mtype, specs, args):
     print(mod.use_name)
 
 
+@subcommand('rm')
 def rm(mtype, specs, args):
     module_cls = module_types[mtype]
-    modules = [module_cls(spec) for spec in specs if os.path.exists(module_cls(spec).file_name)]
+    specs_with_modules = [spec for spec in specs if os.path.exists(module_cls(spec).file_name)]
+    modules = [module_cls(spec) for spec in specs_with_modules]
 
     if not modules:
         tty.msg('No module file matches your query')
@@ -197,21 +156,20 @@ def rm(mtype, specs, args):
 
     # Ask for confirmation
     if not args.yes_to_all:
-        tty.msg('You are about to remove the following module files:\n')
+        tty.msg('You are about to remove {0} module files the following specs:\n'.format(mtype))
-        for s in modules:
+        spack.cmd.display_specs(specs_with_modules, long=True)
-            print(s.file_name)
         print('')
-        ask_for_confirmation('Do you want to proceed ? ')
+        spack.cmd.ask_for_confirmation('Do you want to proceed ? ')
 
     # Remove the module files
     for s in modules:
         s.remove()
 
 
+@subcommand('refresh')
 def refresh(mtype, specs, args):
-    """
+    """Regenerate module files for installed packages
-    Regenerate all module files for installed packages known to
+
-    spack (some packages may no longer exist).
     """
     # Prompt a message to the user about what is going to change
     if not specs:
@@ -219,11 +177,10 @@ def refresh(mtype, specs, args):
         return
 
     if not args.yes_to_all:
-        tty.msg('You are about to regenerate the {name} module files for the following specs:\n'.format(name=mtype))
+        tty.msg('You are about to regenerate {name} module files for the following specs:\n'.format(name=mtype))
-        for s in specs:
+        spack.cmd.display_specs(specs, long=True)
-            print(s.format(color=True))
         print('')
-        ask_for_confirmation('Do you want to proceed ? ')
+        spack.cmd.ask_for_confirmation('Do you want to proceed ? ')
 
     cls = module_types[mtype]
 
@@ -252,16 +209,17 @@ def refresh(mtype, specs, args):
     for x in writers:
         x.write(overwrite=True)
 
-# Dictionary of callbacks based on the value of subparser_name
-callbacks = {
-    'refresh': refresh,
-    'find': find,
-    'load-list': load_list,
-    'rm': rm
-}
-
 
 def module(parser, args):
+    # Qualifiers to be used when querying the db for specs
+    constraint_qualifiers = {
+        'refresh': {
+            'installed': True,
+            'known': True
+        },
+    }
+    arguments.ConstraintAction.qualifiers.update(constraint_qualifiers)
+
     module_type = args.module_type
     constraint = args.constraint
     try:

lib/spack/spack/cmd/uninstall.py

@@ -30,7 +30,6 @@
 import spack
 import spack.cmd
 import spack.repository
-from spack.cmd.find import display_specs
 
 description = "Remove an installed package"
 
@@ -47,17 +46,6 @@
 }
 
 
-def ask_for_confirmation(message):
-    while True:
-        tty.msg(message + '[y/n]')
-        choice = raw_input().lower()
-        if choice == 'y':
-            break
-        elif choice == 'n':
-            raise SystemExit('Operation aborted')
-        tty.warn('Please reply either "y" or "n"')
-
-
 def setup_parser(subparser):
     subparser.add_argument(
         '-f', '--force', action='store_true', dest='force',
@@ -99,7 +87,7 @@ def concretize_specs(specs, allow_multiple_matches=False, force=False):
         if not allow_multiple_matches and len(matching) > 1:
             tty.error("%s matches multiple packages:" % spec)
             print()
-            display_specs(matching, **display_args)
+            spack.cmd.display_specs(matching, **display_args)
             print()
             has_errors = True
 
@@ -179,7 +167,7 @@ def uninstall(parser, args):
                 tty.error("Will not uninstall %s" % spec.format("$_$@$%@$#", color=True))
                 print('')
                 print("The following packages depend on it:")
-                display_specs(lst, **display_args)
+                spack.cmd.display_specs(lst, **display_args)
                 print('')
                 has_error = True
         elif args.dependents:
@@ -193,9 +181,9 @@ def uninstall(parser, args):
         if not args.yes_to_all:
             tty.msg("The following packages will be uninstalled : ")
             print('')
-            display_specs(uninstall_list, **display_args)
+            spack.cmd.display_specs(uninstall_list, **display_args)
             print('')
-            ask_for_confirmation('Do you want to proceed ? ')
+            spack.cmd.ask_for_confirmation('Do you want to proceed ? ')
 
         # Uninstall everything on the list
         do_uninstall(uninstall_list, args.force)

lib/spack/spack/test/cmd/find.py

@@ -27,11 +27,7 @@
 import spack.cmd.find
 import unittest
 
-
+from spack.util.pattern import Bunch
-class Bunch(object):
-
-    def __init__(self, **kwargs):
-        self.__dict__.update(kwargs)
 
 
 class FindTest(unittest.TestCase):

lib/spack/spack/util/pattern.py

@@ -114,3 +114,9 @@ def getter(*args, **kwargs):
         return wrapper_class
 
     return cls_decorator
+
+
+class Bunch(object):
+    """Carries a bunch of named attributes (from Alex Martelli bunch)"""
+    def __init__(self, **kwargs):
+        self.__dict__.update(kwargs)