Only use stable versions for public mirror (#15100)

* add --skip-unstable-versions option to 'spack mirror create' which skips sources/resource for packages if their version is not stable (i.e. if they are the head of a git branch rather than a fixed commit)

* '--skip-unstable-versions' should skip all VCS sources/resources, not just those which are not cachable

11 files changed, 93 insertions, 33 deletions

diff --git a/lib/spack/spack/caches.py b/lib/spack/spack/caches.py
index 98fa8d5795..49624c06b2 100644
--- a/lib/spack/spack/caches.py
+++ b/lib/spack/spack/caches.py
@@ -50,8 +50,9 @@ def _fetch_cache():
 
 
 class MirrorCache(object):
-    def __init__(self, root):
+    def __init__(self, root, skip_unstable_versions):
         self.root = os.path.abspath(root)
+        self.skip_unstable_versions = skip_unstable_versions
 
     def store(self, fetcher, relative_dest):
         """Fetch and relocate the fetcher's target into our mirror cache."""
@@ -84,5 +85,3 @@ class MirrorCache(object):
 
 #: 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 5206927895..1473550a56 100644
--- a/lib/spack/spack/cmd/mirror.py
+++ b/lib/spack/spack/cmd/mirror.py
@@ -45,7 +45,10 @@ def setup_parser(subparser):
              " (this requires significant time and space)")
     create_parser.add_argument(
         '-f', '--file', help="file with specs of packages to put in mirror")
-
+    create_parser.add_argument(
+        '--skip-unstable-versions', action='store_true',
+        help="don't cache versions unless they identify a stable (unchanging)"
+             " source code")
     create_parser.add_argument(
         '-D', '--dependencies', action='store_true',
         help="also fetch all dependencies")
@@ -308,7 +311,8 @@ def mirror_create(args):
     existed = web_util.url_exists(directory)
 
     # Actually do the work to create the mirror
-    present, mirrored, error = spack.mirror.create(directory, mirror_specs)
+    present, mirrored, error = spack.mirror.create(
+        directory, mirror_specs, args.skip_unstable_versions)
     p, m, e = len(present), len(mirrored), len(error)
 
     verb = "updated" if existed else "created"
diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 4ca6a886d4..d923e2b58e 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -1179,6 +1179,15 @@ class S3FetchStrategy(URLFetchStrategy):
             raise FailedDownloadError(self.url)
 
 
+def stable_target(fetcher):
+    """Returns whether the fetcher target is expected to have a stable
+       checksum. This is only true if the target is a preexisting archive
+       file."""
+    if isinstance(fetcher, URLFetchStrategy) and fetcher.cachable:
+        return True
+    return False
+
+
 def from_url(url):
     """Given a URL, find an appropriate fetch strategy for it.
        Currently just gives you a URLFetchStrategy that uses curl.
diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py
index 3f6b2b6a0e..045ca5ffec 100644
--- a/lib/spack/spack/mirror.py
+++ b/lib/spack/spack/mirror.py
@@ -401,7 +401,7 @@ def get_matching_versions(specs, num_versions=1):
     return matching
 
 
-def create(path, specs):
+def create(path, specs, skip_unstable_versions=False):
     """Create a directory to be used as a spack mirror, and fill it with
     package archives.
 
@@ -409,6 +409,9 @@ def create(path, specs):
         path: Path to create a mirror directory hierarchy in.
         specs: Any package versions matching these specs will be added \
             to the mirror.
+        skip_unstable_versions: if true, this skips adding resources when
+            they do not have a stable archive checksum (as determined by
+            ``fetch_strategy.stable_target``)
 
     Return Value:
         Returns a tuple of lists: (present, mirrored, error)
@@ -440,16 +443,14 @@ def create(path, specs):
             raise MirrorError(
                 "Cannot create directory '%s':" % mirror_root, str(e))
 
-    mirror_cache = spack.caches.MirrorCache(mirror_root)
+    mirror_cache = spack.caches.MirrorCache(
+        mirror_root, skip_unstable_versions=skip_unstable_versions)
     mirror_stats = MirrorStats()
