Fix performance regression with `spack mirror create --all` (#32005)

This PR fixes the performance regression reported in #31985 and a few
other issues found while refactoring the spack mirror create command.

Modifications:

* (Primary) Do not require concretization for
  `spack mirror create --all`
* Forbid using --versions-per-spec together with --all
* Fixed a few issues when reading specs from input file (specs were
  not concretized, command would fail when trying to mirror
  dependencies)
* Fix issue with default directory for spack mirror create not being
  canonicalized
* Add more unit tests to poke spack mirror create
* Skip externals also when mirroring environments
* Changed slightly the wording for reporting (it was mentioning
  "Successfully created" even in presence of errors)
* Fix issue with colify (was not called properly during error
  reporting)

14 files changed, 416 insertions, 184 deletions

diff --git a/lib/spack/llnl/util/lang.py b/lib/spack/llnl/util/lang.py
index 5b56990a6f..76b161cbbe 100644
--- a/lib/spack/llnl/util/lang.py
+++ b/lib/spack/llnl/util/lang.py
@@ -13,7 +13,7 @@ import re
 import sys
 import traceback
 from datetime import datetime, timedelta
-from typing import List, Tuple
+from typing import Any, Callable, Iterable, List, Tuple
 
 import six
 from six import string_types
@@ -977,6 +977,31 @@ def enum(**kwargs):
     return type("Enum", (object,), kwargs)
 
 
+def stable_partition(
+    input_iterable,  # type: Iterable
+    predicate_fn,  # type: Callable[[Any], bool]
+):
+    # type: (...) -> Tuple[List[Any], List[Any]]
+    """Partition the input iterable according to a custom predicate.
+
+    Args:
+        input_iterable: input iterable to be partitioned.
+        predicate_fn: predicate function accepting an iterable item
+            as argument.
+
+    Return:
+        Tuple of the list of elements evaluating to True, and
+        list of elements evaluating to False.
+    """
+    true_items, false_items = [], []
+    for item in input_iterable:
+        if predicate_fn(item):
+            true_items.append(item)
+            continue
+        false_items.append(item)
+    return true_items, false_items
+
+
 class TypedMutableSequence(MutableSequence):
     """Base class that behaves like a list, just with a different type.
 
diff --git a/lib/spack/spack/build_systems/racket.py b/lib/spack/spack/build_systems/racket.py
index c6984f1d9a..7b37d85cf2 100644
--- a/lib/spack/spack/build_systems/racket.py
+++ b/lib/spack/spack/build_systems/racket.py
@@ -40,13 +40,13 @@ class RacketPackage(PackageBase):
 
     pkgs = False
     subdirectory = None  # type: Optional[str]
-    name = None  # type: Optional[str]
+    racket_name = None  # type: Optional[str]
     parallel = True
 
     @lang.classproperty
     def homepage(cls):
         if cls.pkgs:
-            return "https://pkgs.racket-lang.org/package/{0}".format(cls.name)
+            return "https://pkgs.racket-lang.org/package/{0}".format(cls.racket_name)
 
     @property
     def build_directory(self):
@@ -66,7 +66,7 @@ class RacketPackage(PackageBase):
                 "-t",
                 "dir",
                 "-n",
-                self.name,
+                self.racket_name,
                 "--deps",
                 "fail",
                 "--ignore-implies",
@@ -86,5 +86,5 @@ class RacketPackage(PackageBase):
                     (
                         "Racket package {0} was already installed, uninstalling via "
                         "Spack may make someone unhappy!"
-                    ).format(self.name)
+                    ).format(self.racket_name)
                 )
diff --git a/lib/spack/spack/cmd/mirror.py b/lib/spack/spack/cmd/mirror.py
index 63ec1a6bdd..ca960bf6e6 100644
--- a/lib/spack/spack/cmd/mirror.py
+++ b/lib/spack/spack/cmd/mirror.py
@@ -5,7 +5,9 @@
 
 import sys
 
+import llnl.util.lang as lang
 import llnl.util.tty as tty
+import llnl.util.tty.colify as colify
 
 import spack.cmd
 import spack.cmd.common.arguments as arguments
@@ -14,10 +16,11 @@ import spack.config
 import spack.environment as ev
 import spack.mirror
 import spack.repo
+import spack.spec
+import spack.util.path
 import spack.util.url as url_util
 import spack.util.web as web_util
 from spack.error import SpackError
-from spack.spec import Spec
 from spack.util.spack_yaml import syaml_dict
 
 description = "manage mirrors (source and binary)"
@@ -231,131 +234,140 @@ def mirror_list(args):
     mirrors.display()
 
 
-def _read_specs_from_file(filename):
-    specs = []
-    with open(filename, "r") as stream:
-        for i, string in enumerate(stream):
-            try:
-                s = Spec(string)
-                spack.repo.path.get_pkg_class(s.name)
-                specs.append(s)
-            except SpackError as e:
-                tty.debug(e)
-                tty.die("Parse error in %s, line %d:" % (filename, i + 1), ">>> " + string, str(e))
+def specs_from_text_file(filename, concretize=False):
+    """Return a list of specs read from a text file.
+
+    The file should contain one spec per line.
+
+    Args:
+        filename (str): name of the file containing the abstract specs.
+        concretize (bool): if True concretize the specs before returning
+            the list.
+    """
+    with open(filename, "r") as f:
+        specs_in_file = f.readlines()
+        specs_in_file = [s.strip() for s in specs_in_file]
+    return spack.cmd.parse_specs(" ".join(specs_in_file), concretize=concretize)
+
+
+def concrete_specs_from_user(args):
+    """Return the list of concrete specs that the user wants to mirror. The list
+    is passed either from command line or from a text file.
+    """
+    specs = concrete_specs_from_cli_or_file(args)
+    specs = extend_with_additional_versions(specs, num_versions=versions_per_spec(args))
+    if args.dependencies:
+        specs = extend_with_dependencies(specs)
+    specs = filter_externals(specs)
+    specs = list(set(specs))
+    specs.sort(key=lambda s: (s.name, s.version))
+    specs, _ = lang.stable_partition(specs, predicate_fn=not_excluded_fn(args))
     return specs
 
 
-def _determine_specs_to_mirror(args):
-    if args.specs and args.all:
-        raise SpackError(
-            "Cannot specify specs on command line if you" " chose to mirror all specs with '--all'"
-        )
-    elif args.file and args.all:
-        raise SpackError(
-            "Cannot specify specs with a file ('-f') if you"
-            " chose to mirror all specs with '--all'"
-        )
-
-    if not args.versions_per_spec:
-        num_versions = 1
-    elif args.versions_per_spec == "all":
-        num_versions = "all"
+def extend_with_additional_versions(specs, num_versions):
+    if num_versions == "all":
+        mirror_specs = spack.mirror.get_all_versions(specs)
     else:
-        try:
-            num_versions = int(args.versions_per_spec)
-        except ValueError:
-            raise SpackError(
-                "'--versions-per-spec' must be a number or 'all',"
-                " got '{0}'".format(args.versions_per_spec)
-            )
+        mirror_specs = spack.mirror.get_matching_versions(specs, num_versions=num_versions)
+    mirror_specs = [x.concretized() for x in mirror_specs]
+    return mirror_specs
+
+
+def filter_externals(specs):
+    specs, external_specs = lang.stable_partition(specs, predicate_fn=lambda x: not x.external)
+    for spec in external_specs:
+        msg = "Skipping {0} as it is an external spec."
+        tty.msg(msg.format(spec.cshort_spec))
+    return specs
 
-    # try to parse specs from the command line first.
+
+def extend_with_dependencies(specs):
+    """Extend the input list by adding all the dependencies explicitly."""
+    result = set()
+    for spec in specs:
+        for s in spec.traverse():
+            result.add(s)
+    return list(result)
+
+
+def concrete_specs_from_cli_or_file(args):
+    tty.msg("Concretizing input specs")
     with spack.concretize.disable_compiler_existence_check():
-        specs = spack.cmd.parse_specs(args.specs, concretize=True)
+        if args.specs:
+            specs = spack.cmd.parse_specs(args.specs, concretize=True)
+            if not specs:
+                raise SpackError("unable to parse specs from command line")
 
-        # If there is a file, parse each line as a spec and add it to the list.
         if args.file:
-            if specs:
-                tty.die("Cannot pass specs on the command line with --file.")
-            specs = _read_specs_from_file(args.file)
-
-        env_specs = None
-        if not specs:
-            # If nothing is passed, use environment or all if no active env
-            if not args.all:
-                tty.die(
-                    "No packages were specified.",
-                    "To mirror all packages, use the '--all' option"
-                    " (this will require significant time and space).",
-                )
-
-            env = ev.active_environment()
-            if env:
-                env_specs = env.all_specs()
-            else:
-                specs = [Spec(n) for n in spack.repo.all_package_names()]
-        else:
-            # If the user asked for dependencies, traverse spec DAG get them.
-            if args.dependencies:
-                new_specs = set()
-                for spec in specs:
-                    spec.concretize()
-                    for s in spec.traverse():
-                        new_specs.add(s)
-                specs = list(new_specs)
-
-            # Skip external specs, as they are already installed
-            external_specs = [s for s in specs if s.external]
-            specs = [s for s in specs if not s.external]
-
-            for spec in external_specs:
-                msg = "Skipping {0} as it is an external spec."
-                tty.msg(msg.format(spec.cshort_spec))
-
-        if env_specs:
-            if args.versions_per_spec:
-                tty.warn("Ignoring '--versions-per-spec' for mirroring specs" " in environment.")
-            mirror_specs = env_specs
-        else:
-            if num_versions == "all":
-                mirror_specs = spack.mirror.get_all_versions(specs)
-            else:
-                mirror_specs = spack.mirror.get_matching_versions(specs, num_versions=num_versions)
-            mirror_specs.sort(key=lambda s: (s.name, s.version))
+            specs = specs_from_text_file(args.file, concretize=True)
+            if not specs:
+                raise SpackError("unable to parse specs from file '{}'".format(args.file))
+    return specs
 
+
+def not_excluded_fn(args):
+    """Return a predicate that evaluate to True if a spec was not explicitly
+    excluded by the user.
+    """
     exclude_specs = []
     if args.exclude_file:
-        exclude_specs.extend(_read_specs_from_file(args.exclude_file))
+        exclude_specs.extend(specs_from_text_file(args.exclude_file, concretize=False))
     if args.exclude_specs:
         exclude_specs.extend(spack.cmd.parse_specs(str(args.exclude_specs).split()))
-    if exclude_specs:
-        mirror_specs = list(
-            x for x in mirror_specs if not any(x.satisfies(y, strict=True) for y in exclude_specs)
-        )
 
+    def not_excluded(x):
+        return not any(x.satisfies(y, strict=True) for y in exclude_specs)
+
+    return not_excluded
+
+
+def concrete_specs_from_environment(selection_fn):
+    env = ev.active_environment()
+    assert env, "an active environment is required"
+    mirror_specs = env.all_specs()
+    mirror_specs = filter_externals(mirror_specs)
+    mirror_specs, _ = lang.stable_partition(mirror_specs, predicate_fn=selection_fn)
     return mirror_specs
 
 
-def mirror_create(args):
-    """Create a directory to be used as a spack mirror, and fill it with
-    package archives."""
-    mirror_specs = _determine_specs_to_mirror(args)
+def all_specs_with_all_versions(selection_fn):
+    specs = [spack.spec.Spec(n) for n in spack.repo.all_package_names()]
+    mirror_specs = spack.mirror.get_all_versions(specs)
+    mirror_specs.sort(key=lambda s: (s.name, s.version))
+    mirror_specs, _ = lang.stable_partition(mirror_specs, predicate_fn=selection_fn)
+    return mirror_specs
 
-    mirror = spack.mirror.Mirror(args.directory or spack.config.get("config:source_cache"))
 
-    directory = url_util.format(mirror.push_url)
+def versions_per_spec(args):
+    """Return how many versions should be mirrored per spec."""
+    if not args.versions_per_spec:
+        num_versions = 1
+    elif args.versions_per_spec == "all":
+        num_versions = "all"
+    else:
+        try:
+            num_versions = int(args.versions_per_spec)
+        except ValueError:
+            raise SpackError(
+                "'--versions-per-spec' must be a number or 'all',"
+                " got '{0}'".format(args.versions_per_spec)
+            )
+    return num_versions
 
-    existed = web_util.url_exists(directory)
 
-    # Actually do the work to create the mirror
+def create_mirror_for_individual_specs(mirror_specs, directory_hint, skip_unstable_versions):
+    local_push_url = local_mirror_url_from_user(directory_hint)
     present, mirrored, error = spack.mirror.create(
-        directory, mirror_specs, args.skip_unstable_versions
+        local_push_url, mirror_specs, skip_unstable_versions
     )
-    p, m, e = len(present), len(mirrored), len(error)
+    tty.msg("Summary for mirror in {}".format(local_push_url))
+    process_mirror_stats(present, mirrored, error)
 
-    verb = "updated" if existed else "created"
+
+def process_mirror_stats(present, mirrored, error):
+    p, m, e = len(present), len(mirrored), len(error)
     tty.msg(
-        "Successfully %s mirror in %s" % (verb, directory),
         "Archive stats:",
         "  %-4d already present" % p,
         "  %-4d added" % m,
@@ -363,10 +375,104 @@ def mirror_create(args):
     )
     if error:
         tty.error("Failed downloads:")
-        tty.colify(s.cformat("{name}{@version}") for s in error)
+        colify.colify(s.cformat("{name}{@version}") for s in error)
         sys.exit(1)
 
 
+def local_mirror_url_from_user(directory_hint):
+    """Return a file:// url pointing to the local mirror to be used.
+
+    Args:
+        directory_hint (str or None): directory where to create the mirror. If None,
+            defaults to "config:source_cache".
+    """
+    mirror_directory = spack.util.path.canonicalize_path(
+        directory_hint or spack.config.get("config:source_cache")
+    )
+    tmp_mirror = spack.mirror.Mirror(mirror_directory)
+    local_url = url_util.format(tmp_mirror.push_url)
+    return local_url
+
+
+def mirror_create(args):
+    """Create a directory to be used as a spack mirror, and fill it with
+    package archives.
+    """
+    if args.specs and args.all:
+        raise SpackError(
+            "cannot specify specs on command line if you chose to mirror all specs with '--all'"
+        )
+
+    if args.file and args.all:
+        raise SpackError(
+            "cannot specify specs with a file if you chose to mirror all specs with '--all'"
+        )
+
+    if args.file and args.specs:
+        raise SpackError("cannot specify specs with a file AND on command line")
+
+    if not args.specs and not args.file and not args.all:
+        raise SpackError(
+            "no packages were specified.",
+            "To mirror all packages, use the '--all' option "
+            "(this will require significant time and space).",
+        )
+
+    if args.versions_per_spec and args.all:
+        raise SpackError(
+            "cannot specify '--versions_per-spec' and '--all' together",
+            "The option '--all' already implies mirroring all versions for each package.",
+        )
+
+    if args.all and not ev.active_environment():
+        create_mirror_for_all_specs(
+            directory_hint=args.directory,
+            skip_unstable_versions=args.skip_unstable_versions,
+            selection_fn=not_excluded_fn(args),
+        )
+        return
+
+    if args.all and ev.active_environment():
+        create_mirror_for_all_specs_inside_environment(
+            directory_hint=args.directory,
+            skip_unstable_versions=args.skip_unstable_versions,
+            selection_fn=not_excluded_fn(args),
+        )
+        return
+
+    mirror_specs = concrete_specs_from_user(args)
+    create_mirror_for_individual_specs(
+        mirror_specs,
+        directory_hint=args.directory,
+        skip_unstable_versions=args.skip_unstable_versions,
+    )
+
+
+def create_mirror_for_all_specs(directory_hint, skip_unstable_versions, selection_fn):
+    mirror_specs = all_specs_with_all_versions(selection_fn=selection_fn)
+    local_push_url = local_mirror_url_from_user(directory_hint=directory_hint)
+    mirror_cache, mirror_stats = spack.mirror.mirror_cache_and_stats(
+        local_push_url, skip_unstable_versions=skip_unstable_versions
+    )
+    for candidate in mirror_specs:
+        pkg_cls = spack.repo.path.get_pkg_class(candidate.name)
+        pkg_obj = pkg_cls(spack.spec.Spec(candidate))
+        mirror_stats.next_spec(pkg_obj.spec)
+        spack.mirror.create_mirror_from_package_object(pkg_obj, mirror_cache, mirror_stats)
+    process_mirror_stats(*mirror_stats.stats())
+
+
+def create_mirror_for_all_specs_inside_environment(
+    directory_hint, skip_unstable_versions, selection_fn
+):
+    mirror_specs = concrete_specs_from_environment(selection_fn=selection_fn)
+    create_mirror_for_individual_specs(
+        mirror_specs,
+        directory_hint=directory_hint,
+        skip_unstable_versions=skip_unstable_versions,
+    )
+
+
 def mirror_destroy(args):
     """Given a url, recursively delete everything under it."""
     mirror_url = None
diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py
index 7d352fe5fe..1bd9c6cd28 100644
--- a/lib/spack/spack/mirror.py
+++ b/lib/spack/spack/mirror.py
@@ -2,7 +2,6 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
 """
 This file contains code for creating spack mirror directories.  A
 mirror is an organized hierarchy containing specially named archive
@@ -56,7 +55,7 @@ class Mirror(object):
 
     Mirrors have a fetch_url that indicate where and how artifacts are fetched
     from them, and a push_url that indicate where and how artifacts are pushed
-    to them.  These two URLs are usually the same.
+    to them. These two URLs are usually the same.
     """
 
     def __init__(self, fetch_url, push_url=None, name=None):
@@ -499,37 +498,43 @@ def create(path, specs, skip_unstable_versions=False):
         * present:  Package specs that were already present.
         * mirrored: Package specs that were successfully mirrored.
         * error:    Package specs that failed to mirror due to some error.
+    """
+    # automatically spec-ify anything in the specs array.
+    specs = [s if isinstance(s, spack.spec.Spec) else spack.spec.Spec(s) for s in specs]
+
+    mirror_cache, mirror_stats = mirror_cache_and_stats(path, skip_unstable_versions)
+    for spec in specs:
+        mirror_stats.next_spec(spec)
+        create_mirror_from_package_object(spec.package, mirror_cache, mirror_stats)
+
+    return mirror_stats.stats()
+
 
-    This routine iterates through all known package versions, and
-    it creates specs for those versions.  If the version satisfies any spec
-    in the specs list, it is downloaded and added to the mirror.
+def mirror_cache_and_stats(path, skip_unstable_versions=False):
+    """Return both a mirror cache and a mirror stats, starting from the path
+    where a mirror ought to be created.
+
+    Args:
+        path (str): path to create a mirror directory hierarchy in.
+        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``)
     """
     parsed = url_util.parse(path)
     mirror_root = url_util.local_file_path(parsed)
     if not mirror_root:
         raise spack.error.SpackError("MirrorCaches only work with file:// URLs")
-
-    # automatically spec-ify anything in the specs array.
-    specs = [s if isinstance(s, spack.spec.Spec) else spack.spec.Spec(s) for s in specs]
-
     # Get the absolute path of the root before we start jumping around.
     if not os.path.isdir(mirror_root):
         try:
             mkdirp(mirror_root)
         except OSError as e:
             raise MirrorError("Cannot create directory '%s':" % mirror_root, str(e))
-
     mirror_cache = spack.caches.MirrorCache(
         mirror_root, skip_unstable_versions=skip_unstable_versions
     )
     mirror_stats = MirrorStats()
-
-    # 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()
+    return mirror_cache, mirror_stats
 
 
 def add(name, url, scope, args={}):
@@ -631,37 +636,29 @@ class MirrorStats(object):
         self.errors.add(self.current_spec)
 
 
-def _add_single_spec(spec, mirror, mirror_stats):
-    """Add a single spec to a mirror.
+def create_mirror_from_package_object(pkg_obj, mirror_cache, mirror_stats):
+    """Add a single package object to a mirror.
+
+    The package object is only required to have an associated spec
+    with a concrete version.
 
     Args:
-        spec (spack.spec.Spec): spec to be added. If not concrete it will
-            be concretized.
-        mirror (spack.mirror.Mirror): mirror where to add the spec.
+        pkg_obj (spack.package_base.PackageBase): package object with to be added.
+        mirror_cache (spack.caches.MirrorCache): mirror where to add the spec.
         mirror_stats (spack.mirror.MirrorStats): statistics on the current mirror
 
     Return:
         True if the spec was added successfully, False otherwise
     """
-    # Ensure that the spec is concrete, since we'll stage it later
-    try:
-        if not spec.concrete:
-            spec = spec.concretized()
-    except Exception as e:
-        msg = "Skipping '{0}', as it fails to concretize [{1}]".format(spec, str(e))
-        tty.debug(msg)
-        mirror_stats.error()
-        return False
-
-    tty.msg("Adding package {pkg} to mirror".format(pkg=spec.format("{name}{@version}")))
+    tty.msg("Adding package {} to mirror".format(pkg_obj.spec.format("{name}{@version}")))
     num_retries = 3
     while num_retries > 0:
         try:
-            with spec.package.stage as pkg_stage:
-                pkg_stage.cache_mirror(mirror, mirror_stats)
-                for patch in spec.package.all_patches():
+            with pkg_obj.stage as pkg_stage:
+                pkg_stage.cache_mirror(mirror_cache, mirror_stats)
+                for patch in pkg_obj.all_patches():
                     if patch.stage:
-                        patch.stage.cache_mirror(mirror, mirror_stats)
+                        patch.stage.cache_mirror(mirror_cache, mirror_stats)
                     patch.clean()
             exception = None
             break
@@ -669,18 +666,16 @@ def _add_single_spec(spec, mirror, mirror_stats):
             exc_tuple = sys.exc_info()
             exception = e
         num_retries -= 1
-
     if exception:
         if spack.config.get("config:debug"):
             traceback.print_exception(file=sys.stderr, *exc_tuple)
         else:
             tty.warn(
-                "Error while fetching %s" % spec.cformat("{name}{@version}"),
+                "Error while fetching %s" % pkg_obj.spec.cformat("{name}{@version}"),
                 getattr(exception, "message", exception),
             )
         mirror_stats.error()
         return False
-
     return True
 
 
diff --git a/lib/spack/spack/test/cmd/mirror.py b/lib/spack/spack/test/cmd/mirror.py
index c3e322487a..e64d54428a 100644
--- a/lib/spack/spack/test/cmd/mirror.py
+++ b/lib/spack/spack/test/cmd/mirror.py
@@ -8,6 +8,7 @@ import sys
 
 import pytest
 
+import spack.cmd.mirror
 import spack.config
 import spack.environment as ev
 from spack.main import SpackCommand, SpackCommandError
@@ -113,6 +114,7 @@ class MockMirrorArgs(object):
         dependencies=False,
         exclude_file=None,
         exclude_specs=None,
+        directory=None,
     ):
         self.specs = specs or []
         self.all = all
@@ -121,6 +123,7 @@ class MockMirrorArgs(object):
         self.dependencies = dependencies
         self.exclude_file = exclude_file
         self.exclude_specs = exclude_specs
+        self.directory = directory
 
 
 def test_exclude_specs(mock_packages, config):
@@ -128,11 +131,13 @@ def test_exclude_specs(mock_packages, config):
         specs=["mpich"], versions_per_spec="all", exclude_specs="mpich@3.0.1:3.0.2 mpich@1.0"
     )
 
-    mirror_specs = spack.cmd.mirror._determine_specs_to_mirror(args)
-    expected_include = set(spack.spec.Spec(x) for x in ["mpich@3.0.3", "mpich@3.0.4", "mpich@3.0"])
+    mirror_specs = spack.cmd.mirror.concrete_specs_from_user(args)
+    expected_include = set(
+        spack.spec.Spec(x).concretized() for x in ["mpich@3.0.3", "mpich@3.0.4", "mpich@3.0"]
+    )
     expected_exclude = set(spack.spec.Spec(x) for x in ["mpich@3.0.1", "mpich@3.0.2", "mpich@1.0"])
     assert expected_include <= set(mirror_specs)
-    assert not expected_exclude & set(mirror_specs)
+    assert not any(spec.satisfies(y) for spec in mirror_specs for y in expected_exclude)
 
 
 def test_exclude_file(mock_packages, tmpdir, config):
@@ -147,11 +152,13 @@ mpich@1.0
 
     args = MockMirrorArgs(specs=["mpich"], versions_per_spec="all", exclude_file=exclude_path)
 
-    mirror_specs = spack.cmd.mirror._determine_specs_to_mirror(args)
-    expected_include = set(spack.spec.Spec(x) for x in ["mpich@3.0.3", "mpich@3.0.4", "mpich@3.0"])
+    mirror_specs = spack.cmd.mirror.concrete_specs_from_user(args)
+    expected_include = set(
+        spack.spec.Spec(x).concretized() for x in ["mpich@3.0.3", "mpich@3.0.4", "mpich@3.0"]
+    )
     expected_exclude = set(spack.spec.Spec(x) for x in ["mpich@3.0.1", "mpich@3.0.2", "mpich@1.0"])
     assert expected_include <= set(mirror_specs)
-    assert not expected_exclude & set(mirror_specs)
+    assert not any(spec.satisfies(y) for spec in mirror_specs for y in expected_exclude)
 
 
 def test_mirror_crud(tmp_scope, capsys):
@@ -288,3 +295,118 @@ def test_mirror_destroy(
 
     uninstall("-y", spec_name)
     mirror("remove", "atest")
+
+
+@pytest.mark.usefixtures("mock_packages")
+class TestMirrorCreate(object):
+    @pytest.mark.regression("31736", "31985")
+    def test_all_specs_with_all_versions_dont_concretize(self):
+        args = MockMirrorArgs(exclude_file=None, exclude_specs=None)
+        specs = spack.cmd.mirror.all_specs_with_all_versions(
+            selection_fn=spack.cmd.mirror.not_excluded_fn(args)
+        )
+        assert all(not s.concrete for s in specs)
+
+    @pytest.mark.parametrize(
+        "cli_args,error_str",
+        [
+            # Passed more than one among -f --all and specs
+            ({"specs": "hdf5", "file": None, "all": True}, "cannot specify specs on command line"),
+            (
+                {"specs": None, "file": "input.txt", "all": True},
+                "cannot specify specs with a file if",
+            ),
+            (
+                {"specs": "hdf5", "file": "input.txt", "all": False},
+                "cannot specify specs with a file AND",
+            ),
+            ({"specs": None, "file": None, "all": False}, "no packages were specified"),
+            # Passed -n along with --all
+            (
+                {"specs": None, "file": None, "all": True, "versions_per_spec": 2},
+                "cannot specify '--versions_per-spec'",
+            ),
+        ],
+    )
+    def test_error_conditions(self, cli_args, error_str):
+        args = MockMirrorArgs(**cli_args)
+        with pytest.raises(spack.error.SpackError, match=error_str):
+            spack.cmd.mirror.mirror_create(args)
+
+    @pytest.mark.parametrize(
+        "cli_args,expected_end",
+        [
+            ({"directory": None}, os.path.join("source")),
+            ({"directory": os.path.join("foo", "bar")}, os.path.join("foo", "bar")),
+        ],
+    )
+    def test_mirror_path_is_valid(self, cli_args, expected_end, config):
+        args = MockMirrorArgs(**cli_args)
+        local_push_url = spack.cmd.mirror.local_mirror_url_from_user(args.directory)
+        assert local_push_url.startswith("file:")
+        assert os.path.isabs(local_push_url.replace("file://", ""))
+        assert local_push_url.endswith(expected_end)
+
+    @pytest.mark.parametrize(
+        "cli_args,not_expected",
+        [
+            (
+                {
+                    "specs": "boost bowtie callpath",
+                    "exclude_specs": "bowtie",
+                    "dependencies": False,
+                },
+                ["bowtie"],
+            ),
+            (
+                {
+                    "specs": "boost bowtie callpath",
+                    "exclude_specs": "bowtie callpath",
+                    "dependencies": False,
+                },
+                ["bowtie", "callpath"],
+            ),
+            (
+                {
+                    "specs": "boost bowtie callpath",
+                    "exclude_specs": "bowtie",
+                    "dependencies": True,
+                },
+                ["bowtie"],
+            ),
+        ],
+    )
+    def test_exclude_specs_from_user(self, cli_args, not_expected, config):
+        specs = spack.cmd.mirror.concrete_specs_from_user(MockMirrorArgs(**cli_args))
+        assert not any(s.satisfies(y) for s in specs for y in not_expected)
+
+    @pytest.mark.parametrize(
+        "abstract_specs",
+        [
+            ("bowtie", "callpath"),
+        ],
+    )
+    def test_specs_from_cli_are_the_same_as_from_file(self, abstract_specs, config, tmpdir):
+        args = MockMirrorArgs(specs=" ".join(abstract_specs))
+        specs_from_cli = spack.cmd.mirror.concrete_specs_from_user(args)
+
+        input_file = tmpdir.join("input.txt")
+        input_file.write("\n".join(abstract_specs))
+        args = MockMirrorArgs(file=str(input_file))
+        specs_from_file = spack.cmd.mirror.concrete_specs_from_user(args)
+
+        assert specs_from_cli == specs_from_file
+
+    @pytest.mark.parametrize(
+        "input_specs,nversions",
+        [
+            ("callpath", 1),
+            ("mpich", 4),
+            ("callpath mpich", 3),
+            ("callpath mpich", "all"),
+        ],
+    )
+    def test_versions_per_spec_produces_concrete_specs(self, input_specs, nversions, config):
+        args = MockMirrorArgs(specs=input_specs, versions_per_spec=nversions)
+        specs = spack.cmd.mirror.concrete_specs_from_user(args)
+        assert all(s.concrete for s in specs)
diff --git a/lib/spack/spack/test/mirror.py b/lib/spack/spack/test/mirror.py
index 191cbb9705..c156db867c 100644
--- a/lib/spack/spack/test/mirror.py
+++ b/lib/spack/spack/test/mirror.py
@@ -311,19 +311,3 @@ def test_get_all_versions(specs, expected_specs):
     output_list = [str(x) for x in output_list]
     # Compare sets since order is not important
     assert set(output_list) == set(expected_specs)
-
-
-@pytest.mark.regression("31736")
-def test_non_concretizable_spec_does_not_raise():
-    s = Spec("doesnotexist")
-
-    class Stats(object):
-        called = False
-
-        def error(self):
-            self.called = True
-
-    mirror_stats = Stats()
-    result = spack.mirror._add_single_spec(s, mirror=None, mirror_stats=mirror_stats)
-    assert result is False
-    assert mirror_stats.called is True
diff --git a/var/spack/repos/builtin/packages/rkt-base/package.py b/var/spack/repos/builtin/packages/rkt-base/package.py
index 4ae2eda51c..256f90106e 100644
--- a/var/spack/repos/builtin/packages/rkt-base/package.py
+++ b/var/spack/repos/builtin/packages/rkt-base/package.py
@@ -17,6 +17,6 @@ class RktBase(RacketPackage):
     version("8.3", commit="cab83438422bfea0e4bd74bc3e8305e6517cf25f")  # tag='v8.3'
     depends_on("racket@8.3", type=("build", "run"), when="@8.3")
 
-    name = "base"
+    racket_name = "base"
     pkgs = True
-    subdirectory = "pkgs/{0}".format(name)
+    subdirectory = "pkgs/{0}".format(racket_name)
diff --git a/var/spack/repos/builtin/packages/rkt-cext-lib/package.py b/var/spack/repos/builtin/packages/rkt-cext-lib/package.py
index 66de2376e8..8d9b257c05 100644
--- a/var/spack/repos/builtin/packages/rkt-cext-lib/package.py
+++ b/var/spack/repos/builtin/packages/rkt-cext-lib/package.py
@@ -20,6 +20,6 @@ class RktCextLib(RacketPackage):
     depends_on("rkt-dynext-lib@8.3", type=("build", "run"), when="@8.3")
     depends_on("rkt-scheme-lib@8.3", type=("build", "run"), when="@8.3")
 
-    name = "cext-lib"
+    racket_name = "cext-lib"
     pkgs = True
-    subdirectory = name
+    subdirectory = racket_name
diff --git a/var/spack/repos/builtin/packages/rkt-compiler-lib/package.py b/var/spack/repos/builtin/packages/rkt-compiler-lib/package.py
index 17ca12ab09..fee9b3ea9a 100644
--- a/var/spack/repos/builtin/packages/rkt-compiler-lib/package.py
+++ b/var/spack/repos/builtin/packages/rkt-compiler-lib/package.py
@@ -20,6 +20,6 @@ class RktCompilerLib(RacketPackage):
     depends_on("rkt-rackunit-lib@8.3", type=("build", "run"), when="@8.3")
     depends_on("rkt-zo-lib@1.3", type=("build", "run"), when="@8.3")
 
-    name = "compiler-lib"
+    racket_name = "compiler-lib"
     pkgs = True
-    subdirectory = "pkgs/{0}".format(name)
+    subdirectory = "pkgs/{0}".format(racket_name)
diff --git a/var/spack/repos/builtin/packages/rkt-dynext-lib/package.py b/var/spack/repos/builtin/packages/rkt-dynext-lib/package.py
index a235aca112..81f7ca811f 100644
--- a/var/spack/repos/builtin/packages/rkt-dynext-lib/package.py
+++ b/var/spack/repos/builtin/packages/rkt-dynext-lib/package.py
@@ -16,6 +16,6 @@ class RktDynextLib(RacketPackage):
     version("8.3", commit="cc22e2456df881a9008240d70dd9012ef37395f5")  # tag = 'v8.3'
     depends_on("rkt-base@8.3", type=("build", "run"), when="@8.3")
 
-    name = "dynext-lib"
+    racket_name = "dynext-lib"
     pkgs = True
-    subdirectory = name
+    subdirectory = racket_name
diff --git a/var/spack/repos/builtin/packages/rkt-rackunit-lib/package.py b/var/spack/repos/builtin/packages/rkt-rackunit-lib/package.py
index e7661eec09..783aebe306 100644
--- a/var/spack/repos/builtin/packages/rkt-rackunit-lib/package.py
+++ b/var/spack/repos/builtin/packages/rkt-rackunit-lib/package.py
@@ -17,6 +17,6 @@ class RktRackunitLib(RacketPackage):
     depends_on("rkt-base@8.3:", type=("build", "run"), when="@8.3")
     depends_on("rkt-testing-util-lib@8.3", type=("build", "run"), when="@8.3")
 
-    name = "rackunit-lib"
+    racket_name = "rackunit-lib"
     pkgs = True
-    subdirectory = name
+    subdirectory = racket_name
diff --git a/var/spack/repos/builtin/packages/rkt-scheme-lib/package.py b/var/spack/repos/builtin/packages/rkt-scheme-lib/package.py
index 1765318158..75346eac70 100644
--- a/var/spack/repos/builtin/packages/rkt-scheme-lib/package.py
+++ b/var/spack/repos/builtin/packages/rkt-scheme-lib/package.py
@@ -16,5 +16,5 @@ class RktSchemeLib(RacketPackage):
     version("8.3", commit="a36e729680818712820ee5269f5208c3c0715a6a")  # tag='v8.3'
     depends_on("rkt-base@8.3", type=("build", "run"), when="@8.3")
 
-    name = "scheme-lib"
+    racket_name = "scheme-lib"
     pkgs = True
diff --git a/var/spack/repos/builtin/packages/rkt-testing-util-lib/package.py b/var/spack/repos/builtin/packages/rkt-testing-util-lib/package.py
index aef3488150..9186845fc9 100644
--- a/var/spack/repos/builtin/packages/rkt-testing-util-lib/package.py
+++ b/var/spack/repos/builtin/packages/rkt-testing-util-lib/package.py
@@ -16,6 +16,6 @@ class RktTestingUtilLib(RacketPackage):
     version("8.3", commit="683237bee2a979c7b1541092922fb51a75ea8ca9")  # tag='v8.3'
     depends_on("rkt-base@8.3:", type=("build", "run"), when="@8.3")
 
-    name = "testing-util-lib"
+    racket_name = "testing-util-lib"
     pkgs = True
-    subdirectory = name
+    subdirectory = racket_name
diff --git a/var/spack/repos/builtin/packages/rkt-zo-lib/package.py b/var/spack/repos/builtin/packages/rkt-zo-lib/package.py
index ae2373785a..66f3d498af 100644
--- a/var/spack/repos/builtin/packages/rkt-zo-lib/package.py
+++ b/var/spack/repos/builtin/packages/rkt-zo-lib/package.py
@@ -16,6 +16,6 @@ class RktZoLib(RacketPackage):
     version("1.3", commit="cab83438422bfea0e4bd74bc3e8305e6517cf25f")  # tag='v1.3'
     depends_on("rkt-base@8.3:", type=("build", "run"), when="@1.3")
 
-    name = "zo-lib"
+    racket_name = "zo-lib"
     pkgs = True
-    subdirectory = "pkgs/{0}".format(name)
+    subdirectory = "pkgs/{0}".format(racket_name)