mirror: fetch by digest (#45809)

Source mirrors store entries by digest and add a human readable alias of the
form 'name-version'. If no digest is available, the alias is used as the primary
storage location.

Spack erroneously fetches by alias when the digest path does not exist. This is
problematic if `version(..., sha256=...)` changes in package.py, and the mirror
is populated with the old shasum. That would result in an error when a digest
is available, but in case of git versions with a modified commit sha, the wrong
sources would be fetched without error. With this PR, only the digest path is
used, not the alias, in case a digest is available. This is also a small performance
optimization, as the number of request is halved for mirrors that don't contain
the sources.

Further, for git sources the tag was used as a digest, but this is a moving
target. Only commit sha is used now.

Also whenever the alias already existed, Spack used to keep it in place when
updating the mirror cache, which means that aliases would always point to
outdated mirror entries whenever digests are modified. With this PR the alias
is moved in place.

Lastly, fix a recent regression where `Stage.disable_mirrors` disabled mirrors
but not the local download cache, which was the intention.

8 files changed, 173 insertions, 182 deletions

diff --git a/lib/spack/spack/caches.py b/lib/spack/spack/caches.py
index 14d03a14b3..eda141ed15 100644
--- a/lib/spack/spack/caches.py
+++ b/lib/spack/spack/caches.py
@@ -9,11 +9,11 @@ from typing import Union
 
 import llnl.util.lang
 from llnl.util.filesystem import mkdirp
-from llnl.util.symlink import symlink
 
 import spack.config
 import spack.error
 import spack.fetch_strategy
+import spack.mirror
 import spack.paths
 import spack.util.file_cache
 import spack.util.path
@@ -74,23 +74,6 @@ class MirrorCache:
         mkdirp(os.path.dirname(dst))
         fetcher.archive(dst)
 
-    def symlink(self, mirror_ref):
-        """Symlink a human readible path in our mirror to the actual
-        storage location."""
-
-        cosmetic_path = os.path.join(self.root, mirror_ref.cosmetic_path)
-        storage_path = os.path.join(self.root, mirror_ref.storage_path)
-        relative_dst = os.path.relpath(storage_path, start=os.path.dirname(cosmetic_path))
-
-        if not os.path.exists(cosmetic_path):
-            if os.path.lexists(cosmetic_path):
-                # In this case the link itself exists but it is broken: remove
-                # it and recreate it (in order to fix any symlinks broken prior
-                # to https://github.com/spack/spack/pull/13908)
-                os.unlink(cosmetic_path)
-            mkdirp(os.path.dirname(cosmetic_path))
-            symlink(relative_dst, cosmetic_path)
-
 
 #: Spack's local cache for downloaded source archives
 FETCH_CACHE: Union[spack.fetch_strategy.FsCache, llnl.util.lang.Singleton] = (
diff --git a/lib/spack/spack/cmd/develop.py b/lib/spack/spack/cmd/develop.py
index e226c16766..cc181d9a92 100644
--- a/lib/spack/spack/cmd/develop.py
+++ b/lib/spack/spack/cmd/develop.py
@@ -10,8 +10,10 @@ import llnl.util.tty as tty
 import spack.cmd
 import spack.config
 import spack.fetch_strategy
+import spack.package_base
 import spack.repo
 import spack.spec
+import spack.stage
 import spack.util.path
 import spack.version
 from spack.cmd.common import arguments
@@ -62,7 +64,7 @@ def _update_config(spec, path):
     spack.config.change_or_add("develop", find_fn, change_fn)
 
 
-def _retrieve_develop_source(spec, abspath):
+def _retrieve_develop_source(spec: spack.spec.Spec, abspath: str) -> None:
     # "steal" the source code via staging API. We ask for a stage
     # to be created, then copy it afterwards somewhere else. It would be
     # better if we can create the `source_path` directly into its final
@@ -71,13 +73,13 @@ def _retrieve_develop_source(spec, abspath):
     # We construct a package class ourselves, rather than asking for
     # Spec.package, since Spec only allows this when it is concrete
     package = pkg_cls(spec)
-    source_stage = package.stage[0]
+    source_stage: spack.stage.Stage = package.stage[0]
     if isinstance(source_stage.fetcher, spack.fetch_strategy.GitFetchStrategy):
         source_stage.fetcher.get_full_repo = True
         # If we retrieved this version before and cached it, we may have
         # done so without cloning the full git repo; likewise, any
         # mirror might store an instance with truncated history.
-        source_stage.disable_mirrors()
+        source_stage.default_fetcher_only = True
 
     source_stage.fetcher.set_package(package)
     package.stage.steal_source(abspath)
diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 3b22cfb94c..f163f2cb34 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -574,18 +574,18 @@ class VCSFetchStrategy(FetchStrategy):
         # Set a URL based on the type of fetch strategy.
         self.url = kwargs.get(self.url_attr, None)
         if not self.url:
-            raise ValueError("%s requires %s argument." % (self.__class__, self.url_attr))
+            raise ValueError(f"{self.__class__} requires {self.url_attr} argument.")
 
         for attr in self.optional_attrs:
             setattr(self, attr, kwargs.get(attr, None))
 
     @_needs_stage
     def check(self):
-        tty.debug("No checksum needed when fetching with {0}".format(self.url_attr))
+        tty.debug(f"No checksum needed when fetching with {self.url_attr}")
 
     @_needs_stage
     def expand(self):
-        tty.debug("Source fetched with %s is already expanded." % self.url_attr)
+        tty.debug(f"Source fetched with {self.url_attr} is already expanded.")
 
     @_needs_stage
     def archive(self, destination, *, exclude: Optional[str] = None):
@@ -605,10 +605,10 @@ class VCSFetchStrategy(FetchStrategy):
             )
 
     def __str__(self):
-        return "VCS: %s" % self.url
+        return f"VCS: {self.url}"
 
     def __repr__(self):
-        return "%s<%s>" % (self.__class__, self.url)
+        return f"{self.__class__}<{self.url}>"
 
 
 @fetcher
@@ -717,6 +717,11 @@ class GitFetchStrategy(VCSFetchStrategy):
     git_version_re = r"git version (\S+)"
 
     def __init__(self, **kwargs):
+
+        self.commit: Optional[str] = None
+        self.tag: Optional[str] = None
+        self.branch: Optional[str] = None
+
         # Discards the keywords in kwargs that may conflict with the next call
         # to __init__
         forwarded_args = copy.copy(kwargs)
@@ -765,43 +770,42 @@ class GitFetchStrategy(VCSFetchStrategy):
 
     @property
     def cachable(self):
-        return self.cache_enabled and bool(self.commit or self.tag)
+        return self.cache_enabled and bool(self.commit)
 
     def source_id(self):
-        return self.commit or self.tag
+        # TODO: tree-hash would secure download cache and mirrors, commit only secures checkouts.
+        return self.commit
 
     def mirror_id(self):
-        repo_ref = self.commit or self.tag or self.branch
-        if repo_ref:
+        if self.commit:
             repo_path = urllib.parse.urlparse(self.url).path
-            result = os.path.sep.join(["git", repo_path, repo_ref])
+            result = os.path.sep.join(["git", repo_path, self.commit])
             return result
 
     def _repo_info(self):
         args = ""
-
         if self.commit:
-            args = " at commit {0}".format(self.commit)
+            args = f" at commit {self.commit}"
         elif self.tag:
-            args = " at tag {0}".format(self.tag)
+            args = f" at tag {self.tag}"
         elif self.branch:
-            args = " on branch {0}".format(self.branch)
+            args = f" on branch {self.branch}"
 
-        return "{0}{1}".format(self.url, args)
+        return f"{self.url}{args}"
 
     @_needs_stage
     def fetch(self):
         if self.stage.expanded:
-            tty.debug("Already fetched {0}".format(self.stage.source_path))
+            tty.debug(f"Already fetched {self.stage.source_path}")
             return
 
         if self.git_sparse_paths:
-            self._sparse_clone_src(commit=self.commit, branch=self.branch, tag=self.tag)
+            self._sparse_clone_src()
         else:
-            self._clone_src(commit=self.commit, branch=self.branch, tag=self.tag)
+            self._clone_src()
         self.submodule_operations()
 
-    def bare_clone(self, dest):
+    def bare_clone(self, dest: str) -> None:
         """
         Execute a bare clone for metadata only
 
@@ -809,7 +813,7 @@ class GitFetchStrategy(VCSFetchStrategy):
         and shouldn't be used for staging.
         """
         # Default to spack source path
-        tty.debug("Cloning git repository: {0}".format(self._repo_info()))
+        tty.debug(f"Cloning git repository: {self._repo_info()}")
 
         git = self.git
         debug = spack.config.get("config:debug")
@@ -821,24 +825,16 @@ class GitFetchStrategy(VCSFetchStrategy):
         clone_args.extend([self.url, dest])
         git(*clone_args)
 
-    def _clone_src(self, commit=None, branch=None, tag=None):
-        """
-        Clone a repository to a path using git.
-
-        Arguments:
-            commit (str or None): A commit to fetch from the remote. Only one of
-                commit, branch, and tag may be non-None.
-            branch (str or None): A branch to fetch from the remote.
-            tag (str or None): A tag to fetch from the remote.
-        """
+    def _clone_src(self) -> None:
+        """Clone a repository to a path using git."""
         # Default to spack source path
         dest = self.stage.source_path
-        tty.debug("Cloning git repository: {0}".format(self._repo_info()))
+        tty.debug(f"Cloning git repository: {self._repo_info()}")
 
         git = self.git
         debug = spack.config.get("config:debug")
 
-        if commit:
+        if self.commit:
             # Need to do a regular clone and check out everything if
             # they asked for a particular commit.
             clone_args = ["clone", self.url]
@@ -857,7 +853,7 @@ class GitFetchStrategy(VCSFetchStrategy):
                 )
 
             with working_dir(dest):
