mirrors: patches are now properly added to mirrors (#8993)

* This fixes a number of bugs:

  * Patches were not properly downloaded and added to mirrors.

  * Mirror create didn't respect `list_url` in packages

  * Update the `spack mirror` command to add all packages in the
    concretized DAG (where originally it only added the package specified
    by the user). This is required in order to collect patches that are specified
    by dependents. Example:
      * if X->Y and X requires a patch on Y called Pxy, then Pxy will only
        be discovered if you create a mirror with X.

  * replace confusing --one-version-per-spec option for `spack mirror create`
    with --versions-per-spec; support retrieving multiple versions for 
    concrete specs

* Implementation details:

  * `spack mirror create` now uses regular staging logic to download files 
    into a mirror, instead of reimplementing it in `add_single_spec`.

  * use a separate resource caching object to keep track of new
    resources and already-existing resources; also accepts storing
    resources retrieved from a cache (unlike the local cache)

  * mirror cache object now stores resources that are considered
    non-cachable, like (e.g. the tip of a branch);

  * the 'create' function of the mirror module no longer traverses
    dependencies since this was already handled by the 'mirror' command; 

  * Change handling of `--no-checksum`:

    * now that 'mirror create' uses stages, the mirror tests disable
      checksums when creating the mirror

    * remove `no_checksum` argument from library functions - this is now
      handled at the Spack-command-level (like for 'spack install')

6 files changed, 107 insertions, 59 deletions

diff --git a/lib/spack/spack/caches.py b/lib/spack/spack/caches.py
index 1d74e9dc3b..6afc8b4ffe 100644
--- a/lib/spack/spack/caches.py
+++ b/lib/spack/spack/caches.py
@@ -7,6 +7,7 @@
 import os
 
 import llnl.util.lang
+from llnl.util.filesystem import mkdirp
 
 import spack.paths
 import spack.config
@@ -47,5 +48,26 @@ def _fetch_cache():
     return spack.fetch_strategy.FsCache(path)
 
 
+class MirrorCache(object):
+    def __init__(self, root):
+        self.root = os.path.abspath(root)
+        self.new_resources = set()
+        self.existing_resources = set()
+
+    def store(self, fetcher, relative_dest):
+        # Note this will archive package sources even if they would not
+        # normally be cached (e.g. the current tip of an hg/git branch)
+
+        dst = os.path.join(self.root, relative_dest)
+        if os.path.exists(dst):
+            self.existing_resources.add(relative_dest)
+        else:
+            self.new_resources.add(relative_dest)
+            mkdirp(os.path.dirname(dst))
+            fetcher.archive(dst)
+
+
 #: Spack's local cache for downloaded source archives
 fetch_cache = llnl.util.lang.Singleton(_fetch_cache)
+
+mirror_cache = None
diff --git a/lib/spack/spack/cmd/mirror.py b/lib/spack/spack/cmd/mirror.py
index 4d70df431b..36d7f0cbd7 100644
--- a/lib/spack/spack/cmd/mirror.py
+++ b/lib/spack/spack/cmd/mirror.py
@@ -44,9 +44,9 @@ def setup_parser(subparser):
         '-D', '--dependencies', action='store_true',
         help="also fetch all dependencies")
     create_parser.add_argument(
-        '-o', '--one-version-per-spec', action='store_const',
-        const=1, default=0,
-        help="only fetch one 'preferred' version per spec, not all known")
+        '-n', '--versions-per-spec', type=int,
+        default=1,
+        help="the number of versions to fetch for each spec")
 
     # used to construct scope arguments below
     scopes = spack.config.scopes()
@@ -192,7 +192,7 @@ def mirror_create(args):
 
         # Actually do the work to create the mirror
         present, mirrored, error = spack.mirror.create(
-            directory, specs, num_versions=args.one_version_per_spec)
+            directory, specs, num_versions=args.versions_per_spec)
         p, m, e = len(present), len(mirrored), len(error)
 
         verb = "updated" if existed else "created"
@@ -214,4 +214,7 @@ def mirror(parser, args):
               'rm': mirror_remove,
               'list': mirror_list}
 
+    if args.no_checksum:
+        spack.config.set('config:checksum', False, scope='command_line')
+
     action[args.mirror_command](args)
diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py
index 3310bc53fb..712a62117a 100644
--- a/lib/spack/spack/mirror.py
+++ b/lib/spack/spack/mirror.py
@@ -83,7 +83,11 @@ def mirror_archive_path(spec, fetcher, resource_id=None):
 
 
 def get_matching_versions(specs, **kwargs):
