Buildcache: symlinks, externals, & install-checking (#5894)

* When creating a tar of a package for a build cache, symlinks are
  preserved (the corresponding path in the newly-created tarfile will
  be a symlink rather than a copy of the file)
* Dont add external packages to a build cache
* When installing from binary cache, don't create install prefix until
  verification is complete

4 files changed, 62 insertions, 61 deletions

diff --git a/lib/spack/docs/binary_caches.rst b/lib/spack/docs/binary_caches.rst
index 3839290c98..c7eed69a6e 100644
--- a/lib/spack/docs/binary_caches.rst
+++ b/lib/spack/docs/binary_caches.rst
@@ -26,7 +26,7 @@ Build caches are created via:
 
 .. code-block:: console
 
-   $ spack buildcache create
+   $ spack buildcache create spec
 
 
 ---------------------------------------
@@ -76,12 +76,12 @@ need to be adjusted for better re-locatability.
 Create tarball of installed Spack package and all dependencies.
 Tarballs are checksummed and signed if gpg2 is available.
 Places them in a directory ``build_cache`` that can be copied to a mirror.
-Commands like ``spack buildcache install`` will search it for pre-compiled packages.
+Commands like ``spack buildcache install`` will search Spack mirrors for build_cache to get the list of build caches.
 
 ==============  ========================================================================================================================
 Arguments       Description
 ==============  ========================================================================================================================
-``<packages>``  list of package specs or package hashes with leading ``/``
+``<specs>``     list of partial specs or hashes with a leading ``/`` to match from installed packages and used for creating build caches
 ``-d <path>``   directory in which ``build_cache`` directory is created, defaults to ``.``
 ``-f``          overwrite ``.spack`` file in ``build_cache`` directory if it exists
 ``-k <key>``    the key to sign package with. In the case where multiple keys exist, the package will be unsigned unless ``-k`` is used.
@@ -95,11 +95,11 @@ Arguments       Description
 
 Retrieves all specs for build caches available on a Spack mirror.
 
-==============  ==============================================================================
+==============  =====================================================================================
 Arguments       Description
-==============  ==============================================================================
-``<packages>``  string to be matched to matched to beginning of listed concretized short specs
-==============  ==============================================================================
+==============  =====================================================================================
+``<specs>``     list of partial package specs to be matched against specs downloaded for build caches
+==============  =====================================================================================
 
 E.g. ``spack buildcache list gcc`` with print only commands to install ``gcc`` package(s)
 
@@ -108,15 +108,15 @@ E.g. ``spack buildcache list gcc`` with print only commands to install ``gcc`` p
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Retrieves all specs for build caches available on a Spack mirror and installs build caches
-with specs matching the specs or hashes input.
+with specs matching the specs input.
 
-==============  ==============================================================
+==============  ==============================================================================================
 Arguments       Description
-==============  ==============================================================
-``<packages>``  list of package specs or package hashes with leading ``/``
+==============  ==============================================================================================
+``<specs>``     list of partial package specs or hashes with a leading ``/`` to be installed from build caches
 ``-f``          remove install directory if it exists before unpacking tarball
 ``-y``          answer yes to all to don't verify package with gpg questions
-==============  ==============================================================
+==============  ==============================================================================================
 
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 ``spack buildcache keys``
diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index a04699d1a2..a8bcaf5b1f 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -243,7 +243,7 @@ def build_tarball(spec, outdir, force=False, rel=False, yes_to_all=False,
     workdir = join_path(outdir, os.path.basename(spec.prefix))
     if os.path.exists(workdir):
         shutil.rmtree(workdir)
-    install_tree(spec.prefix, workdir)
+    install_tree(spec.prefix, workdir, symlinks=True)
 
     # create info for later relocation and create tar
     write_buildinfo_file(workdir)
@@ -370,7 +370,6 @@ def extract_tarball(spec, filename, yes_to_all=False, force=False):
             shutil.rmtree(installpath)
         else:
             raise NoOverwriteException(str(installpath))
-    mkdirp(installpath)
     stagepath = os.path.dirname(filename)
     spackfile_name = tarball_name(spec, '.spack')
     spackfile_path = os.path.join(stagepath, spackfile_name)
@@ -404,6 +403,8 @@ def extract_tarball(spec, filename, yes_to_all=False, force=False):
         if bchecksum['hash'] != checksum:
             raise NoChecksumException()
 
+    # delay creating installpath until verification is complete
+    mkdirp(installpath)
     with closing(tarfile.open(tarfile_path, 'r')) as tar:
         tar.extractall(path=join_path(installpath, '..'))
 
diff --git a/lib/spack/spack/cmd/buildcache.py b/lib/spack/spack/cmd/buildcache.py
index 12ccbd95f9..3d3396d8b7 100644
--- a/lib/spack/spack/cmd/buildcache.py
+++ b/lib/spack/spack/cmd/buildcache.py
@@ -38,18 +38,6 @@ description = "Create, download and install build cache files."
 section = "caching"
 level = "long"
 
-# Arguments for display_specs when we find ambiguity
-display_args = {
-    'long': True,
-    'show_flags': True,
-    'variants': True
-}
-
-error_message = """You can either:
-    a) use a more specific spec, or
-    b) use the package hash with a leading /
-"""
-
 
 def setup_parser(subparser):
     setup_parser.parser = subparser
@@ -106,7 +94,7 @@ def setup_parser(subparser):
     dlkeys.set_defaults(func=getkeys)
 
 
-def find_matching_specs(specs, allow_multiple_matches=False, force=False):
+def find_matching_specs(pkgs, allow_multiple_matches=False, force=False):
     """Returns a list of specs matching the not necessarily
        concretized specs given from cli
 
@@ -120,15 +108,15 @@ def find_matching_specs(specs, allow_multiple_matches=False, force=False):
     # List of specs that match expressions given via command line
     specs_from_cli = []
     has_errors = False
+    specs = spack.cmd.parse_specs(pkgs)
     for spec in specs:
         matching = spack.store.db.query(spec)
         # For each spec provided, make sure it refers to only one package.
         # Fail and ask user to be unambiguous if it doesn't
         if not allow_multiple_matches and len(matching) > 1:
-            tty.error('{0} matches multiple packages:'.format(spec))
-            print()
-            spack.cmd.display_specs(matching, **display_args)
-            print()
+            tty.error('%s matches multiple installed packages:' % spec)
+            for match in matching:
+                tty.msg('"%s"' % match.format())
             has_errors = True
 
         # No installed package matches the query
@@ -139,7 +127,7 @@ def find_matching_specs(specs, allow_multiple_matches=False, force=False):
 
         specs_from_cli.extend(matching)
     if has_errors:
-        tty.die(error_message)
+        tty.die('use one of the matching specs above')
 
     return specs_from_cli
 
@@ -163,15 +151,19 @@ def match_downloaded_specs(pkgs, allow_multiple_matches=False, force=False):
         matches = []
         tty.msg("buildcache spec(s) matching %s \n" % pkg)
         for spec in sorted(specs):
-            if spec.satisfies(pkg):
-                matches.append(spec)
+            if pkg.startswith('/'):
+                pkghash = pkg.replace('/', '')
+                if spec.dag_hash().startswith(pkghash):
+                    matches.append(spec)
+            else:
+                if spec.satisfies(pkg):
+                    matches.append(spec)
         # For each pkg provided, make sure it refers to only one package.
         # Fail and ask user to be unambiguous if it doesn't
         if not allow_multiple_matches and len(matches) > 1:
             tty.error('%s matches multiple downloaded packages:' % pkg)
-            print()
-            spack.cmd.display_specs(matches, **display_args)
-            print()
+            for match in matches:
+                tty.msg('"%s"' % match.format())
             has_errors = True
 
         # No downloaded package matches the query
@@ -181,7 +173,7 @@ def match_downloaded_specs(pkgs, allow_multiple_matches=False, force=False):
 
         specs_from_cli.extend(matches)
     if has_errors:
-        tty.die(error_message)
+        tty.die('use one of the matching specs above')
 
     return specs_from_cli
 
@@ -210,18 +202,22 @@ def createtarball(args):
 
     matches = find_matching_specs(pkgs, False, False)
     for match in matches:
-        tty.msg('adding matching spec %s' % match.format())
-        specs.add(match)
-        tty.msg('recursing dependencies')
-        for d, node in match.traverse(order='post',
-                                      depth=True,
-                                      deptype=('link', 'run')):
-            if node.external or node.virtual:
-                tty.msg('Skipping external or virtual dependency %s' %
-                        node.format())
-            else:
-                tty.msg('adding dependency %s' % node.format())
-                specs.add(node)
+        if match.external or match.virtual:
+            tty.msg('skipping external or virtual spec %s' %
+                    match.format())
+        else:
+            tty.msg('adding matching spec %s' % match.format())
+            specs.add(match)
+            tty.msg('recursing dependencies')
+            for d, node in match.traverse(order='post',
+                                          depth=True,
+                                          deptype=('link', 'run')):
+                if node.external or node.virtual:
+                    tty.msg('skipping external or virtual dependency %s' %
+                            node.format())
+                else:
+                    tty.msg('adding dependency %s' % node.format())
+                    specs.add(node)
 
     for spec in specs:
         tty.msg('creating binary cache file for package %s ' % spec.format())
@@ -307,18 +303,21 @@ def listspecs(args):
     if args.packages:
         pkgs = set(args.packages)
         for pkg in pkgs:
-            tty.msg("buildcache spec(s) matching %s \n" % pkgs)
+            tty.msg("buildcache spec(s) matching " +
+                    "%s and commands to install them" % pkgs)
             for spec in sorted(specs):
                 if spec.satisfies(pkg):
-                    tty.msg('spack buildcache install /%s\n' %
+                    tty.msg('Enter\nspack buildcache install /%s\n' %
                             spec.dag_hash(7) +
-                            '   to install %s' %
+                            ' to install "%s"' %
                             spec.format())
     else:
-        tty.msg("buildcache specs ")
+        tty.msg("buildcache specs and commands to install them")
         for spec in sorted(specs):
-            tty.msg('spack buildcache install /%s\n   to install %s' %
-                    (spec.dag_hash(7), spec.format()))
+            tty.msg('Enter\nspack buildcache install /%s\n' %
+                    spec.dag_hash(7) +
+                    ' to install "%s"' %
+                    spec.format())
 
 
 def getkeys(args):
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index 29a0f0b69a..a7bca2b875 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -94,6 +94,7 @@ echo $PATH"""
     pkg = spack.repo.get(spec)
     fake_fetchify(mock_archive.url, pkg)
     pkg.do_install()
+    pkghash = '/' + spec.dag_hash(7)
 
     # Put some non-relocatable file in there
     filename = os.path.join(spec.prefix, "dummy.txt")
@@ -133,7 +134,7 @@ echo $PATH"""
         pkg.do_uninstall(force=True)
 
         # test overwrite install
-        args = parser.parse_args(['install', '-f', str(spec)])
+        args = parser.parse_args(['install', '-f', str(pkghash)])
         buildcache.buildcache(parser, args)
 
         # create build cache with relative path and signing
@@ -149,7 +150,7 @@ echo $PATH"""
         buildcache.install_tarball(spec, args)
 
         # test overwrite install
-        args = parser.parse_args(['install', '-f', str(spec)])
+        args = parser.parse_args(['install', '-f', str(pkghash)])
         buildcache.buildcache(parser, args)
 
     else:
@@ -166,12 +167,12 @@ echo $PATH"""
         buildcache.install_tarball(spec, args)
 
         # test overwrite install without verification
-        args = parser.parse_args(['install', '-f', '-y', str(spec)])
+        args = parser.parse_args(['install', '-f', '-y', str(pkghash)])
         buildcache.buildcache(parser, args)
 
         # create build cache with relative path
         args = parser.parse_args(
-            ['create', '-d', mirror_path, '-f', '-r', '-y', str(spec)])
+            ['create', '-d', mirror_path, '-f', '-r', '-y', str(pkghash)])
         buildcache.buildcache(parser, args)
 
         # Uninstall the package
@@ -182,7 +183,7 @@ echo $PATH"""
         buildcache.install_tarball(spec, args)
 
         # test overwrite install
-        args = parser.parse_args(['install', '-f', '-y', str(spec)])
+        args = parser.parse_args(['install', '-f', '-y', str(pkghash)])
         buildcache.buildcache(parser, args)
 
     # Validate the relocation information