-                checkout_args = ["checkout", commit]
+                checkout_args = ["checkout", self.commit]
                 if not debug:
                     checkout_args.insert(1, "--quiet")
                 git(*checkout_args)
@@ -869,10 +865,10 @@ class GitFetchStrategy(VCSFetchStrategy):
                 args.append("--quiet")
 
             # If we want a particular branch ask for it.
-            if branch:
-                args.extend(["--branch", branch])
-            elif tag and self.git_version >= spack.version.Version("1.8.5.2"):
-                args.extend(["--branch", tag])
+            if self.branch:
+                args.extend(["--branch", self.branch])
+            elif self.tag and self.git_version >= spack.version.Version("1.8.5.2"):
+                args.extend(["--branch", self.tag])
 
             # Try to be efficient if we're using a new enough git.
             # This checks out only one branch's history
@@ -904,7 +900,7 @@ class GitFetchStrategy(VCSFetchStrategy):
                 # For tags, be conservative and check them out AFTER
                 # cloning.  Later git versions can do this with clone
                 # --branch, but older ones fail.
-                if tag and self.git_version < spack.version.Version("1.8.5.2"):
+                if self.tag and self.git_version < spack.version.Version("1.8.5.2"):
                     # pull --tags returns a "special" error code of 1 in
                     # older versions that we have to ignore.
                     # see: https://github.com/git/git/commit/19d122b
