Spack BundlePackage: a group of other packages (#11981)

This adds a special package type to Spack which is used to aggregate
a set of packages that a user might commonly install together; it
does not include any source code itself and does not require a
download URL like other Spack packages. It may include an 'install'
method to generate scripts, and Spack will run post-install hooks
(including module generation).

* Add new BundlePackage type
* Update the Xsdk package to be a BundlePackage and remove the
  'install' method (previously it had a noop install method)
* "spack create --template" now takes "bundle" as an option
* Rename cmd_create_repo fixture to "mock_test_repo" and relocate it
  to shared pytest fixtures
* Add unit tests for BundlePackage behavior

lib/spack/spack/cmd/create.py

@@ -11,7 +11,6 @@
 import llnl.util.tty as tty
 from llnl.util.filesystem import mkdirp
 
-import spack.cmd
 import spack.util.web
 import spack.repo
 from spack.spec import Spec
@@ -58,35 +57,33 @@ class {class_name}({base_class_name}):
 
     # FIXME: Add a proper url for your package's homepage here.
     homepage = "http://www.example.com"
-    url      = "{url}"
+{url_def}
 
 {versions}
 
 {dependencies}
 
-{body}
+{body_def}
 '''
 
 
-class PackageTemplate(object):
+class BundlePackageTemplate(object):
-    """Provides the default values to be used for the package file template"""
+    """
+    Provides the default values to be used for a bundle package file template.
+    """
 
-    base_class_name = 'Package'
+    base_class_name = 'BundlePackage'
 
     dependencies = """\
     # FIXME: Add dependencies if required.
     # depends_on('foo')"""
 
-    body = """\
+    url_def = "    # There is no URL since there is no code to download."
-    def install(self, spec, prefix):
+    body_def = "    # There is no need for install() since there is no code."
-        # FIXME: Unknown build system
-        make()
-        make('install')"""
 
-    def __init__(self, name, url, versions):
+    def __init__(self, name, versions):
         self.name       = name
         self.class_name = mod_to_class(name)
-        self.url        = url
         self.versions   = versions
 
     def write(self, pkg_path):
@@ -98,10 +95,29 @@ def write(self, pkg_path):
                 name=self.name,
                 class_name=self.class_name,
                 base_class_name=self.base_class_name,
-                url=self.url,
+                url_def=self.url_def,
                 versions=self.versions,
                 dependencies=self.dependencies,
-                body=self.body))
+                body_def=self.body_def))
+
+
+class PackageTemplate(BundlePackageTemplate):
+    """Provides the default values to be used for the package file template"""
+
+    base_class_name = 'Package'
+
+    body_def = """\
+    def install(self, spec, prefix):
+        # FIXME: Unknown build system
+        make()
+        make('install')"""
+
+    url_line = """    url      = \"{url}\""""
+
+    def __init__(self, name, url, versions):
+        super(PackageTemplate, self).__init__(name, versions)
+
+        self.url_def = self.url_line.format(url=url)
 
 
 class AutotoolsPackageTemplate(PackageTemplate):
@@ -376,6 +392,7 @@ def __init__(self, name, *args):
     'autotools':  AutotoolsPackageTemplate,
     'autoreconf': AutoreconfPackageTemplate,
     'cmake':      CMakePackageTemplate,
+    'bundle':     BundlePackageTemplate,
     'qmake':      QMakePackageTemplate,
     'scons':      SconsPackageTemplate,
     'waf':        WafPackageTemplate,
@@ -438,7 +455,7 @@ def __call__(self, stage, url):
         # Most octave extensions are hosted on Octave-Forge:
         #     http://octave.sourceforge.net/index.html
         # They all have the same base URL.
-        if 'downloads.sourceforge.net/octave/' in url:
+        if url is not None and 'downloads.sourceforge.net/octave/' in url:
             self.build_system = 'octave'
             return
 
@@ -507,15 +524,22 @@ def get_name(args):
     # Default package name
     name = 'example'
 
-    if args.name:
+    if args.name is not None:
         # Use a user-supplied name if one is present
         name = args.name
-        tty.msg("Using specified package name: '{0}'".format(name))
+        if len(args.name.strip()) > 0:
-    elif args.url:
+            tty.msg("Using specified package name: '{0}'".format(name))
+        else:
+            tty.die("A package name must be provided when using the option.")
+    elif args.url is not None:
         # Try to guess the package name based on the URL
         try:
             name = parse_name(args.url)
-            tty.msg("This looks like a URL for {0}".format(name))
+            if name != args.url:
+                desc = 'URL'
+            else:
+                desc = 'package name'
+            tty.msg("This looks like a {0} for {1}".format(desc, name))
         except UndetectableNameError:
             tty.die("Couldn't guess a name for this package.",
                     "  Please report this bug. In the meantime, try running:",
@@ -567,21 +591,27 @@ def get_versions(args, name):
             BuildSystemGuesser object
     """
 
-    # Default version, hash, and guesser
+    # Default version with hash
-    versions = """\
+    hashed_versions = """\
     # FIXME: Add proper versions and checksums here.
     # version('1.2.3', '0123456789abcdef0123456789abcdef')"""
 
+    # Default version without hash
+    unhashed_versions = """\
+    # FIXME: Add proper versions here.
+    # version('1.2.4')"""
+
+    # Default guesser
     guesser = BuildSystemGuesser()
 
-    if args.url:
+    if args.url is not None and args.template != 'bundle':
         # Find available versions
         try:
             url_dict = spack.util.web.find_versions_of_archive(args.url)
         except UndetectableVersionError:
             # Use fake versions
             tty.warn("Couldn't detect version in: {0}".format(args.url))
-            return versions, guesser
+            return hashed_versions, guesser
 
         if not url_dict:
             # If no versions were found, revert to what the user provided
@@ -591,6 +621,8 @@ def get_versions(args, name):
         versions = spack.util.web.get_checksums_for_versions(
             url_dict, name, first_stage_function=guesser,
             keep_stage=args.keep_stage)
+    else:
+        versions = unhashed_versions
 
     return versions, guesser
 
@@ -610,15 +642,14 @@ def get_build_system(args, guesser):
     Returns:
         str: The name of the build system template to use
     """
-
     # Default template
     template = 'generic'
 
-    if args.template:
+    if args.template is not None:
         # Use a user-supplied template if one is present
         template = args.template
         tty.msg("Using specified package template: '{0}'".format(template))
-    elif args.url:
+    elif args.url is not None:
         # Use whatever build system the guesser detected
         template = guesser.build_system
         if template == 'generic':
@@ -681,8 +712,11 @@ def create(parser, args):
     build_system = get_build_system(args, guesser)
 
     # Create the package template object
+    constr_args = {'name': name, 'versions': versions}
     package_class = templates[build_system]
-    package = package_class(name, url, versions)
+    if package_class != BundlePackageTemplate:
+        constr_args['url'] = url
+    package = package_class(**constr_args)
     tty.msg("Created template for {0} package".format(package.name))
 
     # Create a directory for the new package

lib/spack/spack/cmd/info.py

@@ -174,16 +174,19 @@ def print_text_info(pkg):
                             not v.isdevelop(),
                             v)
         preferred = sorted(pkg.versions, key=key_fn).pop()
+        url = ''
+        if pkg.has_code:
+            url = fs.for_package_version(pkg, preferred)
 
-        f = fs.for_package_version(pkg, preferred)
+        line = version('    {0}'.format(pad(preferred))) + color.cescape(url)
-        line = version('    {0}'.format(pad(preferred))) + color.cescape(f)
         color.cprint(line)
         color.cprint('')
         color.cprint(section_title('Safe versions:  '))
 
         for v in reversed(sorted(pkg.versions)):
-            f = fs.for_package_version(pkg, v)
+            if pkg.has_code:
-            line = version('    {0}'.format(pad(v))) + color.cescape(f)
+                url = fs.for_package_version(pkg, v)
+            line = version('    {0}'.format(pad(v))) + color.cescape(url)
             color.cprint(line)
 
     color.cprint('')
@@ -193,12 +196,13 @@ def print_text_info(pkg):
     for line in formatter.lines:
         color.cprint(line)
 
-    color.cprint('')
+    if hasattr(pkg, 'phases') and pkg.phases:
-    color.cprint(section_title('Installation Phases:'))
+        color.cprint('')
-    phase_str = ''
+        color.cprint(section_title('Installation Phases:'))
-    for phase in pkg.phases:
+        phase_str = ''
-        phase_str += "    {0}".format(phase)
+        for phase in pkg.phases:
-    color.cprint(phase_str)
+            phase_str += "    {0}".format(phase)
+        color.cprint(phase_str)
 
     for deptype in ('build', 'link', 'run'):
         color.cprint('')

lib/spack/spack/cmd/url.py

@@ -258,6 +258,12 @@ def inc(fstype, category, attr=None):
     for pkg in spack.repo.path.all_packages():
         npkgs += 1
 
+        if not pkg.has_code:
+            for _ in pkg.versions:
+                inc('No code', 'total')
+                nvers += 1
+            continue
+
         # look at each version
         for v, args in pkg.versions.items():
             # figure out what type of fetcher it is

lib/spack/spack/directives.py

@@ -17,13 +17,14 @@ class OpenMpi(Package):
 
 The available directives are:
 
-  * ``version``
+  * ``conflicts``
   * ``depends_on``
-  * ``provides``
   * ``extends``
   * ``patch``
-  * ``variant``
+  * ``provides``
   * ``resource``
+  * ``variant``
+  * ``version``
 
 """
 
@@ -44,7 +45,7 @@ class OpenMpi(Package):
 from spack.dependency import Dependency, default_deptype, canonical_deptype
 from spack.fetch_strategy import from_kwargs
 from spack.resource import Resource
-from spack.version import Version
+from spack.version import Version, VersionChecksumError
 
 __all__ = []
 
@@ -138,8 +139,8 @@ def __new__(cls, name, bases, attr_dict):
             cls, name, bases, attr_dict)
 
     def __init__(cls, name, bases, attr_dict):
-        # The class is being created: if it is a package we must ensure
+        # The instance is being initialized: if it is a package we must ensure
-        # that the directives are called on the class to set it up
+        # that the directives are called to set it up.
 
         if 'spack.pkg' in cls.__module__:
             # Ensure the presence of the dictionaries associated
@@ -260,16 +261,22 @@ def remove_directives(arg):
 
 @directive('versions')
 def version(ver, checksum=None, **kwargs):
-    """Adds a version and metadata describing how to fetch its source code.
+    """Adds a version and, if appropriate, metadata for fetching its code.
 
-    Metadata is stored as a dict of ``kwargs`` in the package class's
+    The ``version`` directives are aggregated into a ``versions`` dictionary
-    ``versions`` dictionary.
+    attribute with ``Version`` keys and metadata values, where the metadata
+    is stored as a dictionary of ``kwargs``.
 
-    The ``dict`` of arguments is turned into a valid fetch strategy
+    The ``dict`` of arguments is turned into a valid fetch strategy for
-    later. See ``spack.fetch_strategy.for_package_version()``.
+    code packages later. See ``spack.fetch_strategy.for_package_version()``.
     """
     def _execute_version(pkg):
-        if checksum:
+        if checksum is not None:
+            if hasattr(pkg, 'has_code') and not pkg.has_code:
+                raise VersionChecksumError(
+                    "{0}: Checksums not allowed in no-code packages"
+                    "(see '{1}' version).".format(pkg.name, ver))
+
             kwargs['checksum'] = checksum
 
         # Store kwargs for the package to later with a fetch_strategy.
@@ -451,18 +458,23 @@ def patch(url_or_filename, level=1, when=None, working_dir=".", **kwargs):
 
     """
     def _execute_patch(pkg_or_dep):
+        pkg = pkg_or_dep
+        if isinstance(pkg, Dependency):
+            pkg = pkg.pkg
+
+        if hasattr(pkg, 'has_code') and not pkg.has_code:
+            raise UnsupportedPackageDirective(
+                'Patches are not allowed in {0}: package has no code.'.
+                format(pkg.name))
+
         when_spec = make_when_spec(when)
         if not when_spec:
             return
 
-        # if this spec is identical to some other, then append this
+        # If this spec is identical to some other, then append this
         # patch to the existing list.
         cur_patches = pkg_or_dep.patches.setdefault(when_spec, [])
 
-        pkg = pkg_or_dep
-        if isinstance(pkg, Dependency):
-            pkg = pkg.pkg
-
         global _patch_order_index
         ordering_key = (pkg.name, _patch_order_index)
         _patch_order_index += 1
@@ -639,3 +651,7 @@ class CircularReferenceError(DirectiveError):
 
 class DependencyPatchError(DirectiveError):
     """Raised for errors with patching dependencies."""
+
+
+class UnsupportedPackageDirective(DirectiveError):
+    """Raised when an invalid or unsupported package directive is specified."""

lib/spack/spack/fetch_strategy.py

@@ -165,6 +165,51 @@ def matches(cls, args):
         return cls.url_attr in args
 
 
+class BundleFetchStrategy(FetchStrategy):
+    """
+    Fetch strategy associated with bundle, or no-code, packages.
+
+    Having a basic fetch strategy is a requirement for executing post-install
+    hooks.  Consequently, this class provides the API but does little more
+    than log messages.
+
+    TODO: Remove this class by refactoring resource handling and the link
+    between composite stages and composite fetch strategies (see #11981).
+    """
+    #: This is a concrete fetch strategy for no-code packages.
+    enabled = True
+
+    #: There is no associated URL keyword in ``version()`` for no-code
+    #: packages but this property is required for some strategy-related
+    #: functions (e.g., check_pkg_attributes).
+    url_attr = ''
+
+    def fetch(self):
+        tty.msg("No code to fetch.")
+        return True
+
+    def check(self):
+        tty.msg("No code to check.")
+
+    def expand(self):
+        tty.msg("No archive to expand.")
+
+    def reset(self):
+        tty.msg("No code to reset.")
+
+    def archive(self, destination):
+        tty.msg("No code to archive.")
+
+    @property
+    def cachable(self):
+        tty.msg("No code to cache.")
+        return False
+
+    def source_id(self):
+        tty.msg("No code to be uniquely identified.")
+        return ''
+
+
 @pattern.composite(interface=FetchStrategy)
 class FetchStrategyComposite(object):
     """Composite for a FetchStrategy object.
@@ -1117,6 +1162,12 @@ def _from_merged_attrs(fetcher, pkg, version):
 def for_package_version(pkg, version):
     """Determine a fetch strategy based on the arguments supplied to
        version() in the package description."""
+
+    # No-code packages have a custom fetch strategy to work around issues
+    # with resource staging.
+    if not pkg.has_code:
+        return BundleFetchStrategy()
+
     check_pkg_attributes(pkg)
 
     if not isinstance(version, Version):
@@ -1159,6 +1210,7 @@ def from_list_url(pkg):
     """If a package provides a URL which lists URLs for resources by
        version, this can can create a fetcher for a URL discovered for
        the specified package's version."""
+
     if pkg.list_url:
         try:
             versions = pkg.fetch_remote_versions()

lib/spack/spack/package.py

@@ -137,10 +137,8 @@ class PackageMeta(
     spack.directives.DirectiveMeta,
     spack.mixins.PackageMixinsMeta
 ):
-    """Conveniently transforms attributes to permit extensible phases
+    """
-
+    Package metaclass for supporting directives (e.g., depends_on) and phases
-    Iterates over the attribute 'phases' and creates / updates private
-    InstallPhase attributes in the class that is being initialized
     """
     phase_fmt = '_InstallPhase_{0}'
 
@@ -148,7 +146,14 @@ class PackageMeta(
     _InstallPhase_run_after = {}
 
     def __new__(cls, name, bases, attr_dict):
+        """
+        Instance creation is preceded by phase attribute transformations.
 
+        Conveniently transforms attributes to permit extensible phases by
+        iterating over the attribute 'phases' and creating / updating private
+        InstallPhase attributes in the class that will be initialized in
+        __init__.
+        """
         if 'phases' in attr_dict:
             # Turn the strings in 'phases' into InstallPhase instances
             # and add them as private attributes
@@ -338,11 +343,16 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
 
     ***The Package class***
 
-    A package defines how to fetch, verify (via, e.g., sha256), build,
+    At its core, a package consists of a set of software to be installed.
-    and install a piece of software.  A Package also defines what other
+    A package may focus on a piece of software and its associated software
-    packages it depends on, so that dependencies can be installed along
+    dependencies or it may simply be a set, or bundle, of software.  The
-    with the package itself.  Packages are written in pure python by
+    former requires defining how to fetch, verify (via, e.g., sha256), build,
-    users of Spack.
+    and install that software and the packages it depends on, so that
+    dependencies can be installed along with the package itself.   The latter,
+    sometimes referred to as a ``no-source`` package, requires only defining
+    the packages to be built.
+
+    Packages are written in pure Python.
 
     There are two main parts of a Spack package:
 
@@ -353,12 +363,12 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
          used as input to the concretizer.
 
       2. **Package instances**. Once instantiated, a package is
-         essentially an installer for a particular piece of
+         essentially a software installer.  Spack calls methods like
-         software. Spack calls methods like ``do_install()`` on the
+         ``do_install()`` on the ``Package`` object, and it uses those to
-         ``Package`` object, and it uses those to drive user-implemented
+         drive user-implemented methods like ``patch()``, ``install()``, and
-         methods like ``patch()``, ``install()``, and other build steps.
+         other build steps.  To install software, an instantiated package
-         To install software, An instantiated package needs a *concrete*
+         needs a *concrete* spec, which guides the behavior of the various
-         spec, which guides the behavior of the various install methods.
+         install methods.
 
     Packages are imported from repos (see ``repo.py``).
 
@@ -377,12 +387,15 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
 
        p = Package()             # Done for you by spack
 
-       p.do_fetch()              # downloads tarball from a URL
+       p.do_fetch()              # downloads tarball from a URL (or VCS)
        p.do_stage()              # expands tarball in a temp directory
        p.do_patch()              # applies patches to expanded source
        p.do_install()            # calls package's install() function
        p.do_uninstall()          # removes install directory
 
+    although packages that do not have code have nothing to fetch so omit
+    ``p.do_fetch()``.
+
     There are also some other commands that clean the build area:
 
     .. code-block:: python
@@ -392,7 +405,7 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
                                  # re-expands the archive.
 
     The convention used here is that a ``do_*`` function is intended to be
-    called internally by Spack commands (in spack.cmd).  These aren't for
+    called internally by Spack commands (in ``spack.cmd``).  These aren't for
     package writers to override, and doing so may break the functionality
     of the Package class.
 
@@ -400,7 +413,8 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
     override anything in this class.  That is not usually required.
 
     For most use cases.  Package creators typically just add attributes
-    like ``url`` and ``homepage``, or functions like ``install()``.
+    like ``homepage`` and, for a code-based package, ``url``, or functions
+    such as ``install()``.
     There are many custom ``Package`` subclasses in the
     ``spack.build_systems`` package that make things even easier for
     specific build systems.
@@ -410,6 +424,10 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
     # These are default values for instance variables.
     #
 
+    #: Most Spack packages are used to install source or binary code while
+    #: those that do not can be used to install a set of other Spack packages.
+    has_code = True
+
     #: By default we build in parallel.  Subclasses can override this.
     parallel = True
 
@@ -482,7 +500,7 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
     #: Do not include @ here in order not to unnecessarily ping the users.
     maintainers = []
 
-    #: List of attributes which affect do not affect a package's content.
+    #: List of attributes to be excluded from a package's hash.
     metadata_attrs = ['homepage', 'url', 'list_url', 'extendable', 'parallel',
                       'make_jobs']
 
@@ -497,21 +515,6 @@ def __init__(self, spec):
         # a binary cache.
         self.installed_from_binary_cache = False
 
-        # Check versions in the versions dict.
-        for v in self.versions:
-            assert (isinstance(v, Version))
-
-        # Check version descriptors
-        for v in sorted(self.versions):
-            assert (isinstance(self.versions[v], dict))
-
-        # Version-ize the keys in versions dict
-        try:
-            self.versions = dict((Version(v), h)
-                                 for v, h in self.versions.items())
-        except ValueError as e:
-            raise ValueError("In package %s: %s" % (self.name, e.message))
-
         # Set a default list URL (place to find available versions)
         if not hasattr(self, 'list_url'):
             self.list_url = None
@@ -1032,6 +1035,9 @@ def do_fetch(self, mirror_only=False):
         if not self.spec.concrete:
             raise ValueError("Can only fetch concrete packages.")
 
+        if not self.has_code:
+            raise InvalidPackageOpError("Can only fetch a package with a URL.")
+
         start_time = time.time()
         checksum = spack.config.get('config:checksum')
         if checksum and self.version not in self.versions:
@@ -1069,11 +1075,20 @@ def do_stage(self, mirror_only=False):
         if not self.spec.concrete:
             raise ValueError("Can only stage concrete packages.")
 
-        self.do_fetch(mirror_only)     # this will create the stage
+        # Always create the stage directory at this point.  Why?  A no-code
-        self.stage.expand_archive()
+        # package may want to use the installation process to install metadata.
+        self.stage.create()
 
-        if not os.listdir(self.stage.path):
+        # Fetch/expand any associated code.
-            raise FetchError("Archive was empty for %s" % self.name)
+        if self.has_code:
+            self.do_fetch(mirror_only)
+            self.stage.expand_archive()
+
+            if not os.listdir(self.stage.path):
+                raise FetchError("Archive was empty for %s" % self.name)
+        else:
+            # Support for post-install hooks requires a stage.source_path
+            mkdirp(self.stage.source_path)
 
     def do_patch(self):
         """Applies patches if they haven't been applied already."""
@@ -1542,7 +1557,7 @@ def do_install(self, **kwargs):
                     Package._install_bootstrap_compiler(dep.package, **kwargs)
                 dep.package.do_install(**dep_kwargs)
 
-        # Then, install the compiler if it is not already installed.
+        # Then install the compiler if it is not already installed.
         if install_deps:
             Package._install_bootstrap_compiler(self, **kwargs)
 
@@ -1674,8 +1689,8 @@ def build_process():
                 # Ensure the metadata path exists as well
                 mkdirp(spack.store.layout.metadata_path(self.spec), mode=perms)
 
-            # Fork a child to do the actual installation
+            # Fork a child to do the actual installation.
-            # we preserve verbosity settings across installs.
+            # Preserve verbosity settings across installs.
             PackageBase._verbose = spack.build_environment.fork(
                 self, build_process, dirty=dirty, fake=fake)
 
@@ -2357,6 +2372,19 @@ def _run_default_install_time_test_callbacks(self):
 build_system_flags = PackageBase.build_system_flags
 
 
+class BundlePackage(PackageBase):
+    """General purpose bundle, or no-code, package class."""
+    #: There are no phases by default but the property is required to support
+    #: post-install hooks (e.g., for module generation).
+    phases = []
+    #: This attribute is used in UI queries that require to know which
+    #: build-system class we are using
+    build_system_class = 'BundlePackage'
+
+    #: Bundle packages do not have associated source or binary code.
+    has_code = False
+
+
 class Package(PackageBase):
     """General purpose class with a single ``install``
     phase that needs to be coded by packagers.
@@ -2528,6 +2556,10 @@ def __init__(self, cls):
             "Package %s has no version with a URL." % cls.__name__)
 
 
+class InvalidPackageOpError(PackageError):
+    """Raised when someone tries perform an invalid operation on a package."""
+
+
 class ExtensionError(PackageError):
     """Superclass for all errors having to do with extension packages."""
 

lib/spack/spack/pkgkit.py

@@ -11,7 +11,9 @@
 import llnl.util.filesystem
 from llnl.util.filesystem import *
 
-from spack.package import Package, run_before, run_after, on_package_attributes
+from spack.package import \
+    Package, BundlePackage, \
+    run_before, run_after, on_package_attributes
 from spack.package import inject_flags, env_flags, build_system_flags
 from spack.build_systems.makefile import MakefilePackage
 from spack.build_systems.aspell_dict import AspellDictPackage

lib/spack/spack/test/cmd/create.py

@@ -9,29 +9,13 @@
 
 import spack.cmd.create
 import spack.util.editor
-
+from spack.url import UndetectableNameError
 from spack.main import SpackCommand
 
 
 create = SpackCommand('create')
 
 
-@pytest.fixture("module")
-def cmd_create_repo(tmpdir_factory):
-    repo_namespace = 'cmd_create_repo'
-    repodir = tmpdir_factory.mktemp(repo_namespace)
-    repodir.ensure(spack.repo.packages_dir_name, dir=True)
-    yaml = repodir.join('repo.yaml')
-    yaml.write("""
-repo:
-    namespace: cmd_create_repo
-""")
-
-    repo = spack.repo.RepoPath(str(repodir))
-    with spack.repo.swap(repo):
-        yield repo, repodir
-
-
 @pytest.fixture(scope='module')
 def parser():
     """Returns the parser for the module"""
@@ -40,18 +24,91 @@ def parser():
     return prs
 
 
-def test_create_template(parser, cmd_create_repo):
+@pytest.mark.parametrize('args,name_index,expected', [
+    (['test-package'], 0, [r'TestPackage(Package)', r'def install(self']),
+    (['-n', 'test-named-package', 'file://example.tar.gz'], 1,
+     [r'TestNamedPackage(Package)', r'def install(self']),
+    (['file://example.tar.gz'], -1,
+     [r'Example(Package)', r'def install(self']),
+    (['-t', 'bundle', 'test-bundle'], 2, [r'TestBundle(BundlePackage)'])
+])
+def test_create_template(parser, mock_test_repo, args, name_index, expected):
     """Test template creation."""
-    repo, repodir = cmd_create_repo
+    repo, repodir = mock_test_repo
 
-    name = 'test-package'
+    constr_args = parser.parse_args(['--skip-editor'] + args)
-    args = parser.parse_args(['--skip-editor', name])
+    spack.cmd.create.create(parser, constr_args)
-    spack.cmd.create.create(parser, args)
 
-    filename = repo.filename_for_package_name(name)
+    pkg_name = args[name_index] if name_index > -1 else 'example'
+    filename = repo.filename_for_package_name(pkg_name)
     assert os.path.exists(filename)
 
     with open(filename, 'r') as package_file:
         content = ' '.join(package_file.readlines())
-        for entry in [r'TestPackage(Package)', r'def install(self']:
+        for entry in expected:
-            assert content.find(entry) > -1
+            assert entry in content
+
+
+@pytest.mark.parametrize('name,expected', [
+    (' ', 'name must be provided'),
+    ('bad#name', 'name can only contain'),
+])
+def test_create_template_bad_name(parser, mock_test_repo, name, expected):
+    """Test template creation with bad name options."""
+    constr_args = parser.parse_args(['--skip-editor', '-n', name])
+    with pytest.raises(SystemExit, matches=expected):
+        spack.cmd.create.create(parser, constr_args)
+
+
+def test_build_system_guesser_no_stage(parser):
+    """Test build system guesser when stage not provided."""
+    guesser = spack.cmd.create.BuildSystemGuesser()
+
+    # Ensure get the expected build system
+    with pytest.raises(AttributeError,
+                       matches="'NoneType' object has no attribute"):
+        guesser(None, '/the/url/does/not/matter')
+
+
+def test_build_system_guesser_octave(parser):
+    """
+    Test build system guesser for the special case, where the same base URL
+    identifies the build system rather than guessing the build system from
+    files contained in the archive.
+    """
+    url, expected = 'downloads.sourceforge.net/octave/', 'octave'
+    guesser = spack.cmd.create.BuildSystemGuesser()
+
+    # Ensure get the expected build system
+    guesser(None, url)
+    assert guesser.build_system == expected
+
+    # Also ensure get the correct template
+    args = parser.parse_args([url])
+    bs = spack.cmd.create.get_build_system(args, guesser)
+    assert bs == expected
+
+
+@pytest.mark.parametrize('url,expected', [
+    ('testname', 'testname'),
+    ('file://example.com/archive.tar.gz', 'archive'),
+])
+def test_get_name_urls(parser, url, expected):
+    """Test get_name with different URLs."""
+    args = parser.parse_args([url])
+    name = spack.cmd.create.get_name(args)
+    assert name == expected
+
+
+def test_get_name_error(parser, monkeypatch):
+    """Test get_name UndetectableNameError exception path."""
+    def _parse_name_offset(path, v):
+        raise UndetectableNameError(path)
+
+    monkeypatch.setattr(spack.url, 'parse_name_offset', _parse_name_offset)
+
+    url = 'downloads.sourceforge.net/noapp/'
+    args = parser.parse_args([url])
+
+    with pytest.raises(SystemExit, matches="Couldn't guess a name"):
+        spack.cmd.create.get_name(args)

lib/spack/spack/test/cmd/info.py

@@ -41,7 +41,8 @@ def _print(*args):
     'trilinos',
     'boost',
     'python',
-    'dealii'
+    'dealii',
+    'xsdk'  # a BundlePackage
 ])
 def test_it_just_runs(pkg):
     info(pkg)

lib/spack/spack/test/concretize.py

@@ -10,7 +10,7 @@
 import spack.concretize
 import spack.repo
 
-from spack.concretize import find_spec
+from spack.concretize import find_spec, NoValidVersionError
 from spack.spec import Spec, CompilerSpec
 from spack.spec import ConflictsInSpecError, SpecError
 from spack.version import ver
@@ -565,3 +565,9 @@ def test_simultaneous_concretization_of_specs(self, abstract_specs):
         # Make sure the concrete spec are top-level specs with no dependents
         for spec in concrete_specs:
             assert not spec.dependents()
+
+    @pytest.mark.parametrize('spec', ['noversion', 'noversion-bundle'])
+    def test_noversion_pkg(self, spec):
+        """Test concretization failures for no-version packages."""
+        with pytest.raises(NoValidVersionError, match="no valid versions"):
+            Spec(spec).concretized()

lib/spack/spack/test/conftest.py

@@ -944,3 +944,53 @@ def invalid_spec(request):
     """Specs that do not parse cleanly due to invalid formatting.
     """
     return request.param
+
+
+@pytest.fixture("module")
+def mock_test_repo(tmpdir_factory):
+    """Create an empty repository."""
+    repo_namespace = 'mock_test_repo'
+    repodir = tmpdir_factory.mktemp(repo_namespace)
+    repodir.ensure(spack.repo.packages_dir_name, dir=True)
+    yaml = repodir.join('repo.yaml')
+    yaml.write("""
+repo:
+    namespace: mock_test_repo
+""")
+
+    repo = spack.repo.RepoPath(str(repodir))
+    with spack.repo.swap(repo):
+        yield repo, repodir
+
+    shutil.rmtree(str(repodir))
+
+
+##########
+# Class and fixture to work around problems raising exceptions in directives,
+# which cause tests like test_from_list_url to hang for Python 2.x metaclass
+# processing.
+#
+# At this point only version and patch directive handling has been addressed.
+##########
+
+class MockBundle(object):
+    has_code = False
+    name = 'mock-bundle'
+    versions = {}
+
+
+@pytest.fixture
+def mock_directive_bundle():
+    """Return a mock bundle package for directive tests."""
+    return MockBundle()
+
+
+@pytest.fixture
+def clear_directive_functions():
+    """Clear all overidden directive functions for subsequent tests."""
+    yield
+
+    # Make sure any directive functions overidden by tests are cleared before
+    # proceeding with subsequent tests that may depend on the original
+    # functions.
+    spack.directives.DirectiveMeta._directives_to_be_executed = []

lib/spack/spack/test/install.py

@@ -9,7 +9,8 @@
 
 from llnl.util.filesystem import mkdirp, touch, working_dir
 
-from spack.package import InstallError, PackageBase, PackageStillNeededError
+from spack.package import \
+    InstallError, InvalidPackageOpError, PackageBase, PackageStillNeededError
 import spack.patch
 import spack.repo
 import spack.store
@@ -326,6 +327,38 @@ def test_uninstall_by_spec_errors(mutable_database):
         PackageBase.uninstall_by_spec(rec.spec)
 
 
+def test_nosource_pkg_install(install_mockery, mock_fetch, mock_packages):
+    """Test install phases with the nosource package."""
+    spec = Spec('nosource').concretized()
+    pkg = spec.package
+
+    # Make sure install works even though there is no associated code.
+    pkg.do_install()
+
+    # Also make sure an error is raised if `do_fetch` is called.
+    with pytest.raises(InvalidPackageOpError,
+                       match="fetch a package with a URL"):
+        pkg.do_fetch()
+
+
+def test_nosource_pkg_install_post_install(
+        install_mockery, mock_fetch, mock_packages):
+    """Test install phases with the nosource package with post-install."""
+    spec = Spec('nosource-install').concretized()
+    pkg = spec.package
+
+    # Make sure both the install and post-install package methods work.
+    pkg.do_install()
+
+    # Ensure the file created in the package's `install` method exists.
+    install_txt = os.path.join(spec.prefix, 'install.txt')
+    assert os.path.isfile(install_txt)
+
+    # Ensure the file created in the package's `post-install` method exists.
+    post_install_txt = os.path.join(spec.prefix, 'post-install.txt')
+    assert os.path.isfile(post_install_txt)
+
+
 def test_pkg_build_paths(install_mockery):
     # Get a basic concrete spec for the trivial install package.
     spec = Spec('trivial-install-test-package').concretized()

lib/spack/spack/test/package_sanity.py

@@ -56,7 +56,7 @@ def test_all_virtual_packages_have_default_providers():
 
 
 def test_package_version_consistency():
-    """Make sure all versions on builtin packages can produce a fetcher."""
+    """Make sure all versions on builtin packages produce a fetcher."""
     for name in spack.repo.all_package_names():
         pkg = spack.repo.get(name)
         spack.fetch_strategy.check_pkg_attributes(pkg)

lib/spack/spack/test/packages.py

@@ -12,6 +12,8 @@
 from spack.util.naming import mod_to_class
 from spack.spec import Spec
 from spack.util.package_hash import package_content
+from spack.version import VersionChecksumError
+import spack.directives
 
 
 @pytest.mark.usefixtures('config', 'mock_packages')
@@ -369,3 +371,20 @@ def test_rpath_args(mutable_database):
     rpath_args = rec.spec.package.rpath_args
     assert '-rpath' in rpath_args
     assert 'mpich' in rpath_args
+
+
+def test_bundle_version_checksum(mock_directive_bundle,
+                                 clear_directive_functions):
+    """Test raising exception on a version checksum with a bundle package."""
+    with pytest.raises(VersionChecksumError, match="Checksums not allowed"):
+        version = spack.directives.version('1.0', checksum='1badpkg')
+        version(mock_directive_bundle)
+
+
+def test_bundle_patch_directive(mock_directive_bundle,
+                                clear_directive_functions):
+    """Test raising exception on a patch directive with a bundle package."""
+    with pytest.raises(spack.directives.UnsupportedPackageDirective,
+                       match="Patches are not allowed"):
+        patch = spack.directives.patch('mock/patch.txt')
+        patch(mock_directive_bundle)

lib/spack/spack/test/url_fetch.py

@@ -85,61 +85,32 @@ def test_fetch(
             assert 'echo Building...' in contents
 
 
-def test_from_list_url(mock_packages, config):
+@pytest.mark.parametrize('spec,url,digest', [
+    ('url-list-test @0.0.0', 'foo-0.0.0.tar.gz', 'abc000'),
+    ('url-list-test @1.0.0', 'foo-1.0.0.tar.gz', 'abc100'),
+    ('url-list-test @3.0', 'foo-3.0.tar.gz', 'abc30'),
+    ('url-list-test @4.5', 'foo-4.5.tar.gz', 'abc45'),
+    ('url-list-test @2.0.0b2', 'foo-2.0.0b2.tar.gz', 'abc200b2'),
+    ('url-list-test @3.0a1', 'foo-3.0a1.tar.gz', 'abc30a1'),
+    ('url-list-test @4.5-rc5', 'foo-4.5-rc5.tar.gz', 'abc45rc5'),
+])
+def test_from_list_url(mock_packages, config, spec, url, digest):
+    """
+    Test URLs in the url-list-test package, which means they should
+    have checksums in the package.
+    """
+    specification = Spec(spec).concretized()
+    pkg = spack.repo.get(specification)
+    fetch_strategy = from_list_url(pkg)
+    assert isinstance(fetch_strategy, URLFetchStrategy)
+    assert os.path.basename(fetch_strategy.url) == url
+    assert fetch_strategy.digest == digest
+
+
+def test_from_list_url_unspecified(mock_packages, config):
+    """Test non-specific URLs from the url-list-test package."""
     pkg = spack.repo.get('url-list-test')
 
-    # These URLs are all in the url-list-test package and should have
-    # checksums taken from the package.
-    spec = Spec('url-list-test @0.0.0').concretized()
-    pkg = spack.repo.get(spec)
-    fetch_strategy = from_list_url(pkg)
-    assert isinstance(fetch_strategy, URLFetchStrategy)
-    assert os.path.basename(fetch_strategy.url) == 'foo-0.0.0.tar.gz'
-    assert fetch_strategy.digest == 'abc000'
-
-    spec = Spec('url-list-test @1.0.0').concretized()
-    pkg = spack.repo.get(spec)
-    fetch_strategy = from_list_url(pkg)
-    assert isinstance(fetch_strategy, URLFetchStrategy)
-    assert os.path.basename(fetch_strategy.url) == 'foo-1.0.0.tar.gz'
-    assert fetch_strategy.digest == 'abc100'
-
-    spec = Spec('url-list-test @3.0').concretized()
-    pkg = spack.repo.get(spec)
-    fetch_strategy = from_list_url(pkg)
-    assert isinstance(fetch_strategy, URLFetchStrategy)
-    assert os.path.basename(fetch_strategy.url) == 'foo-3.0.tar.gz'
-    assert fetch_strategy.digest == 'abc30'
-
-    spec = Spec('url-list-test @4.5').concretized()
-    pkg = spack.repo.get(spec)
-    fetch_strategy = from_list_url(pkg)
-    assert isinstance(fetch_strategy, URLFetchStrategy)
-    assert os.path.basename(fetch_strategy.url) == 'foo-4.5.tar.gz'
-    assert fetch_strategy.digest == 'abc45'
-
-    spec = Spec('url-list-test @2.0.0b2').concretized()
-    pkg = spack.repo.get(spec)
-    fetch_strategy = from_list_url(pkg)
-    assert isinstance(fetch_strategy, URLFetchStrategy)
-    assert os.path.basename(fetch_strategy.url) == 'foo-2.0.0b2.tar.gz'
-    assert fetch_strategy.digest == 'abc200b2'
-
-    spec = Spec('url-list-test @3.0a1').concretized()
-    pkg = spack.repo.get(spec)
-    fetch_strategy = from_list_url(pkg)
-    assert isinstance(fetch_strategy, URLFetchStrategy)
-    assert os.path.basename(fetch_strategy.url) == 'foo-3.0a1.tar.gz'
-    assert fetch_strategy.digest == 'abc30a1'
-
-    spec = Spec('url-list-test @4.5-rc5').concretized()
-    pkg = spack.repo.get(spec)
-    fetch_strategy = from_list_url(pkg)
-    assert isinstance(fetch_strategy, URLFetchStrategy)
-    assert os.path.basename(fetch_strategy.url) == 'foo-4.5-rc5.tar.gz'
-    assert fetch_strategy.digest == 'abc45rc5'
-
-    # this one is not in the url-list-test package.
     spec = Spec('url-list-test @2.0.0').concretized()
     pkg = spack.repo.get(spec)
     fetch_strategy = from_list_url(pkg)
@@ -148,6 +119,14 @@ def test_from_list_url(mock_packages, config):
     assert fetch_strategy.digest is None
 
 
+def test_nosource_from_list_url(mock_packages, config):
+    """This test confirms BundlePackages do not have list url."""
+    pkg = spack.repo.get('nosource')
+
+    fetch_strategy = from_list_url(pkg)
+    assert fetch_strategy is None
+
+
 def test_hash_detection(checksum_type):
     algo = crypto.hash_fun_for_algo(checksum_type)()
     h = 'f' * (algo.digest_size * 2)  # hex -> bytes

lib/spack/spack/version.py

@@ -30,6 +30,7 @@
 from functools import wraps
 from six import string_types
 
+import spack.error
 from spack.util.spack_yaml import syaml_dict
 
 
@@ -848,3 +849,11 @@ def ver(obj):
         return obj
     else:
         raise TypeError("ver() can't convert %s to version!" % type(obj))
+
+
+class VersionError(spack.error.SpackError):
+    """This is raised when something is wrong with a version."""
+
+
+class VersionChecksumError(VersionError):
+    """Raised for version checksum errors."""

var/spack/repos/builtin.mock/packages/nosource-install/package.py

@@ -0,0 +1,31 @@
+# Copyright 2013-2019 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
+from spack import *
+from llnl.util.filesystem import touch
+
+
+class NosourceInstall(BundlePackage):
+    """Simple bundle package with one dependency and metadata 'install'."""
+
+    homepage = "http://www.example.com"
+
+    version('2.0')
+    version('1.0')
+
+    depends_on('dependency-install')
+
+    # The install phase must be specified.
+    phases = ['install']
+
+    # The install method must also be present.
+    def install(self, spec, prefix):
+        touch(os.path.join(self.prefix, 'install.txt'))
+
+    @run_after('install')
+    def post_install(self):
+        touch(os.path.join(self.prefix, 'post-install.txt'))

var/spack/repos/builtin.mock/packages/nosource/package.py

@@ -0,0 +1,17 @@
+# Copyright 2013-2019 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 spack import *
+
+
+class Nosource(BundlePackage):
+    """Simple bundle package with one dependency"""
+
+    homepage = "http://www.example.com"
+
+    version('1.0')
+
+    depends_on('dependency-install')

var/spack/repos/builtin.mock/packages/noversion-bundle/package.py

@@ -0,0 +1,18 @@
+# Copyright 2013-2019 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 spack import *
+
+
+class NoversionBundle(BundlePackage):
+    """
+    Simple bundle package with no version and one dependency, which
+    should be rejected for lack of a version.
+    """
+
+    homepage = "http://www.example.com"
+
+    depends_on('dependency-install')

var/spack/repos/builtin.mock/packages/noversion/package.py

@@ -0,0 +1,19 @@
+# Copyright 2013-2019 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 spack import *
+
+
+class Noversion(Package):
+    """
+    Simple package with no version, which should be rejected since a version
+    is required.
+    """
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/a-1.0.tar.gz"
+
+    def install(self, spec, prefix):
+        touch(join_path(prefix, 'an_installation_file'))

var/spack/repos/builtin/packages/xsdk/package.py

@@ -4,25 +4,23 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 
-import os
 from spack import *
 
 
-class Xsdk(Package):
+class Xsdk(BundlePackage):
     """Xsdk is a suite of Department of Energy (DOE) packages for numerical
        simulation. This is a Spack bundle package that installs the xSDK
        packages
     """
 
     homepage = "http://xsdk.info"
-    url      = 'http://ftp.mcs.anl.gov/pub/petsc/externalpackages/xsdk.tar.gz'
 
     maintainers = ['balay', 'luszczek']
 
-    version('develop', 'a52dc710c744afa0b71429b8ec9425bc')
+    version('develop')
-    version('0.4.0', 'a52dc710c744afa0b71429b8ec9425bc')
+    version('0.4.0')
-    version('0.3.0', 'a52dc710c744afa0b71429b8ec9425bc')
+    version('0.3.0')
-    version('xsdk-0.2.0', 'a52dc710c744afa0b71429b8ec9425bc')
+    version('xsdk-0.2.0')
 
     variant('debug', default=False, description='Compile in debug mode')
     variant('cuda', default=False, description='Enable CUDA dependent packages')
@@ -123,12 +121,3 @@ class Xsdk(Package):
 
     # How do we propagate debug flag to all depends on packages ?
     # If I just do spack install xsdk+debug will that propogate it down?
-
-    # Dummy install for now,  will be removed when metapackage is available
-    def install(self, spec, prefix):
-        # Prevent the error message
-        #      ==> Error: Install failed for xsdk.  Nothing was installed!
-        #      ==> Error: Installation process had nonzero exit code : 256
-        with open(os.path.join(spec.prefix, 'bundle-package.txt'), 'w') as out:
-            out.write('This is a bundle\n')
-            out.close()