-    """Get a spec for EACH known version matching any spec in the list."""
+    """Get a spec for EACH known version matching any spec in the list.
+    For concrete specs, this retrieves the concrete version and, if more
+    than one version per spec is requested, retrieves the latest versions
+    of the package.
+    """
     matching = []
     for spec in specs:
         pkg = spec.package
@@ -93,15 +97,22 @@ def get_matching_versions(specs, **kwargs):
             tty.msg("No safe (checksummed) versions for package %s" % pkg.name)
             continue
 
-        num_versions = kwargs.get('num_versions', 0)
+        pkg_versions = kwargs.get('num_versions', 1)
+
+        version_order = list(reversed(sorted(pkg.versions)))
         matching_spec = []
-        for i, v in enumerate(reversed(sorted(pkg.versions))):
+        if spec.concrete:
+            matching_spec.append(spec)
+            pkg_versions -= 1
+            version_order.remove(spec.version)
+
+        for v in version_order:
             # Generate no more than num_versions versions for each spec.
-            if num_versions and i >= num_versions:
+            if pkg_versions < 1:
                 break
 
             # Generate only versions that satisfy the spec.
-            if v.satisfies(spec.versions):
+            if spec.concrete or v.satisfies(spec.versions):
                 s = Spec(pkg.name)
                 s.versions = VersionList([v])
                 s.variants = spec.variants.copy()
@@ -109,6 +120,7 @@ def get_matching_versions(specs, **kwargs):
                 # concretization phase
                 s.variants.spec = s
                 matching_spec.append(s)
+                pkg_versions -= 1
 
         if not matching_spec:
             tty.warn("No known version matches spec: %s" % spec)
@@ -139,9 +151,8 @@ def create(path, specs, **kwargs):
             to the mirror.
 
     Keyword args:
-        no_checksum: If True, do not checkpoint when fetching (default False)
         num_versions: Max number of versions to fetch per spec, \
-            if spec is ambiguous (default is 0 for all of them)
+            (default is 1 each spec)
 
     Return Value:
         Returns a tuple of lists: (present, mirrored, error)
@@ -163,7 +174,7 @@ def create(path, specs, **kwargs):
 
     # Get concrete specs for each matching version of these specs.
     version_specs = get_matching_versions(
-        specs, num_versions=kwargs.get('num_versions', 0))
+        specs, num_versions=kwargs.get('num_versions', 1))
     for s in version_specs:
         s.concretize()
 
@@ -183,57 +194,26 @@ def create(path, specs, **kwargs):
         'error': []
     }
 
-    # Iterate through packages and download all safe tarballs for each
-    for spec in version_specs:
-        add_single_spec(spec, mirror_root, categories, **kwargs)
+    mirror_cache = spack.caches.MirrorCache(mirror_root)
+    try:
+        spack.caches.mirror_cache = mirror_cache
+        # Iterate through packages and download all safe tarballs for each
+        for spec in version_specs:
+            add_single_spec(spec, mirror_root, categories, **kwargs)
+    finally:
+        spack.caches.mirror_cache = None
+
+    categories['mirrored'] = list(mirror_cache.new_resources)
+    categories['present'] = list(mirror_cache.existing_resources)
 
     return categories['present'], categories['mirrored'], categories['error']
 
 
 def add_single_spec(spec, mirror_root, categories, **kwargs):
     tty.msg("Adding package {pkg} to mirror".format(pkg=spec.format("$_$@")))
-    spec_exists_in_mirror = True
     try:
-        with spec.package.stage:
-            # fetcher = stage.fetcher
-            # fetcher.fetch()
-            # ...
-            # fetcher.archive(archive_path)
-            for ii, stage in enumerate(spec.package.stage):
-                fetcher = stage.fetcher
-                if ii == 0:
-                    # create a subdirectory for the current package@version
-                    archive_path = os.path.abspath(os.path.join(
-                        mirror_root, mirror_archive_path(spec, fetcher)))
-                    name = spec.cformat("$_$@")
-                else:
-                    resource = stage.resource
-                    archive_path = os.path.abspath(os.path.join(
-                        mirror_root,
-                        mirror_archive_path(spec, fetcher, resource.name)))
-                    name = "{resource} ({pkg}).".format(
-                        resource=resource.name, pkg=spec.cformat("$_$@"))
-                subdir = os.path.dirname(archive_path)
-                mkdirp(subdir)
-
-                if os.path.exists(archive_path):
-                    tty.msg("{name} : already added".format(name=name))
-                else:
-                    spec_exists_in_mirror = False
-                    fetcher.fetch()
-                    if not kwargs.get('no_checksum', False):
-                        fetcher.check()
-                        tty.msg("{name} : checksum passed".format(name=name))
-
-                    # Fetchers have to know how to archive their files.  Use
-                    # that to move/copy/create an archive in the mirror.
-                    fetcher.archive(archive_path)
-                    tty.msg("{name} : added".format(name=name))
-
-        if spec_exists_in_mirror:
-            categories['present'].append(spec)
-        else:
-            categories['mirrored'].append(spec)
+        spec.package.do_patch()
+        spec.package.do_clean()
 
     except Exception as e:
         if spack.config.get('config:debug'):
diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py
index a865d429a4..cbab403f20 100644
--- a/lib/spack/spack/patch.py
+++ b/lib/spack/spack/patch.py
@@ -166,7 +166,7 @@ class UrlPatch(Patch):
 
             # for a compressed archive, Need to check the patch sha256 again
             # and the patch is in a directory, not in the same place
-            if self.archive_sha256:
+            if self.archive_sha256 and spack.config.get('config:checksum'):
                 checker = Checker(self.sha256)
                 if not checker.check(self.path):
                     raise fs.ChecksumError(
diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py
index 32cdaa0090..e97a19ddba 100644
--- a/lib/spack/spack/stage.py
+++ b/lib/spack/spack/stage.py
@@ -429,12 +429,15 @@ class Stage(object):
                      "mirror.  This means we cannot know a checksum for the "
                      "tarball in advance. Be sure that your connection to "
                      "this mirror is secure!")
-        else:
+        elif spack.config.get('config:checksum'):
             self.fetcher.check()
 
     def cache_local(self):
         spack.caches.fetch_cache.store(self.fetcher, self.mirror_path)
 
+        if spack.caches.mirror_cache:
+            spack.caches.mirror_cache.store(self.fetcher, self.mirror_path)
+
     def expand_archive(self):
         """Changes to the stage directory and attempt to expand the downloaded
         archive.  Fail if the stage is not set up or if the archive is not yet
diff --git a/lib/spack/spack/test/mirror.py b/lib/spack/spack/test/mirror.py
index d59f958481..e2ddb85c97 100644
--- a/lib/spack/spack/test/mirror.py
+++ b/lib/spack/spack/test/mirror.py
@@ -51,7 +51,8 @@ def check_mirror():
         # register mirror with spack config
         mirrors = {'spack-mirror-test': 'file://' + mirror_root}
         spack.config.set('mirrors', mirrors)
-        spack.mirror.create(mirror_root, repos, no_checksum=True)
+        with spack.config.override('config:checksum', False):
+            spack.mirror.create(mirror_root, repos)
 
         # Stage directory exists
         assert os.path.isdir(mirror_root)
@@ -138,3 +139,42 @@ def test_all_mirror(
     set_up_package('trivial-install-test-package', mock_archive, 'url')
     check_mirror()
     repos.clear()
+
+
+def test_mirror_with_url_patches(mock_packages, config, monkeypatch):
+    spec = Spec('patch-several-dependencies')
+    spec.concretize()
+
+    files_cached_in_mirror = set()
+
+    def record_store(_class, fetcher, relative_dst):
+        files_cached_in_mirror.add(os.path.basename(relative_dst))
+
+    def successful_fetch(_class):
+        with open(_class.stage.save_filename, 'w'):
+            pass
+
+    def successful_expand(_class):
+        expanded_path = os.path.join(_class.stage.path, 'expanded-dir')
+        os.mkdir(expanded_path)
+        with open(os.path.join(expanded_path, 'test.patch'), 'w'):
+            pass
+
+    def successful_apply(_class, stage):
+        pass
+
+    with Stage('spack-mirror-test') as stage:
+        mirror_root = os.path.join(stage.path, 'test-mirror')
+
+        monkeypatch.setattr(spack.fetch_strategy.URLFetchStrategy, 'fetch',
+                            successful_fetch)
+        monkeypatch.setattr(spack.fetch_strategy.URLFetchStrategy,
+                            'expand', successful_expand)
+        monkeypatch.setattr(spack.patch.Patch, 'apply', successful_apply)
+        monkeypatch.setattr(spack.caches.MirrorCache, 'store', record_store)
+
+        with spack.config.override('config:checksum', False):
+            spack.mirror.create(mirror_root, list(spec.traverse()))
+
+        assert not (set(['urlpatch.patch', 'urlpatch2.patch.gz']) -
+                    files_cached_in_mirror)