@@ -917,16 +913,8 @@ class GitFetchStrategy(VCSFetchStrategy):
                     git(*pull_args, ignore_errors=1)
                     git(*co_args)
 
-    def _sparse_clone_src(self, commit=None, branch=None, tag=None, **kwargs):
-        """
-        Use git's sparse checkout feature to clone portions of a git repository
-
-        Arguments:
-            commit (str or None): A commit to fetch from the remote. Only one of
-                commit, branch, and tag may be non-None.
-            branch (str or None): A branch to fetch from the remote.
-            tag (str or None): A tag to fetch from the remote.
-        """
+    def _sparse_clone_src(self, **kwargs):
+        """Use git's sparse checkout feature to clone portions of a git repository"""
         dest = self.stage.source_path
         git = self.git
 
@@ -945,12 +933,12 @@ class GitFetchStrategy(VCSFetchStrategy):
                     "Cloning the full repository instead."
                 )
             )
-            self._clone_src(commit, branch, tag)
+            self._clone_src()
         else:
             # default to depth=2 to allow for retention of some git properties
             depth = kwargs.get("depth", 2)
-            needs_fetch = branch or tag
-            git_ref = branch or tag or commit
+            needs_fetch = self.branch or self.tag
+            git_ref = self.branch or self.tag or self.commit
 
             assert git_ref
 
