Major improvements to spack create (#2707)

* Initial changes to spack create command

* Get 'spack create <url>' working again

* Simplify call to BuildSystemGuesser

* More verbose output of spack create

* Remove duplicated code from spack create and spack checksum

* Add better documentation to spack create docstrings

* Fix pluralization bug

* Flake8

* Update documentation on spack create and deprecate spack edit --force

* Make it more obvious when we are renaming a package

* Further deprecate spack edit --force

* Fix unit tests

* Rename default template to generic template

* Don't add automake/autoconf deps to Autotools packages

* Remove changes to default $EDITOR

* Completely remove all traces of spack edit --force

* Remove grammar changes to make the PR easier to review

1 files changed, 262 insertions, 176 deletions

diff --git a/lib/spack/spack/cmd/create.py b/lib/spack/spack/cmd/create.py
index cac67f22af..a488104282 100644
--- a/lib/spack/spack/cmd/create.py
+++ b/lib/spack/spack/cmd/create.py
@@ -26,7 +26,6 @@ from __future__ import print_function
 
 import os
 import re
-import string
 
 import llnl.util.tty as tty
 import spack
@@ -35,15 +34,14 @@ import spack.cmd.checksum
 import spack.url
 import spack.util.web
 from llnl.util.filesystem import mkdirp
-from ordereddict_backport import OrderedDict
-from spack.repository import Repo, RepoError
+from spack.repository import Repo
 from spack.spec import Spec
 from spack.util.executable import which
 from spack.util.naming import *
 
-description = "Create a new package file from an archive URL"
+description = "Create a new package file"
 
-package_template = string.Template("""\
+package_template = '''\
 ##############################################################################
 # Copyright (c) 2013-2016, Lawrence Livermore National Security, LLC.
 # Produced at the Lawrence Livermore National Laboratory.
@@ -73,11 +71,11 @@ package_template = string.Template("""\
 # next to all the things you'll want to change. Once you've handled
 # them, you can save this file and test your package like this:
 #
-#     spack install ${name}
+#     spack install {name}
 #
 # You can edit this file again by typing:
 #
-#     spack edit ${name}
+#     spack edit {name}
 #
 # See the Spack documentation for more information on packaging.
 # If you submit this package back to Spack as a pull request,
@@ -86,23 +84,24 @@ package_template = string.Template("""\
 from spack import *
 
 
-class ${class_name}(${base_class_name}):
-    ""\"FIXME: Put a proper description of your package here.""\"
+class {class_name}({base_class_name}):
+    """FIXME: Put a proper description of your package here."""
 
     # FIXME: Add a proper url for your package's homepage here.
     homepage = "http://www.example.com"
-    url      = "${url}"
+    url      = "{url}"
 
-${versions}
+{versions}
 
-${dependencies}
+{dependencies}
 
-${body}
-""")
+{body}
+'''
 
 
-class DefaultGuess(object):
+class PackageTemplate(object):
     """Provides the default values to be used for the package file template"""
+
     base_class_name = 'Package'
 
     dependencies = """\
@@ -115,57 +114,61 @@ class DefaultGuess(object):
         make()
         make('install')"""
 
-    def __init__(self, name, url, version_hash_tuples):
-        self.name = name
+    def __init__(self, name, url, versions):
+        self.name       = name
         self.class_name = mod_to_class(name)
-        self.url = url
-        self.version_hash_tuples = version_hash_tuples
+        self.url        = url
+        self.versions   = versions
+
+    def write(self, pkg_path):
+        """Writes the new package file."""
+
+        # Write out a template for the file
+        with open(pkg_path, "w") as pkg_file:
+            pkg_file.write(package_template.format(
+                name=self.name,
+                class_name=self.class_name,
+                base_class_name=self.base_class_name,
+                url=self.url,
+                versions=self.versions,
+                dependencies=self.dependencies,
+                body=self.body))
 
-    @property
-    def versions(self):
-        """Adds a version() call to the package for each version found."""
-        max_len = max(len(str(v)) for v, h in self.version_hash_tuples)
-        format = "    version(%%-%ds, '%%s')" % (max_len + 2)
-        return '\n'.join(
-            format % ("'%s'" % v, h) for v, h in self.version_hash_tuples
-        )
 
+class AutotoolsPackageTemplate(PackageTemplate):
+    """Provides appropriate overrides for Autotools-based packages"""
 
-class AutotoolsGuess(DefaultGuess):
-    """Provides appropriate overrides for autotools-based packages"""
     base_class_name = 'AutotoolsPackage'
 
     dependencies = """\
     # FIXME: Add dependencies if required.
-    # depends_on('m4', type='build')
-    # depends_on('autoconf', type='build')
-    # depends_on('automake', type='build')
-    # depends_on('libtool', type='build')
     # depends_on('foo')"""
 
     body = """\
     def configure_args(self):
         # FIXME: Add arguments other than --prefix
-        # FIXME: If not needed delete the function
+        # FIXME: If not needed delete this function
         args = []
         return args"""
 
 
-class CMakeGuess(DefaultGuess):
-    """Provides appropriate overrides for cmake-based packages"""
+class CMakePackageTemplate(PackageTemplate):
+    """Provides appropriate overrides for CMake-based packages"""
+
     base_class_name = 'CMakePackage'
 
     body = """\
     def cmake_args(self):
         # FIXME: Add arguments other than
         # FIXME: CMAKE_INSTALL_PREFIX and CMAKE_BUILD_TYPE
-        # FIXME: If not needed delete the function
+        # FIXME: If not needed delete this function
         args = []
         return args"""
 
 
-class SconsGuess(DefaultGuess):
-    """Provides appropriate overrides for scons-based packages"""
+class SconsPackageTemplate(PackageTemplate):
+    """Provides appropriate overrides for SCons-based packages"""
+
     dependencies = """\
     # FIXME: Add additional dependencies if required.
     depends_on('scons', type='build')"""
@@ -177,8 +180,9 @@ class SconsGuess(DefaultGuess):
         scons('install')"""
 
 
-class BazelGuess(DefaultGuess):
-    """Provides appropriate overrides for bazel-based packages"""
+class BazelPackageTemplate(PackageTemplate):
+    """Provides appropriate overrides for Bazel-based packages"""
+
     dependencies = """\
     # FIXME: Add additional dependencies if required.
     depends_on('bazel', type='build')"""
@@ -189,8 +193,9 @@ class BazelGuess(DefaultGuess):
         bazel()"""
 
 
-class PythonGuess(DefaultGuess):
-    """Provides appropriate overrides for python extensions"""
+class PythonPackageTemplate(PackageTemplate):
+    """Provides appropriate overrides for Python extensions"""
+
     dependencies = """\
     extends('python')
 
@@ -204,12 +209,18 @@ class PythonGuess(DefaultGuess):
         setup_py('install', '--prefix={0}'.format(prefix))"""
 
     def __init__(self, name, *args):
-        name = 'py-{0}'.format(name)
-        super(PythonGuess, self).__init__(name, *args)
+        # If the user provided `--name py-numpy`, don't rename it py-py-numpy
+        if not name.startswith('py-'):
+            # Make it more obvious that we are renaming the package
+            tty.msg("Changing package name from {0} to py-{0}".format(name))
+            name = 'py-{0}'.format(name)
 
+        super(PythonPackageTemplate, self).__init__(name, *args)
 
-class RGuess(DefaultGuess):
+
+class RPackageTemplate(PackageTemplate):
     """Provides appropriate overrides for R extensions"""
+
     dependencies = """\
     # FIXME: Add dependencies if required.
     # depends_on('r-foo', type=('build', 'run'))"""
@@ -218,12 +229,18 @@ class RGuess(DefaultGuess):
     # FIXME: Override install() if necessary."""
 
     def __init__(self, name, *args):
-        name = 'r-{0}'.format(name)
-        super(RGuess, self).__init__(name, *args)
+        # If the user provided `--name r-rcpp`, don't rename it r-r-rcpp
+        if not name.startswith('r-'):
+            # Make it more obvious that we are renaming the package
+            tty.msg("Changing package name from {0} to r-{0}".format(name))
+            name = 'r-{0}'.format(name)
 
+        super(RPackageTemplate, self).__init__(name, *args)
 
-class OctaveGuess(DefaultGuess):
+
+class OctavePackageTemplate(PackageTemplate):
     """Provides appropriate overrides for octave packages"""
+
     dependencies = """\
     extends('octave')
 
@@ -240,43 +257,58 @@ class OctaveGuess(DefaultGuess):
                    prefix, self.stage.archive_file))"""
 
     def __init__(self, name, *args):
-        name = 'octave-{0}'.format(name)
-        super(OctaveGuess, self).__init__(name, *args)
+        # If the user provided `--name octave-splines`, don't rename it
+        # octave-octave-splines
+        if not name.startswith('octave-'):
+            # Make it more obvious that we are renaming the package
+            tty.msg("Changing package name from {0} to octave-{0}".format(name))  # noqa
+            name = 'octave-{0}'.format(name)
+
+        super(OctavePackageTemplate, self).__init__(name, *args)
+
+
+templates = {
+    'autotools': AutotoolsPackageTemplate,
+    'cmake':     CMakePackageTemplate,
+    'scons':     SconsPackageTemplate,
+    'bazel':     BazelPackageTemplate,
+    'python':    PythonPackageTemplate,
+    'r':         RPackageTemplate,
+    'octave':    OctavePackageTemplate,
+    'generic':   PackageTemplate
+}
 
 
 def setup_parser(subparser):
-    subparser.add_argument('url', nargs='?', help="url of package archive")
+    subparser.add_argument(
+        'url', nargs='?',
+        help="url of package archive")
     subparser.add_argument(
         '--keep-stage', action='store_true',
         help="Don't clean up staging area when command completes.")
     subparser.add_argument(
-        '-n', '--name', dest='alternate_name', default=None, metavar='NAME',
-        help="Override the autodetected name for the created package.")
+        '-n', '--name',
+        help="name of the package to create")
+    subparser.add_argument(
+        '-t', '--template', metavar='TEMPLATE', choices=templates.keys(),
+        help="build system template to use. options: %(choices)s")
     subparser.add_argument(
-        '-r', '--repo', default=None,
+        '-r', '--repo',
         help="Path to a repository where the package should be created.")
     subparser.add_argument(
         '-N', '--namespace',
         help="Specify a namespace for the package. Must be the namespace of "
         "a repository registered with Spack.")
     subparser.add_argument(
-        '-f', '--force', action='store_true', dest='force',
+        '-f', '--force', action='store_true',
         help="Overwrite any existing package file with the same name.")
 
-    setup_parser.subparser = subparser
-
-
-class BuildSystemGuesser(object):
 
-    _choices = {
-        'autotools': AutotoolsGuess,
-        'cmake': CMakeGuess,
-        'scons': SconsGuess,
-        'bazel': BazelGuess,
-        'python': PythonGuess,
-        'r': RGuess,
-        'octave': OctaveGuess
-    }
+class BuildSystemGuesser:
+    """An instance of BuildSystemGuesser provides a callable object to be used
+    during ``spack create``. By passing this object to ``spack checksum``, we
+    can take a peek at the fetched tarball and discern the build system it uses
+    """
 
     def __call__(self, stage, url):
         """Try to guess the type of build system used by a project based on
@@ -319,65 +351,173 @@ class BuildSystemGuesser(object):
 
         # Determine the build system based on the files contained
         # in the archive.
-        build_system = 'unknown'
+        build_system = 'generic'
         for pattern, bs in clues:
             if any(re.search(pattern, l) for l in lines):
                 build_system = bs
 
         self.build_system = build_system
 
-    def make_guess(self, name, url, ver_hash_tuples):
-        cls = self._choices.get(self.build_system, DefaultGuess)
-        return cls(name, url, ver_hash_tuples)
 
+def get_name(args):
+    """Get the name of the package based on the supplied arguments.
 
-def guess_name_and_version(url, args):
-    # Try to deduce name and version of the new package from the URL
-    version = spack.url.parse_version(url)
-    if not version:
-        tty.die("Couldn't guess a version string from %s" % url)
+    If a name was provided, always use that. Otherwise, if a URL was
+    provided, extract the name from that. Otherwise, use a default.
 
-    # Try to guess a name.  If it doesn't work, allow the user to override.
-    if args.alternate_name:
-        name = args.alternate_name
-    else:
+    :param argparse.Namespace args: The arguments given to ``spack create``
+
+    :returns: The name of the package
+    :rtype: str
+    """
+
+    # Default package name
+    name = 'example'
+
+    if args.name:
+        # Use a user-supplied name if one is present
+        name = args.name
+        tty.msg("Using specified package name: '{0}'".format(name))
+    elif args.url:
+        # Try to guess the package name based on the URL
         try:
-            name = spack.url.parse_name(url, version)
+            name = spack.url.parse_name(args.url)
+            tty.msg("This looks like a URL for {0}".format(name))
         except spack.url.UndetectableNameError:
-            # Use a user-supplied name if one is present
-            tty.die("Couldn't guess a name for this package. Try running:", "",
-                    "spack create --name <name> <url>")
+            tty.die("Couldn't guess a name for this package.",
+                    "  Please report this bug. In the meantime, try running:",
+                    "  `spack create --name <name> <url>`")
 
     if not valid_fully_qualified_module_name(name):
-        tty.die("Package name can only contain A-Z, a-z, 0-9, '_' and '-'")
+        tty.die("Package name can only contain a-z, 0-9, and '-'")
+
+    return name
+
+
+def get_url(args):
+    """Get the URL to use.
+
+    Use a default URL if none is provided.
+
+    :param argparse.Namespace args: The arguments given to ``spack create``
+
+    :returns: The URL of the package
+    :rtype: str
+    """
+
+    # Default URL
+    url = 'http://www.example.com/example-1.2.3.tar.gz'
+
+    if args.url:
+        # Use a user-supplied URL if one is present
+        url = args.url
+
+    return url
+
+
+def get_versions(args, name):
+    """Returns a list of versions and hashes for a package.
+
+    Also returns a BuildSystemGuesser object.
+
+    Returns default values if no URL is provided.
+
+    :param argparse.Namespace args: The arguments given to ``spack create``
+    :param str name: The name of the package
 
-    return name, version
+    :returns: Versions and hashes, and a BuildSystemGuesser object
+    :rtype: str and BuildSystemGuesser
+    """
 
+    # Default version, hash, and guesser
+    versions = """\
+    # FIXME: Add proper versions and checksums here.
+    # version('1.2.3', '0123456789abcdef0123456789abcdef')"""
 
-def find_repository(spec, args):
-    # figure out namespace for spec
+    guesser = BuildSystemGuesser()
+
+    if args.url:
+        # Find available versions
+        url_dict = spack.util.web.find_versions_of_archive(args.url)
+
+        if not url_dict:
+            # If no versions were found, revert to what the user provided
+            version = spack.url.parse_version(args.url)
+            url_dict = {version: args.url}
+
+        versions = spack.cmd.checksum.get_checksums(
+            url_dict, name, first_stage_function=guesser,
+            keep_stage=args.keep_stage)
+
+    return versions, guesser
+
+
+def get_build_system(args, guesser):
+    """Determine the build system template.
+
+    If a template is specified, always use that. Otherwise, if a URL
+    is provided, download the tarball and peek inside to guess what
+    build system it uses. Otherwise, use a generic template by default.
+
+    :param argparse.Namespace args: The arguments given to ``spack create``
+    :param BuildSystemGuesser guesser: The first_stage_function given to \
+        ``spack checksum`` which records the build system it detects
+
+    :returns: The name of the build system template to use
+    :rtype: str
+    """
+
+    # Default template
+    template = 'generic'
+
+    if args.template:
+        # Use a user-supplied template if one is present
+        template = args.template
+        tty.msg("Using specified package template: '{0}'".format(template))
+    elif args.url:
+        # Use whatever build system the guesser detected
+        template = guesser.build_system
+        if template == 'generic':
+            tty.warn("Unable to detect a build system. "
+                     "Using a generic package template.")
+        else:
+            msg = "This package looks like it uses the {0} build system"
+            tty.msg(msg.format(template))
+
+    return template
+
+
+def get_repository(args, name):
+    """Returns a Repo object that will allow us to determine the path where
+    the new package file should be created.
+
+    :param argparse.Namespace args: The arguments given to ``spack create``
+    :param str name: The name of the package to create
+
+    :returns: A Repo object capable of determining the path to the package file
+    :rtype: Repo
+    """
+    spec = Spec(name)
+    # Figure out namespace for spec
     if spec.namespace and args.namespace and spec.namespace != args.namespace:
-        tty.die("Namespaces '%s' and '%s' do not match." % (spec.namespace,
-                                                            args.namespace))
+        tty.die("Namespaces '{0}' and '{1}' do not match.".format(
+            spec.namespace, args.namespace))
 
     if not spec.namespace and args.namespace:
         spec.namespace = args.namespace
 
-    # Figure out where the new package should live.
+    # Figure out where the new package should live
     repo_path = args.repo
     if repo_path is not None:
-        try:
-            repo = Repo(repo_path)
-            if spec.namespace and spec.namespace != repo.namespace:
-                tty.die("Can't create package with namespace %s in repo with "
-                        "namespace %s" % (spec.namespace, repo.namespace))
-        except RepoError as e:
-            tty.die(str(e))
+        repo = Repo(repo_path)
+        if spec.namespace and spec.namespace != repo.namespace:
+            tty.die("Can't create package with namespace {0} in repo with "
+                    "namespace {0}".format(spec.namespace, repo.namespace))
     else:
         if spec.namespace:
             repo = spack.repo.get_repo(spec.namespace, None)
             if not repo:
-                tty.die("Unknown namespace: %s" % spec.namespace)
+                tty.die("Unknown namespace: '{0}'".format(spec.namespace))
         else:
             repo = spack.repo.first_repo()
 
@@ -388,84 +528,30 @@ def find_repository(spec, args):
     return repo
 
 
-def fetch_tarballs(url, name, version):
-    """Try to find versions of the supplied archive by scraping the web.
-    Prompts the user to select how many to download if many are found."""
-    versions = spack.util.web.find_versions_of_archive(url)
-    rkeys = sorted(versions.keys(), reverse=True)
-    versions = OrderedDict(zip(rkeys, (versions[v] for v in rkeys)))
-
-    archives_to_fetch = 1
-    if not versions:
-        # If the fetch failed for some reason, revert to what the user provided
-        versions = {version: url}
-    elif len(versions) > 1:
-        tty.msg("Found %s versions of %s:" % (len(versions), name),
-                *spack.cmd.elide_list(
-                    ["%-10s%s" % (v, u) for v, u in versions.iteritems()]))
-        print('')
-        archives_to_fetch = tty.get_number(
-            "Include how many checksums in the package file?",
-            default=5, abort='q')
-
-        if not archives_to_fetch:
-            tty.die("Aborted.")
-
-    sorted_versions = sorted(versions.keys(), reverse=True)
-    sorted_urls = [versions[v] for v in sorted_versions]
-    return sorted_versions[:archives_to_fetch], sorted_urls[:archives_to_fetch]
-
-
 def create(parser, args):
-    url = args.url
-    if not url:
-        setup_parser.subparser.print_help()
-        return
-
-    # Figure out a name and repo for the package.
-    name, version = guess_name_and_version(url, args)
-    spec = Spec(name)
-    repo = find_repository(spec, args)
-
-    tty.msg("This looks like a URL for %s version %s" % (name, version))
-    tty.msg("Creating template for package %s" % name)
-
-    # Fetch tarballs (prompting user if necessary)
-    versions, urls = fetch_tarballs(url, name, version)
-
-    # Try to guess what build system is used.
-    guesser = BuildSystemGuesser()
-    ver_hash_tuples = spack.cmd.checksum.get_checksums(
-        versions, urls,
-        first_stage_function=guesser,
-        keep_stage=args.keep_stage)
-
-    if not ver_hash_tuples:
-        tty.die("Could not fetch any tarballs for %s" % name)
-
-    guess = guesser.make_guess(name, url, ver_hash_tuples)
-
-    # Create a directory for the new package.
-    pkg_path = repo.filename_for_package_name(guess.name)
+    # Gather information about the package to be created
+    name = get_name(args)
+    url = get_url(args)
+    versions, guesser = get_versions(args, name)
+    build_system = get_build_system(args, guesser)
+
+    # Create the package template object
+    PackageClass = templates[build_system]
+    package = PackageClass(name, url, versions)
+    tty.msg("Created template for {0} package".format(package.name))
+
+    # Create a directory for the new package
+    repo = get_repository(args, name)
+    pkg_path = repo.filename_for_package_name(package.name)
     if os.path.exists(pkg_path) and not args.force:
-        tty.die("%s already exists." % pkg_path)
+        tty.die('{0} already exists.'.format(pkg_path),
+                '  Try running `spack create --force` to overwrite it.')
     else:
         mkdirp(os.path.dirname(pkg_path))
 
-    # Write out a template for the file
-    with open(pkg_path, "w") as pkg_file:
-        pkg_file.write(
-            package_template.substitute(
-                name=guess.name,
-                class_name=guess.class_name,
-                base_class_name=guess.base_class_name,
-                url=guess.url,
-                versions=guess.versions,
-                dependencies=guess.dependencies,
-                body=guess.body
-            )
-        )
-
-    # If everything checks out, go ahead and edit.
+    # Write the new package file
+    package.write(pkg_path)
+    tty.msg("Created package file: {0}".format(pkg_path))
+
+    # Open up the new package file in your $EDITOR
     spack.editor(pkg_path)
-    tty.msg("Created package %s" % pkg_path)