-    try:
-        spack.caches.mirror_cache = mirror_cache
-        # Iterate through packages and download all safe tarballs for each
-        for spec in specs:
-            mirror_stats.next_spec(spec)
-            add_single_spec(spec, mirror_root, mirror_stats)
-    finally:
-        spack.caches.mirror_cache = None
+
+    # Iterate through packages and download all safe tarballs for each
+    for spec in specs:
+        mirror_stats.next_spec(spec)
+        _add_single_spec(spec, mirror_cache, mirror_stats)
 
     return mirror_stats.stats()
 
@@ -495,7 +496,7 @@ class MirrorStats(object):
         self.errors.add(self.current_spec)
 
 
-def add_single_spec(spec, mirror_root, mirror_stats):
+def _add_single_spec(spec, mirror, mirror_stats):
     tty.msg("Adding package {pkg} to mirror".format(
         pkg=spec.format("{name}{@version}")
     ))
@@ -503,10 +504,10 @@ def add_single_spec(spec, mirror_root, mirror_stats):
     while num_retries > 0:
         try:
             with spec.package.stage as pkg_stage:
-                pkg_stage.cache_mirror(mirror_stats)
+                pkg_stage.cache_mirror(mirror, mirror_stats)
                 for patch in spec.package.all_patches():
-                    if patch.cache():
-                        patch.cache().cache_mirror(mirror_stats)
+                    if patch.stage:
+                        patch.stage.cache_mirror(mirror, mirror_stats)
                     patch.clean()
             exception = None
             break
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index b7efc3989b..56bdab22c1 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -1135,8 +1135,8 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
 
         for patch in self.spec.patches:
             patch.fetch()
-            if patch.cache():
-                patch.cache().cache_local()
+            if patch.stage:
+                patch.stage.cache_local()
 
     def do_stage(self, mirror_only=False):
         """Unpacks and expands the fetched tarball."""
diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py
index bcb45387a8..3a3c1507e1 100644
--- a/lib/spack/spack/patch.py
+++ b/lib/spack/spack/patch.py
@@ -85,7 +85,8 @@ class Patch(object):
 
         apply_patch(stage, self.path, self.level, self.working_dir)
 
-    def cache(self):
+    @property
+    def stage(self):
         return None
 
     def to_dict(self):
@@ -248,9 +249,6 @@ class UrlPatch(Patch):
         self._stage.create()
         return self._stage
 
-    def cache(self):
-        return self.stage
-
     def clean(self):
         self.stage.destroy()
 
diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py
index b445638228..dd05131391 100644
--- a/lib/spack/spack/stage.py
+++ b/lib/spack/spack/stage.py
@@ -493,8 +493,14 @@ class Stage(object):
         spack.caches.fetch_cache.store(
             self.fetcher, self.mirror_paths.storage_path)
 
-    def cache_mirror(self, stats):
-        """Perform a fetch if the resource is not already cached"""
+    def cache_mirror(self, mirror, stats):
+        """Perform a fetch if the resource is not already cached
+
+        Arguments:
+            mirror (MirrorCache): the mirror to cache this Stage's resource in
+            stats (MirrorStats): this is updated depending on whether the
+                caching operation succeeded or failed
+        """
         if isinstance(self.default_fetcher, fs.BundleFetchStrategy):
             # BundleFetchStrategy has no source to fetch. The associated
             # fetcher does nothing but the associated stage may still exist.
@@ -505,20 +511,23 @@ class Stage(object):
             # must examine the type of the fetcher.
             return
 
-        dst_root = spack.caches.mirror_cache.root
+        if (mirror.skip_unstable_versions and
+            not fs.stable_target(self.default_fetcher)):
+            return
+
         absolute_storage_path = os.path.join(
-            dst_root, self.mirror_paths.storage_path)
+            mirror.root, self.mirror_paths.storage_path)
 
         if os.path.exists(absolute_storage_path):
             stats.already_existed(absolute_storage_path)
         else:
             self.fetch()
             self.check()