@@ -1050,7 +1038,7 @@ class GitFetchStrategy(VCSFetchStrategy):
         return not (self.url.startswith("http://") or self.url.startswith("/"))
 
     def __str__(self):
-        return "[git] {0}".format(self._repo_info())
+        return f"[git] {self._repo_info()}"
 
 
 @fetcher
@@ -1668,7 +1656,7 @@ def for_package_version(pkg, version=None):
     raise InvalidArgsError(pkg, version, **args)
 
 
-def from_url_scheme(url: str, **kwargs):
+def from_url_scheme(url: str, **kwargs) -> FetchStrategy:
     """Finds a suitable FetchStrategy by matching its url_attr with the scheme
     in the given url."""
     parsed_url = urllib.parse.urlparse(url, scheme="file")
diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py
index af2a179dd0..8caa272137 100644
--- a/lib/spack/spack/mirror.py
+++ b/lib/spack/spack/mirror.py
@@ -21,6 +21,7 @@ import urllib.parse
 from typing import List, Optional, Union
 
 import llnl.url
+import llnl.util.symlink
 import llnl.util.tty as tty
 from llnl.util.filesystem import mkdirp
 
@@ -30,6 +31,7 @@ import spack.error
 import spack.fetch_strategy
 import spack.mirror
 import spack.oci.image
+import spack.repo
 import spack.spec
 import spack.util.path
 import spack.util.spack_json as sjson
@@ -427,27 +429,58 @@ Spack not to expand it with the following syntax:
 
 
 class MirrorLayout:
-    """A ``MirrorLayout`` stores the relative locations of files in a mirror directory. The main
-    storage location is ``storage_path``. An additional, human-readable path may be obtained as the
-    second entry when iterating this object."""
+    """A ``MirrorLayout`` object describes the relative path of a mirror entry."""
 
-    def __init__(self, storage_path: str) -> None:
-        self.storage_path = storage_path
+    def __init__(self, path: str) -> None:
+        self.path = path
 
     def __iter__(self):
-        yield self.storage_path
+        """Yield all paths including aliases where the resource can be found."""
+        yield self.path
+
+    def make_alias(self, root: str) -> None:
+        """Make the entry ``root / self.path`` available under a human readable alias"""
+        pass
 
 
 class DefaultLayout(MirrorLayout):
-    def __init__(self, cosmetic_path: str, global_path: Optional[str] = None) -> None:
-        super().__init__(global_path or cosmetic_path)
-        self.global_path = global_path
-        self.cosmetic_path = cosmetic_path
+    def __init__(self, alias_path: str, digest_path: Optional[str] = None) -> None:
+        # When we have a digest, it is used as the primary storage location. If not, then we use
+        # the human-readable alias. In case of mirrors of a VCS checkout, we currently do not have
+        # a digest, that's why an alias is required and a digest optional.
+        super().__init__(path=digest_path or alias_path)
+        self.alias = alias_path
+        self.digest_path = digest_path
+
+    def make_alias(self, root: str) -> None:
+        """Symlink a human readible path in our mirror to the actual storage location."""
+        # We already use the human-readable path as the main storage location.
+        if not self.digest_path:
+            return
+
+        alias, digest = os.path.join(root, self.alias), os.path.join(root, self.digest_path)
+
+        alias_dir = os.path.dirname(alias)
+        relative_dst = os.path.relpath(digest, start=alias_dir)
+
+        mkdirp(alias_dir)
+        tmp = f"{alias}.tmp"
+        llnl.util.symlink.symlink(relative_dst, tmp)
+
+        try:
+            os.rename(tmp, alias)
+        except OSError:
+            # Clean up the temporary if possible
+            try:
+                os.unlink(tmp)
+            except OSError:
+                pass
+            raise
 
     def __iter__(self):
-        if self.global_path:
-            yield self.global_path
-        yield self.cosmetic_path
+        if self.digest_path:
+            yield self.digest_path
+        yield self.alias
 
 
 class OCILayout(MirrorLayout):
@@ -458,7 +491,11 @@ class OCILayout(MirrorLayout):
         super().__init__(os.path.join("blobs", digest.algorithm, digest.digest))
 
 