-            spack.caches.mirror_cache.store(
+            mirror.store(
                 self.fetcher, self.mirror_paths.storage_path)
             stats.added(absolute_storage_path)
 
-        spack.caches.mirror_cache.symlink(self.mirror_paths)
+        mirror.symlink(self.mirror_paths)
 
     def expand_archive(self):
         """Changes to the stage directory and attempt to expand the downloaded
diff --git a/lib/spack/spack/test/cmd/mirror.py b/lib/spack/spack/test/cmd/mirror.py
index d62d7df432..4bb4fad224 100644
--- a/lib/spack/spack/test/cmd/mirror.py
+++ b/lib/spack/spack/test/cmd/mirror.py
@@ -66,6 +66,29 @@ def test_mirror_from_env(tmpdir, mock_packages, mock_fetch, config,
         assert mirror_res == expected
 
 
+@pytest.fixture
+def source_for_pkg_with_hash(mock_packages, tmpdir):
+    pkg = spack.repo.get('trivial-pkg-with-valid-hash')
+    local_url_basename = os.path.basename(pkg.url)
+    local_path = os.path.join(str(tmpdir), local_url_basename)
+    with open(local_path, 'w') as f:
+        f.write(pkg.hashed_content)
+    local_url = "file://" + local_path
+    pkg.versions[spack.version.Version('1.0')]['url'] = local_url
+
+
+def test_mirror_skip_unstable(tmpdir_factory, mock_packages, config,
+                              source_for_pkg_with_hash):
+    mirror_dir = str(tmpdir_factory.mktemp('mirror-dir'))
+
+    specs = [spack.spec.Spec(x).concretized() for x in
+             ['git-test', 'trivial-pkg-with-valid-hash']]
+    spack.mirror.create(mirror_dir, specs, skip_unstable_versions=True)
+
+    assert (set(os.listdir(mirror_dir)) - set(['_source-cache']) ==
+            set(['trivial-pkg-with-valid-hash']))
+
+
 def test_mirror_crud(tmp_scope, capsys):
     with capsys.disabled():
         mirror('add', '--scope', tmp_scope, 'mirror', 'http://spack.io')
diff --git a/lib/spack/spack/test/mirror.py b/lib/spack/spack/test/mirror.py
index 08b32f74f1..570c329b71 100644
--- a/lib/spack/spack/test/mirror.py
+++ b/lib/spack/spack/test/mirror.py
@@ -213,7 +213,7 @@ def test_mirror_cache_symlinks(tmpdir):
     """
     cosmetic_path = 'zlib/zlib-1.2.11.tar.gz'
     global_path = '_source-cache/archive/c3/c3e5.tar.gz'
-    cache = spack.caches.MirrorCache(str(tmpdir))
+    cache = spack.caches.MirrorCache(str(tmpdir), False)
     reference = spack.mirror.MirrorReference(cosmetic_path, global_path)
 
     cache.store(MockFetcher(), reference.storage_path)
diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash
index 869d63be31..38788438f7 100755
--- a/share/spack/spack-completion.bash
+++ b/share/spack/spack-completion.bash
@@ -1025,7 +1025,7 @@ _spack_mirror() {
 _spack_mirror_create() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help -d --directory -a --all -f --file -D --dependencies -n --versions-per-spec"
+        SPACK_COMPREPLY="-h --help -d --directory -a --all -f --file --skip-unstable-versions -D --dependencies -n --versions-per-spec"
     else
         _all_packages
     fi
diff --git a/var/spack/repos/builtin.mock/packages/trivial-pkg-with-valid-hash/package.py b/var/spack/repos/builtin.mock/packages/trivial-pkg-with-valid-hash/package.py
new file mode 100644
index 0000000000..52fd3d99a6
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/trivial-pkg-with-valid-hash/package.py
@@ -0,0 +1,17 @@
+# Copyright 2013-2020 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 TrivialPkgWithValidHash(Package):
+    url = "http://www.unit-test-should-replace-this-url/trivial_install-1.0"
+
+    version('1.0', '6ae8a75555209fd6c44157c0aed8016e763ff435a19cf186f76863140143ff72', expand=False)
+
+    hashed_content = "test content"
+
+    def install(self, spec, prefix):
+        pass