-def mirror_archive_paths(fetcher, per_package_ref, spec=None):
+def default_mirror_layout(
+    fetcher: "spack.fetch_strategy.FetchStrategy",
+    per_package_ref: str,
+    spec: Optional["spack.spec.Spec"] = None,
+) -> MirrorLayout:
     """Returns a ``MirrorReference`` object which keeps track of the relative
     storage path of the resource associated with the specified ``fetcher``."""
     ext = None
diff --git a/lib/spack/spack/package_base.py b/lib/spack/spack/package_base.py
index f4b7c2b4c1..3fa4a86143 100644
--- a/lib/spack/spack/package_base.py
+++ b/lib/spack/spack/package_base.py
@@ -741,7 +741,7 @@ class PackageBase(WindowsRPath, PackageViewMixin, RedistributionMixin, metaclass
             raise ValueError(msg.format(self))
 
         # init internal variables
-        self._stage = None
+        self._stage: Optional[StageComposite] = None
         self._fetcher = None
         self._tester: Optional["PackageTest"] = None
 
@@ -1099,7 +1099,7 @@ class PackageBase(WindowsRPath, PackageViewMixin, RedistributionMixin, metaclass
             root=root_stage,
             resource=resource,
             name=self._resource_stage(resource),
-            mirror_paths=spack.mirror.mirror_archive_paths(
+            mirror_paths=spack.mirror.default_mirror_layout(
                 resource.fetcher, os.path.join(self.name, pretty_resource_name)
             ),
             mirrors=spack.mirror.MirrorCollection(source=True).values(),
@@ -1114,7 +1114,7 @@ class PackageBase(WindowsRPath, PackageViewMixin, RedistributionMixin, metaclass
         # Construct a mirror path (TODO: get this out of package.py)
         format_string = "{name}-{version}"
         pretty_name = self.spec.format_path(format_string)
-        mirror_paths = spack.mirror.mirror_archive_paths(
+        mirror_paths = spack.mirror.default_mirror_layout(
             fetcher, os.path.join(self.name, pretty_name), self.spec
         )
         # Construct a path where the stage should build..
@@ -1180,7 +1180,7 @@ class PackageBase(WindowsRPath, PackageViewMixin, RedistributionMixin, metaclass
         return self._stage
 
     @stage.setter
-    def stage(self, stage):
+    def stage(self, stage: StageComposite):
         """Allow a stage object to be set to override the default."""
         self._stage = stage
 
diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py
index b2630f7c00..68f3f47bb2 100644
--- a/lib/spack/spack/patch.py
+++ b/lib/spack/spack/patch.py
@@ -326,7 +326,7 @@ class UrlPatch(Patch):
         name = "{0}-{1}".format(os.path.basename(self.url), fetch_digest[:7])
 
         per_package_ref = os.path.join(self.owner.split(".")[-1], name)
-        mirror_ref = spack.mirror.mirror_archive_paths(fetcher, per_package_ref)
+        mirror_ref = spack.mirror.default_mirror_layout(fetcher, per_package_ref)
         self._stage = spack.stage.Stage(
             fetcher,
             name=f"{spack.stage.stage_prefix}patch-{fetch_digest}",
diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py
index 8956a4e933..752ac42e4e 100644
--- a/lib/spack/spack/stage.py
+++ b/lib/spack/spack/stage.py
@@ -364,36 +364,30 @@ class Stage(LockableStagingDir):
         """Create a stage object.
         Parameters:
           url_or_fetch_strategy
-              URL of the archive to be downloaded into this stage, OR
-              a valid FetchStrategy.
+              URL of the archive to be downloaded into this stage, OR a valid FetchStrategy.
 
           name
-              If a name is provided, then this stage is a named stage
-              and will persist between runs (or if you construct another
-              stage object later).  If name is not provided, then this
+              If a name is provided, then this stage is a named stage and will persist between runs
+              (or if you construct another stage object later).  If name is not provided, then this
               stage will be given a unique name automatically.
 
           mirror_paths
-              If provided, Stage will search Spack's mirrors for
-              this archive at each of the provided relative mirror paths
-              before using the default fetch strategy.
+              If provided, Stage will search Spack's mirrors for this archive at each of the
+              provided relative mirror paths before using the default fetch strategy.
 
           keep
-              By default, when used as a context manager, the Stage
-              is deleted on exit when no exceptions are raised.
-              Pass True to keep the stage intact even if no
-              exceptions are raised.
+              By default, when used as a context manager, the Stage is deleted on exit when no
+              exceptions are raised. Pass True to keep the stage intact even if no exceptions are
+              raised.
 
          path
               If provided, the stage path to use for associated builds.
 
          lock
-              True if the stage directory file lock is to be used, False
-              otherwise.
+              True if the stage directory file lock is to be used, False otherwise.
 
          search_fn
-              The search function that provides the fetch strategy
-              instance.
+              The search function that provides the fetch strategy instance.
         """
         super().__init__(name, path, keep, lock)
 
@@ -419,26 +413,27 @@ class Stage(LockableStagingDir):
 
         self.srcdir = None
 
-        self.mirror_paths = mirror_paths
+        self.mirror_layout = mirror_paths
         self.mirrors = list(mirrors) if mirrors else []
+        # Allow users the disable both mirrors and download cache
+        self.default_fetcher_only = False
 
     @property
     def expected_archive_files(self):
         """Possible archive file paths."""
-        paths = []
         fnames = []
         expanded = True
         if isinstance(self.default_fetcher, fs.URLFetchStrategy):
             expanded = self.default_fetcher.expand_archive
             fnames.append(url_util.default_download_filename(self.default_fetcher.url))
 
-        if self.mirror_paths:
-            fnames.extend(os.path.basename(x) for x in self.mirror_paths)
+        if self.mirror_layout:
+            fnames.append(os.path.basename(self.mirror_layout.path))
 
-        paths.extend(os.path.join(self.path, f) for f in fnames)
+        paths = [os.path.join(self.path, f) for f in fnames]
         if not expanded:
-            # If the download file is not compressed, the "archive" is a
-            # single file placed in Stage.source_path
+            # If the download file is not compressed, the "archive" is a single file placed in
+            # Stage.source_path
             paths.extend(os.path.join(self.source_path, f) for f in fnames)
 
         return paths
@@ -471,14 +466,8 @@ class Stage(LockableStagingDir):
         """Returns the well-known source directory path."""
         return os.path.join(self.path, _source_path_subdir)
 
-    def disable_mirrors(self):
-        """The Stage will not attempt to look for the associated fetcher
-        target in any of Spack's mirrors (including the local download cache).
-        """
-        self.mirror_paths = None
-
     def _generate_fetchers(self, mirror_only=False) -> Generator[fs.FetchStrategy, None, None]:
-        fetchers = []
+        fetchers: List[fs.FetchStrategy] = []
         if not mirror_only:
             fetchers.append(self.default_fetcher)
 
@@ -495,27 +484,26 @@ class Stage(LockableStagingDir):
         # TODO: move mirror logic out of here and clean it up!
         # TODO: Or @alalazo may have some ideas about how to use a
         # TODO: CompositeFetchStrategy here.
-        if self.mirror_paths and self.mirrors:
+        if not self.default_fetcher_only and self.mirror_layout and self.mirrors:
             # Add URL strategies for all the mirrors with the digest
             # Insert fetchers in the order that the URLs are provided.
             fetchers[:0] = (
                 fs.from_url_scheme(
-                    url_util.join(mirror.fetch_url, rel_path),
+                    url_util.join(mirror.fetch_url, self.mirror_layout.path),
                     checksum=digest,
                     expand=expand,
                     extension=extension,
                 )
                 for mirror in self.mirrors
-                if not mirror.fetch_url.startswith("oci://")
-                for rel_path in self.mirror_paths
+                if not mirror.fetch_url.startswith("oci://")  # no support for mirrors yet
             )
 
-        if self.mirror_paths and self.default_fetcher.cachable:
-            fetchers[:0] = (
+        if not self.default_fetcher_only and self.mirror_layout and self.default_fetcher.cachable:
+            fetchers.insert(
+                0,
                 spack.caches.FETCH_CACHE.fetcher(
-                    rel_path, digest, expand=expand, extension=extension
-                )
-                for rel_path in self.mirror_paths
+                    self.mirror_layout.path, digest, expand=expand, extension=extension
+                ),
             )
 
         yield from fetchers
@@ -611,41 +599,42 @@ class Stage(LockableStagingDir):
             self.fetcher.check()
 
     def cache_local(self):
-        spack.caches.FETCH_CACHE.store(self.fetcher, self.mirror_paths.storage_path)
+        spack.caches.FETCH_CACHE.store(self.fetcher, self.mirror_layout.path)
 
-    def cache_mirror(self, mirror, stats):
+    def cache_mirror(
+        self, mirror: spack.caches.MirrorCache, stats: spack.mirror.MirrorStats
+    ) -> None:
         """Perform a fetch if the resource is not already cached
 
         Arguments:
-            mirror (spack.caches.MirrorCache): the mirror to cache this Stage's
-                resource in
-            stats (spack.mirror.MirrorStats): this is updated depending on whether the
-                caching operation succeeded or failed
+            mirror: the mirror to cache this Stage's resource in
+            stats: 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.
-            # There is currently no method available on the fetcher to
-            # distinguish this ('cachable' refers to whether the fetcher
-            # refers to a resource with a fixed ID, which is not the same
-            # concept as whether there is anything to fetch at all) so we
-            # must examine the type of the fetcher.
+            # BundleFetchStrategy has no source to fetch. The associated fetcher does nothing but
+            # the associated stage may still exist. There is currently no method available on the
+            # fetcher to distinguish this ('cachable' refers to whether the fetcher refers to a
+            # resource with a fixed ID, which is not the same concept as whether there is anything
+            # to fetch at all) so we must examine the type of the fetcher.
+            return
+
+        elif mirror.skip_unstable_versions and not fs.stable_target(self.default_fetcher):
             return
 
-        if mirror.skip_unstable_versions and not fs.stable_target(self.default_fetcher):
+        elif not self.mirror_layout:
             return
 
-        absolute_storage_path = os.path.join(mirror.root, self.mirror_paths.storage_path)
+        absolute_storage_path = os.path.join(mirror.root, self.mirror_layout.path)
 
         if os.path.exists(absolute_storage_path):
             stats.already_existed(absolute_storage_path)
         else:
             self.fetch()
             self.check()
-            mirror.store(self.fetcher, self.mirror_paths.storage_path)
+            mirror.store(self.fetcher, self.mirror_layout.path)
             stats.added(absolute_storage_path)
 
-        mirror.symlink(self.mirror_paths)
+        self.mirror_layout.make_alias(mirror.root)
 
     def expand_archive(self):
         """Changes to the stage directory and attempt to expand the downloaded
@@ -653,9 +642,9 @@ class Stage(LockableStagingDir):
         downloaded."""
         if not self.expanded:
             self.fetcher.expand()
-            tty.debug("Created stage in {0}".format(self.path))
+            tty.debug(f"Created stage in {self.path}")
         else:
-            tty.debug("Already staged {0} in {1}".format(self.name, self.path))
+            tty.debug(f"Already staged {self.name} in {self.path}")
 
     def restage(self):
         """Removes the expanded archive path if it exists, then re-expands
diff --git a/lib/spack/spack/test/mirror.py b/lib/spack/spack/test/mirror.py
index 2c389538ef..5609595f46 100644
--- a/lib/spack/spack/test/mirror.py
+++ b/lib/spack/spack/test/mirror.py
@@ -11,15 +11,16 @@ import pytest
 from llnl.util.symlink import resolve_link_target_relative_to_the_link
 
 import spack.caches
+import spack.config
 import spack.fetch_strategy
 import spack.mirror
 import spack.patch
 import spack.repo
+import spack.stage
 import spack.util.executable
 import spack.util.spack_json as sjson
 import spack.util.url as url_util
 from spack.spec import Spec
-from spack.stage import Stage
 from spack.util.executable import which
 from spack.util.spack_yaml import SpackYAMLError
 
@@ -51,7 +52,7 @@ def set_up_package(name, repository, url_attr):
 
 
 def check_mirror():
-    with Stage("spack-mirror-test") as stage:
+    with spack.stage.Stage("spack-mirror-test") as stage:
         mirror_root = os.path.join(stage.path, "test-mirror")
         # register mirror with spack config
         mirrors = {"spack-mirror-test": url_util.path_to_file_url(mirror_root)}
@@ -66,8 +67,8 @@ def check_mirror():
             for spec in specs:
                 fetcher = spec.package.fetcher
                 per_package_ref = os.path.join(spec.name, "-".join([spec.name, str(spec.version)]))
-                mirror_paths = spack.mirror.mirror_archive_paths(fetcher, per_package_ref)
-                expected_path = os.path.join(mirror_root, mirror_paths.storage_path)
+                mirror_layout = spack.mirror.default_mirror_layout(fetcher, per_package_ref)
+                expected_path = os.path.join(mirror_root, mirror_layout.path)
                 assert os.path.exists(expected_path)
 
             # Now try to fetch each package.
@@ -203,13 +204,11 @@ def test_invalid_json_mirror_collection(invalid_json, error_message):
 def test_mirror_archive_paths_no_version(mock_packages, mock_archive):
     spec = Spec("trivial-install-test-package@=nonexistingversion").concretized()
     fetcher = spack.fetch_strategy.URLFetchStrategy(url=mock_archive.url)
-    spack.mirror.mirror_archive_paths(fetcher, "per-package-ref", spec)
+    spack.mirror.default_mirror_layout(fetcher, "per-package-ref", spec)
 
 
 def test_mirror_with_url_patches(mock_packages, monkeypatch):
-    spec = Spec("patch-several-dependencies")
-    spec.concretize()
-
+    spec = Spec("patch-several-dependencies").concretized()
     files_cached_in_mirror = set()
 
     def record_store(_class, fetcher, relative_dst, cosmetic_path=None):
@@ -228,30 +227,25 @@ def test_mirror_with_url_patches(mock_packages, monkeypatch):
     def successful_apply(*args, **kwargs):
         pass
 
-    def successful_symlink(*args, **kwargs):
+    def successful_make_alias(*args, **kwargs):
         pass
 
-    with Stage("spack-mirror-test") as stage:
+    with spack.stage.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, "apply_patch", successful_apply)
         monkeypatch.setattr(spack.caches.MirrorCache, "store", record_store)
-        monkeypatch.setattr(spack.caches.MirrorCache, "symlink", successful_symlink)
+        monkeypatch.setattr(spack.mirror.DefaultLayout, "make_alias", successful_make_alias)
 
         with spack.config.override("config:checksum", False):
             spack.mirror.create(mirror_root, list(spec.traverse()))
 
-        assert not (
-            set(
-                [
-                    "abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234",
-                    "abcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcd.gz",
-                ]
-            )
-            - files_cached_in_mirror
-        )
+        assert {
+            "abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234",
+            "abcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcd.gz",
+        }.issubset(files_cached_in_mirror)
 
 
 class MockFetcher:
@@ -266,23 +260,21 @@ class MockFetcher:
 
 
 @pytest.mark.regression("14067")
-def test_mirror_cache_symlinks(tmpdir):
+def test_mirror_layout_make_alias(tmpdir):
     """Confirm that the cosmetic symlink created in the mirror cache (which may
     be relative) targets the storage path correctly.
     """
-    cosmetic_path = os.path.join("zlib", "zlib-1.2.11.tar.gz")
-    global_path = os.path.join("_source-cache", "archive", "c3", "c3e5.tar.gz")
-    cache = spack.caches.MirrorCache(str(tmpdir), False)
-    reference = spack.mirror.DefaultLayout(cosmetic_path, global_path)
+    alias = os.path.join("zlib", "zlib-1.2.11.tar.gz")
+    path = os.path.join("_source-cache", "archive", "c3", "c3e5.tar.gz")
+    cache = spack.caches.MirrorCache(root=str(tmpdir), skip_unstable_versions=False)
+    layout = spack.mirror.DefaultLayout(alias, path)
 
-    cache.store(MockFetcher(), reference.storage_path)
-    cache.symlink(reference)
+    cache.store(MockFetcher(), layout.path)
+    layout.make_alias(cache.root)
 
-    link_target = resolve_link_target_relative_to_the_link(
-        os.path.join(cache.root, reference.cosmetic_path)
-    )
+    link_target = resolve_link_target_relative_to_the_link(os.path.join(cache.root, layout.alias))
     assert os.path.exists(link_target)
-    assert os.path.normpath(link_target) == os.path.join(cache.root, reference.storage_path)
+    assert os.path.normpath(link_target) == os.path.join(cache.root, layout.path)
 
 
 @pytest.mark.regression("31627")