mirrors: distinguish between source/binary mirror; simplify schema (#34523)

Allow the following formats:

```yaml
mirrors:
  name: <url>
```

```yaml
mirrors:
  name:
    url: s3://xyz
    access_pair: [x, y]
```

```yaml
mirrors:
  name:
    fetch: http://xyz
    push:
      url: s3://xyz
      access_pair: [x, y]
```

And reserve two new properties to indicate the mirror type (e.g.
mirror.spack.io is a source mirror, not a binary cache)

```yaml
mirrors:
  spack-public:
    source: true
    binary: false
    url: https://mirror.spack.io
```

etc/spack/defaults/mirrors.yaml

@@ -1,2 +1,4 @@
 mirrors:
-  spack-public: https://mirror.spack.io
+  spack-public:
+    binary: false
+    url: https://mirror.spack.io

lib/spack/spack/binary_distribution.py

@@ -317,9 +317,9 @@ def update(self, with_cooldown=False):
         from each configured mirror and stored locally (both in memory and
         on disk under ``_index_cache_root``)."""
         self._init_local_index_cache()
-
+        configured_mirror_urls = [
-        mirrors = spack.mirror.MirrorCollection()
+            m.fetch_url for m in spack.mirror.MirrorCollection(binary=True).values()
-        configured_mirror_urls = [m.fetch_url for m in mirrors.values()]
+        ]
         items_to_remove = []
         spec_cache_clear_needed = False
         spec_cache_regenerate_needed = not self._mirrors_for_spec
@@ -1465,8 +1465,9 @@ def download_tarball(spec, unsigned=False, mirrors_for_spec=None):
            "signature_verified": "true-if-binary-pkg-was-already-verified"
        }
     """
-    if not spack.mirror.MirrorCollection():
+    configured_mirrors = spack.mirror.MirrorCollection(binary=True).values()
-        tty.die("Please add a spack mirror to allow " + "download of pre-compiled packages.")
+    if not configured_mirrors:
+        tty.die("Please add a spack mirror to allow download of pre-compiled packages.")
 
     tarball = tarball_path_name(spec, ".spack")
     specfile_prefix = tarball_name(spec, ".spec")
@@ -1483,11 +1484,7 @@ def download_tarball(spec, unsigned=False, mirrors_for_spec=None):
     # we need was in an un-indexed mirror.  No need to check any
     # mirror for the spec twice though.
     try_first = [i["mirror_url"] for i in mirrors_for_spec] if mirrors_for_spec else []
-    try_next = [
+    try_next = [i.fetch_url for i in configured_mirrors if i.fetch_url not in try_first]
-        i.fetch_url
-        for i in spack.mirror.MirrorCollection().values()
-        if i.fetch_url not in try_first
-    ]
 
     for url in try_first + try_next:
         mirrors_to_try.append(
@@ -1980,7 +1977,9 @@ def try_direct_fetch(spec, mirrors=None):
     specfile_is_signed = False
     found_specs = []
 
-    for mirror in spack.mirror.MirrorCollection(mirrors=mirrors).values():
+    binary_mirrors = spack.mirror.MirrorCollection(mirrors=mirrors, binary=True).values()
+
+    for mirror in binary_mirrors:
         buildcache_fetch_url_json = url_util.join(
             mirror.fetch_url, _build_cache_relative_path, specfile_name
         )
@@ -2043,7 +2042,7 @@ def get_mirrors_for_spec(spec=None, mirrors_to_check=None, index_only=False):
     if spec is None:
         return []
 
-    if not spack.mirror.MirrorCollection(mirrors=mirrors_to_check):
+    if not spack.mirror.MirrorCollection(mirrors=mirrors_to_check, binary=True):
         tty.debug("No Spack mirrors are currently configured")
         return {}
 
@@ -2082,7 +2081,7 @@ def clear_spec_cache():
 
 def get_keys(install=False, trust=False, force=False, mirrors=None):
     """Get pgp public keys available on mirror with suffix .pub"""
-    mirror_collection = mirrors or spack.mirror.MirrorCollection()
+    mirror_collection = mirrors or spack.mirror.MirrorCollection(binary=True)
 
     if not mirror_collection:
         tty.die("Please add a spack mirror to allow " + "download of build caches.")
@@ -2243,7 +2242,7 @@ def check_specs_against_mirrors(mirrors, specs, output_file=None):
 
     """
     rebuilds = {}
-    for mirror in spack.mirror.MirrorCollection(mirrors).values():
+    for mirror in spack.mirror.MirrorCollection(mirrors, binary=True).values():
         tty.debug("Checking for built specs at {0}".format(mirror.fetch_url))
 
         rebuild_list = []
@@ -2287,7 +2286,7 @@ def _download_buildcache_entry(mirror_root, descriptions):
 
 
 def download_buildcache_entry(file_descriptions, mirror_url=None):
-    if not mirror_url and not spack.mirror.MirrorCollection():
+    if not mirror_url and not spack.mirror.MirrorCollection(binary=True):
         tty.die(
             "Please provide or add a spack mirror to allow " + "download of buildcache entries."
         )
@@ -2296,7 +2295,7 @@ def download_buildcache_entry(file_descriptions, mirror_url=None):
         mirror_root = os.path.join(mirror_url, _build_cache_relative_path)
         return _download_buildcache_entry(mirror_root, file_descriptions)
 
-    for mirror in spack.mirror.MirrorCollection().values():
+    for mirror in spack.mirror.MirrorCollection(binary=True).values():
         mirror_root = os.path.join(mirror.fetch_url, _build_cache_relative_path)
 
         if _download_buildcache_entry(mirror_root, file_descriptions):

lib/spack/spack/ci.py

@@ -224,7 +224,7 @@ def _print_staging_summary(spec_labels, stages, mirrors_to_check, rebuild_decisi
     if not stages:
         return
 
-    mirrors = spack.mirror.MirrorCollection(mirrors=mirrors_to_check)
+    mirrors = spack.mirror.MirrorCollection(mirrors=mirrors_to_check, binary=True)
     tty.msg("Checked the following mirrors for binaries:")
     for m in mirrors.values():
         tty.msg("  {0}".format(m.fetch_url))

lib/spack/spack/cmd/gpg.py

@@ -216,7 +216,7 @@ def gpg_publish(args):
         url = spack.util.url.path_to_file_url(args.directory)
         mirror = spack.mirror.Mirror(url, url)
     elif args.mirror_name:
-        mirror = spack.mirror.MirrorCollection().lookup(args.mirror_name)
+        mirror = spack.mirror.MirrorCollection(binary=True).lookup(args.mirror_name)
     elif args.mirror_url:
         mirror = spack.mirror.Mirror(args.mirror_url, args.mirror_url)
 

lib/spack/spack/cmd/mirror.py

@@ -21,7 +21,6 @@
 import spack.util.path
 import spack.util.web as web_util
 from spack.error import SpackError
-from spack.util.spack_yaml import syaml_dict
 
 description = "manage mirrors (source and binary)"
 section = "config"
@@ -104,6 +103,15 @@ def setup_parser(subparser):
         default=spack.config.default_modify_scope(),
         help="configuration scope to modify",
     )
+    add_parser.add_argument(
+        "--type",
+        action="append",
+        choices=("binary", "source"),
+        help=(
+            "specify the mirror type: for both binary "
+            "and source use `--type binary --type source` (default)"
+        ),
+    )
     arguments.add_s3_connection_args(add_parser, False)
     # Remove
     remove_parser = sp.add_parser("remove", aliases=["rm"], help=mirror_remove.__doc__)
@@ -120,8 +128,12 @@ def setup_parser(subparser):
     set_url_parser = sp.add_parser("set-url", help=mirror_set_url.__doc__)
     set_url_parser.add_argument("name", help="mnemonic name for mirror", metavar="mirror")
     set_url_parser.add_argument("url", help="url of mirror directory from 'spack mirror create'")
-    set_url_parser.add_argument(
+    set_url_push_or_fetch = set_url_parser.add_mutually_exclusive_group(required=False)
-        "--push", action="store_true", help="set only the URL used for uploading new packages"
+    set_url_push_or_fetch.add_argument(
+        "--push", action="store_true", help="set only the URL used for uploading"
+    )
+    set_url_push_or_fetch.add_argument(
+        "--fetch", action="store_true", help="set only the URL used for downloading"
     )
     set_url_parser.add_argument(
         "--scope",
@@ -132,6 +144,35 @@ def setup_parser(subparser):
     )
     arguments.add_s3_connection_args(set_url_parser, False)
 
+    # Set
+    set_parser = sp.add_parser("set", help=mirror_set.__doc__)
+    set_parser.add_argument("name", help="mnemonic name for mirror", metavar="mirror")
+    set_parser_push_or_fetch = set_parser.add_mutually_exclusive_group(required=False)
+    set_parser_push_or_fetch.add_argument(
+        "--push", action="store_true", help="modify just the push connection details"
+    )
+    set_parser_push_or_fetch.add_argument(
+        "--fetch", action="store_true", help="modify just the fetch connection details"
+    )
+    set_parser.add_argument(
+        "--type",
+        action="append",
+        choices=("binary", "source"),
+        help=(
+            "specify the mirror type: for both binary "
+            "and source use `--type binary --type source`"
+        ),
+    )
+    set_parser.add_argument("--url", help="url of mirror directory from 'spack mirror create'")
+    set_parser.add_argument(
+        "--scope",
+        choices=scopes,
+        metavar=scopes_metavar,
+        default=spack.config.default_modify_scope(),
+        help="configuration scope to modify",
+    )
+    arguments.add_s3_connection_args(set_parser, False)
+
     # List
     list_parser = sp.add_parser("list", help=mirror_list.__doc__)
     list_parser.add_argument(
@@ -151,17 +192,21 @@ def mirror_add(args):
         or args.s3_access_token
         or args.s3_profile
         or args.s3_endpoint_url
+        or args.type
     ):
         connection = {"url": args.url}
         if args.s3_access_key_id and args.s3_access_key_secret:
-            connection["access_pair"] = (args.s3_access_key_id, args.s3_access_key_secret)
+            connection["access_pair"] = [args.s3_access_key_id, args.s3_access_key_secret]
         if args.s3_access_token:
             connection["access_token"] = args.s3_access_token
         if args.s3_profile:
             connection["profile"] = args.s3_profile
         if args.s3_endpoint_url:
             connection["endpoint_url"] = args.s3_endpoint_url
-        mirror = spack.mirror.Mirror(fetch_url=connection, push_url=connection, name=args.name)
+        if args.type:
+            connection["binary"] = "binary" in args.type
+            connection["source"] = "source" in args.type
+        mirror = spack.mirror.Mirror(connection, name=args.name)
     else:
         mirror = spack.mirror.Mirror(args.url, name=args.name)
     spack.mirror.add(mirror, args.scope)
@@ -172,75 +217,51 @@ def mirror_remove(args):
     spack.mirror.remove(args.name, args.scope)
 
 
-def mirror_set_url(args):
+def _configure_mirror(args):
-    """change the URL of a mirror"""
-    url = args.url
     mirrors = spack.config.get("mirrors", scope=args.scope)
-    if not mirrors:
-        mirrors = syaml_dict()
 
     if args.name not in mirrors:
-        tty.die("No mirror found with name %s." % args.name)
+        tty.die(f"No mirror found with name {args.name}.")
 
-    entry = mirrors[args.name]
+    entry = spack.mirror.Mirror(mirrors[args.name], args.name)
-    key_values = ["s3_access_key_id", "s3_access_token", "s3_profile"]
+    direction = "fetch" if args.fetch else "push" if args.push else None
+    changes = {}
+    if args.url:
+        changes["url"] = args.url
+    if args.s3_access_key_id and args.s3_access_key_secret:
+        changes["access_pair"] = [args.s3_access_key_id, args.s3_access_key_secret]
+    if args.s3_access_token:
+        changes["access_token"] = args.s3_access_token
+    if args.s3_profile:
+        changes["profile"] = args.s3_profile
+    if args.s3_endpoint_url:
+        changes["endpoint_url"] = args.s3_endpoint_url
 
-    if any(value for value in key_values if value in args):
+    # argparse cannot distinguish between --binary and --no-binary when same dest :(
-        incoming_data = {
+    # notice that set-url does not have these args, so getattr
-            "url": url,
+    if getattr(args, "type", None):
-            "access_pair": (args.s3_access_key_id, args.s3_access_key_secret),
+        changes["binary"] = "binary" in args.type
-            "access_token": args.s3_access_token,
+        changes["source"] = "source" in args.type
-            "profile": args.s3_profile,
-            "endpoint_url": args.s3_endpoint_url,
-        }
-    try:
-        fetch_url = entry["fetch"]
-        push_url = entry["push"]
-    except TypeError:
-        fetch_url, push_url = entry, entry
 
-    changes_made = False
+    changed = entry.update(changes, direction)
 
-    if args.push:
+    if changed:
-        if isinstance(push_url, dict):
+        mirrors[args.name] = entry.to_dict()
-            changes_made = changes_made or push_url != incoming_data
+        spack.config.set("mirrors", mirrors, scope=args.scope)
-            push_url = incoming_data
-        else:
-            changes_made = changes_made or push_url != url
-            push_url = url
-    else:
-        if isinstance(push_url, dict):
-            changes_made = changes_made or push_url != incoming_data or push_url != incoming_data
-            fetch_url, push_url = incoming_data, incoming_data
-        else:
-            changes_made = changes_made or push_url != url
-            fetch_url, push_url = url, url
-
-    items = [
-        (
-            (n, u)
-            if n != args.name
-            else (
-                (n, {"fetch": fetch_url, "push": push_url})
-                if fetch_url != push_url
-                else (n, {"fetch": fetch_url, "push": fetch_url})
-            )
-        )
-        for n, u in mirrors.items()
-    ]
-
-    mirrors = syaml_dict(items)
-    spack.config.set("mirrors", mirrors, scope=args.scope)
-
-    if changes_made:
-        tty.msg(
-            "Changed%s url or connection information for mirror %s."
-            % ((" (push)" if args.push else ""), args.name)
-        )
     else:
         tty.msg("No changes made to mirror %s." % args.name)
 
 
+def mirror_set(args):
+    """Configure the connection details of a mirror"""
+    _configure_mirror(args)
+
+
+def mirror_set_url(args):
+    """Change the URL of a mirror."""
+    _configure_mirror(args)
+
+
 def mirror_list(args):
     """print out available mirrors to the console"""
 
@@ -488,6 +509,7 @@ def mirror(parser, args):
         "remove": mirror_remove,
         "rm": mirror_remove,
         "set-url": mirror_set_url,
+        "set": mirror_set,
         "list": mirror_list,
     }
 

lib/spack/spack/installer.py

@@ -505,8 +505,8 @@ def _try_install_from_binary_cache(
             otherwise, ``False``
         timer: timer to keep track of binary install phases.
     """
-    # Early exit if no mirrors are configured.
+    # Early exit if no binary mirrors are configured.
-    if not spack.mirror.MirrorCollection():
+    if not spack.mirror.MirrorCollection(binary=True):
         return False
 
     tty.debug("Searching for binary cache of {0}".format(package_id(pkg)))

lib/spack/spack/mirror.py

@@ -18,6 +18,7 @@
 import sys
 import traceback
 import urllib.parse
+from typing import Optional, Union
 
 import llnl.util.tty as tty
 from llnl.util.filesystem import mkdirp
@@ -40,15 +41,6 @@
 supported_url_schemes = ("file", "http", "https", "sftp", "ftp", "s3", "gs")
 
 
-def _display_mirror_entry(size, name, url, type_=None):
-    if type_:
-        type_ = "".join((" (", type_, ")"))
-    else:
-        type_ = ""
-
-    print("%-*s%s%s" % (size + 4, name, url, type_))
-
-
 def _url_or_path_to_url(url_or_path: str) -> str:
     """For simplicity we allow mirror URLs in config files to be local, relative paths.
     This helper function takes care of distinguishing between URLs and paths, and
@@ -71,36 +63,24 @@ class Mirror:
     to them. These two URLs are usually the same.
     """
 
-    def __init__(self, fetch_url, push_url=None, name=None):
+    def __init__(self, data: Union[str, dict], name: Optional[str] = None):
-        self._fetch_url = fetch_url
+        self._data = data
-        self._push_url = push_url
         self._name = name
 
-    def __eq__(self, other):
-        return self._fetch_url == other._fetch_url and self._push_url == other._push_url
-
-    def to_json(self, stream=None):
-        return sjson.dump(self.to_dict(), stream)
-
-    def to_yaml(self, stream=None):
-        return syaml.dump(self.to_dict(), stream)
-
     @staticmethod
     def from_yaml(stream, name=None):
-        data = syaml.load(stream)
+        return Mirror(syaml.load(stream), name)
-        return Mirror.from_dict(data, name)
 
     @staticmethod
     def from_json(stream, name=None):
         try:
-            d = sjson.load(stream)
+            return Mirror(sjson.load(stream), name)
-            return Mirror.from_dict(d, name)
         except Exception as e:
             raise sjson.SpackJSONError("error parsing JSON mirror:", str(e)) from e
 
     @staticmethod
     def from_local_path(path: str):
-        return Mirror(fetch_url=url_util.path_to_file_url(path))
+        return Mirror(url_util.path_to_file_url(path))
 
     @staticmethod
     def from_url(url: str):
@@ -111,165 +91,220 @@ def from_url(url: str):
                     url, ", ".join(supported_url_schemes)
                 )
             )
-        return Mirror(fetch_url=url)
+        return Mirror(url)
 
-    def to_dict(self):
+    def __eq__(self, other):
-        # Keep it a key-value pair <name>: <url> when possible.
+        if not isinstance(other, Mirror):
-        if isinstance(self._fetch_url, str) and self._push_url is None:
+            return NotImplemented
-            return self._fetch_url
+        return self._data == other._data and self._name == other._name
-
-        if self._push_url is None:
-            return syaml_dict([("fetch", self._fetch_url), ("push", self._fetch_url)])
-        else:
-            return syaml_dict([("fetch", self._fetch_url), ("push", self._push_url)])
-
-    @staticmethod
-    def from_dict(d, name=None):
-        if isinstance(d, str):
-            return Mirror(d, name=name)
-        else:
-            return Mirror(d["fetch"], d["push"], name=name)
-
-    def display(self, max_len=0):
-        if self._push_url is None:
-            _display_mirror_entry(max_len, self._name, self.fetch_url)
-        else:
-            _display_mirror_entry(max_len, self._name, self.fetch_url, "fetch")
-            _display_mirror_entry(max_len, self._name, self.push_url, "push")
 
     def __str__(self):
-        name = self._name
+        return f"{self._name}: {self.push_url} {self.fetch_url}"
-        if name is None:
-            name = ""
-        else:
-            name = ' "%s"' % name
-
-        if self._push_url is None:
-            return "[Mirror%s (%s)]" % (name, self._fetch_url)
-
-        return "[Mirror%s (fetch: %s, push: %s)]" % (name, self._fetch_url, self._push_url)
 
     def __repr__(self):
-        return "".join(
+        return f"Mirror(name={self._name!r}, data={self._data!r})"
-            (
+
-                "Mirror(",
+    def to_json(self, stream=None):
-                ", ".join(
+        return sjson.dump(self.to_dict(), stream)
-                    "%s=%s" % (k, repr(v))
+
-                    for k, v in (
+    def to_yaml(self, stream=None):
-                        ("fetch_url", self._fetch_url),
+        return syaml.dump(self.to_dict(), stream)
-                        ("push_url", self._push_url),
+
-                        ("name", self._name),
+    def to_dict(self):
-                    )
+        return self._data
-                    if k == "fetch_url" or v
+
-                ),
+    def display(self, max_len=0):
-                ")",
+        fetch, push = self.fetch_url, self.push_url
-            )
+        # don't print the same URL twice
-        )
+        url = fetch if fetch == push else f"fetch: {fetch} push: {push}"
+        source = "s" if self.source else " "
+        binary = "b" if self.binary else " "
+        print(f"{self.name: <{max_len}} [{source}{binary}] {url}")
 
     @property
     def name(self):
         return self._name or "<unnamed>"
 
-    def get_profile(self, url_type):
+    @property
-        if isinstance(self._fetch_url, dict):
+    def binary(self):
-            if url_type == "push":
+        return isinstance(self._data, str) or self._data.get("binary", True)
-                return self._push_url.get("profile", None)
-            return self._fetch_url.get("profile", None)
-        else:
-            return None
 
-    def set_profile(self, url_type, profile):
+    @property
-        if url_type == "push":
+    def source(self):
-            self._push_url["profile"] = profile
+        return isinstance(self._data, str) or self._data.get("source", True)
-        else:
-            self._fetch_url["profile"] = profile
-
-    def get_access_pair(self, url_type):
-        if isinstance(self._fetch_url, dict):
-            if url_type == "push":
-                return self._push_url.get("access_pair", None)
-            return self._fetch_url.get("access_pair", None)
-        else:
-            return None
-
-    def set_access_pair(self, url_type, connection_tuple):
-        if url_type == "push":
-            self._push_url["access_pair"] = connection_tuple
-        else:
-            self._fetch_url["access_pair"] = connection_tuple
-
-    def get_endpoint_url(self, url_type):
-        if isinstance(self._fetch_url, dict):
-            if url_type == "push":
-                return self._push_url.get("endpoint_url", None)
-            return self._fetch_url.get("endpoint_url", None)
-        else:
-            return None
-
-    def set_endpoint_url(self, url_type, url):
-        if url_type == "push":
-            self._push_url["endpoint_url"] = url
-        else:
-            self._fetch_url["endpoint_url"] = url
-
-    def get_access_token(self, url_type):
-        if isinstance(self._fetch_url, dict):
-            if url_type == "push":
-                return self._push_url.get("access_token", None)
-            return self._fetch_url.get("access_token", None)
-        else:
-            return None
-
-    def set_access_token(self, url_type, connection_token):
-        if url_type == "push":
-            self._push_url["access_token"] = connection_token
-        else:
-            self._fetch_url["access_token"] = connection_token
 
     @property
     def fetch_url(self):
         """Get the valid, canonicalized fetch URL"""
-        url_or_path = (
+        return self.get_url("fetch")
-            self._fetch_url if isinstance(self._fetch_url, str) else self._fetch_url["url"]
-        )
-        return _url_or_path_to_url(url_or_path)
-
-    @fetch_url.setter
-    def fetch_url(self, url):
-        self._fetch_url["url"] = url
-        self._normalize()
 
     @property
     def push_url(self):
-        """Get the valid, canonicalized push URL. Returns fetch URL if no custom
+        """Get the valid, canonicalized fetch URL"""
-        push URL is defined"""
+        return self.get_url("push")
-        if self._push_url is None:
-            return self.fetch_url
-        url_or_path = self._push_url if isinstance(self._push_url, str) else self._push_url["url"]
-        return _url_or_path_to_url(url_or_path)
 
-    @push_url.setter
+    def _update_connection_dict(self, current_data: dict, new_data: dict, top_level: bool):
-    def push_url(self, url):
+        keys = ["url", "access_pair", "access_token", "profile", "endpoint_url"]
-        self._push_url["url"] = url
+        if top_level:
-        self._normalize()
+            keys += ["binary", "source"]
+        changed = False
+        for key in keys:
+            if key in new_data and current_data.get(key) != new_data[key]:
+                current_data[key] = new_data[key]
+                changed = True
+        return changed
 
-    def _normalize(self):
+    def update(self, data: dict, direction: Optional[str] = None) -> bool:
-        if self._push_url is not None and self._push_url == self._fetch_url:
+        """Modify the mirror with the given data. This takes care
-            self._push_url = None
+        of expanding trivial mirror definitions by URL to something more
+        rich with a dict if necessary
+
+        Args:
+            data (dict): The data to update the mirror with.
+            direction (str): The direction to update the mirror in (fetch
+                or push or None for top-level update)
+
+        Returns:
+            bool: True if the mirror was updated, False otherwise."""
+
+        # Modify the top-level entry when no direction is given.
+        if not data:
+            return False
+
+        # If we only update a URL, there's typically no need to expand things to a dict.
+        set_url = data["url"] if len(data) == 1 and "url" in data else None
+
+        if direction is None:
+            # First deal with the case where the current top-level entry is just a string.
+            if isinstance(self._data, str):
+                # Can we replace that string with something new?
+                if set_url:
+                    if self._data == set_url:
+                        return False
+                    self._data = set_url
+                    return True
+
+                # Otherwise promote to a dict
+                self._data = {"url": self._data}
+
+            # And update the dictionary accordingly.
+            return self._update_connection_dict(self._data, data, top_level=True)
+
+        # Otherwise, update the fetch / push entry; turn top-level
+        # url string into a dict if necessary.
+        if isinstance(self._data, str):
+            self._data = {"url": self._data}
+
+        # Create a new fetch / push entry if necessary
+        if direction not in self._data:
+            # Keep config minimal if we're just setting the URL.
+            if set_url:
+                self._data[direction] = set_url
+                return True
+            self._data[direction] = {}
+
+        entry = self._data[direction]
+
+        # Keep the entry simple if we're just swapping out the URL.
+        if isinstance(entry, str):
+            if set_url:
+                if entry == set_url:
+                    return False
+                self._data[direction] = set_url
+                return True
+
+            # Otherwise promote to a dict
+            self._data[direction] = {"url": entry}
+
+        return self._update_connection_dict(self._data[direction], data, top_level=False)
+
+    def _get_value(self, attribute: str, direction: str):
+        """Returns the most specific value for a given attribute (either push/fetch or global)"""
+        if direction not in ("fetch", "push"):
+            raise ValueError(f"direction must be either 'fetch' or 'push', not {direction}")
+
+        if isinstance(self._data, str):
+            return None
+
+        # Either a string (url) or a dictionary, we care about the dict here.
+        value = self._data.get(direction, {})
+
+        # Return top-level entry if only a URL was set.
+        if isinstance(value, str):
+            return self._data.get(attribute, None)
+
+        return self._data.get(direction, {}).get(attribute, None)
+
+    def get_url(self, direction: str):
+        if direction not in ("fetch", "push"):
+            raise ValueError(f"direction must be either 'fetch' or 'push', not {direction}")
+
+        # Whole mirror config is just a url.
+        if isinstance(self._data, str):
+            return _url_or_path_to_url(self._data)
+
+        # Default value
+        url = self._data.get("url")
+
+        # Override it with a direction-specific value
+        if direction in self._data:
+            # Either a url as string or a dict with url key
+            info = self._data[direction]
+            if isinstance(info, str):
+                url = info
+            elif "url" in info:
+                url = info["url"]
+
+        return _url_or_path_to_url(url) if url else None
+
+    def get_access_token(self, direction: str):
+        return self._get_value("access_token", direction)
+
+    def get_access_pair(self, direction: str):
+        return self._get_value("access_pair", direction)
+
+    def get_profile(self, direction: str):
+        return self._get_value("profile", direction)
+
+    def get_endpoint_url(self, direction: str):
+        return self._get_value("endpoint_url", direction)
 
 
 class MirrorCollection(collections.abc.Mapping):
     """A mapping of mirror names to mirrors."""
 
-    def __init__(self, mirrors=None, scope=None):
+    def __init__(
-        self._mirrors = collections.OrderedDict(
+        self,
-            (name, Mirror.from_dict(mirror, name))
+        mirrors=None,
+        scope=None,
+        binary: Optional[bool] = None,
+        source: Optional[bool] = None,
+    ):
+        """Initialize a mirror collection.
+
+        Args:
+            mirrors: A name-to-mirror mapping to initialize the collection with.
+            scope: The scope to use when looking up mirrors from the config.
+            binary: If True, only include binary mirrors.
+                    If False, omit binary mirrors.
+                    If None, do not filter on binary mirrors.
+            source: If True, only include source mirrors.
+                    If False, omit source mirrors.
+                    If None, do not filter on source mirrors."""
+        self._mirrors = {
+            name: Mirror(data=mirror, name=name)
             for name, mirror in (
                 mirrors.items()
                 if mirrors is not None
                 else spack.config.get("mirrors", scope=scope).items()
             )
-        )
+        }
+
+        if source is not None:
+            self._mirrors = {k: v for k, v in self._mirrors.items() if v.source == source}
+
+        if binary is not None:
+            self._mirrors = {k: v for k, v in self._mirrors.items() if v.binary == binary}
 
     def __eq__(self, other):
         return self._mirrors == other._mirrors
@@ -325,7 +360,7 @@ def lookup(self, name_or_url):
         result = self.get(name_or_url)
 
         if result is None:
-            result = Mirror(fetch_url=name_or_url)
+            result = Mirror(fetch=name_or_url)
 
         return result
 
@@ -576,24 +611,8 @@ def remove(name, scope):
     if name not in mirrors:
         tty.die("No mirror with name %s" % name)
 
-    old_value = mirrors.pop(name)
+    mirrors.pop(name)
     spack.config.set("mirrors", mirrors, scope=scope)
-
-    debug_msg_url = "url %s"
-    debug_msg = ["Removed mirror %s with"]
-    values = [name]
-
-    try:
-        fetch_value = old_value["fetch"]
-        push_value = old_value["push"]
-
-        debug_msg.extend(("fetch", debug_msg_url, "and push", debug_msg_url))
-        values.extend((fetch_value, push_value))
-    except TypeError:
-        debug_msg.append(debug_msg_url)
-        values.append(old_value)
-
-    tty.debug(" ".join(debug_msg) % tuple(values))
     tty.msg("Removed mirror %s." % name)
 
 

lib/spack/spack/schema/mirrors.py

@@ -6,29 +6,55 @@
 """Schema for mirrors.yaml configuration file.
 
 .. literalinclude:: _spack_root/lib/spack/spack/schema/mirrors.py
+   :lines: 12-69
 """
 
+#: Common properties for connection specification
+connection = {
+    "url": {"type": "string"},
+    # todo: replace this with named keys "username" / "password" or "id" / "secret"
+    "access_pair": {
+        "type": "array",
+        "items": {"type": ["string", "null"], "minItems": 2, "maxItems": 2},
+    },
+    "access_token": {"type": ["string", "null"]},
+    "profile": {"type": ["string", "null"]},
+    "endpoint_url": {"type": ["string", "null"]},
+}
+
+#: Mirror connection inside pull/push keys
+fetch_and_push = {
+    "anyOf": [
+        {"type": "string"},
+        {
+            "type": "object",
+            "additionalProperties": False,
+            "properties": {**connection},  # type: ignore
+        },
+    ]
+}
+
+#: Mirror connection when no pull/push keys are set
+mirror_entry = {
+    "type": "object",
+    "additionalProperties": False,
+    "anyOf": [{"required": ["url"]}, {"required": ["fetch"]}, {"required": ["pull"]}],
+    "properties": {
+        "source": {"type": "boolean"},
+        "binary": {"type": "boolean"},
+        "fetch": fetch_and_push,
+        "push": fetch_and_push,
+        **connection,  # type: ignore
+    },
+}
+
 #: Properties for inclusion in other schemas
 properties = {
     "mirrors": {
         "type": "object",
         "default": {},
         "additionalProperties": False,
-        "patternProperties": {
+        "patternProperties": {r"\w[\w-]*": {"anyOf": [{"type": "string"}, mirror_entry]}},
-            r"\w[\w-]*": {
-                "anyOf": [
-                    {"type": "string"},
-                    {
-                        "type": "object",
-                        "required": ["fetch", "push"],
-                        "properties": {
-                            "fetch": {"type": ["string", "object"]},
-                            "push": {"type": ["string", "object"]},
-                        },
-                    },
-                ]
-            }
-        },
     }
 }
 

lib/spack/spack/stage.py

@@ -449,22 +449,11 @@ def fetch(self, mirror_only=False, err_msg=None):
             # Join URLs of mirror roots with mirror paths. Because
             # urljoin() will strip everything past the final '/' in
             # the root, so we add a '/' if it is not present.
-            mirror_urls = {}
+            mirror_urls = [
-            for mirror in spack.mirror.MirrorCollection().values():
+                url_util.join(mirror.fetch_url, rel_path)
-                for rel_path in self.mirror_paths:
+                for mirror in spack.mirror.MirrorCollection(source=True).values()
-                    mirror_url = url_util.join(mirror.fetch_url, rel_path)
+                for rel_path in self.mirror_paths
-                    mirror_urls[mirror_url] = {}
+            ]
-                    if (
-                        mirror.get_access_pair("fetch")
-                        or mirror.get_access_token("fetch")
-                        or mirror.get_profile("fetch")
-                    ):
-                        mirror_urls[mirror_url] = {
-                            "access_token": mirror.get_access_token("fetch"),
-                            "access_pair": mirror.get_access_pair("fetch"),
-                            "access_profile": mirror.get_profile("fetch"),
-                            "endpoint_url": mirror.get_endpoint_url("fetch"),
-                        }
 
             # If this archive is normally fetched from a tarball URL,
             # then use the same digest.  `spack mirror` ensures that
@@ -483,16 +472,9 @@ def fetch(self, mirror_only=False, err_msg=None):
 
             # Add URL strategies for all the mirrors with the digest
             # Insert fetchers in the order that the URLs are provided.
-            for url in reversed(list(mirror_urls.keys())):
+            for url in reversed(mirror_urls):
                 fetchers.insert(
-                    0,
+                    0, fs.from_url_scheme(url, digest, expand=expand, extension=extension)
-                    fs.from_url_scheme(
-                        url,
-                        digest,
-                        expand=expand,
-                        extension=extension,
-                        connection=mirror_urls[url],
-                    ),
                 )
 
             if self.default_fetcher.cachable:

lib/spack/spack/test/cmd/mirror.py

@@ -149,7 +149,7 @@ def test_mirror_crud(mutable_config, capsys):
         assert "No changes made" in output
 
         output = mirror("set-url", "--push", "mirror", "s3://spack-public")
-        assert "Changed (push) url" in output
+        assert not output
 
         # no-op
         output = mirror("set-url", "--push", "mirror", "s3://spack-public")
@@ -348,3 +348,54 @@ def test_versions_per_spec_produces_concrete_specs(self, input_specs, nversions,
         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)
+
+
+def test_mirror_type(mutable_config):
+    """Test the mirror set command"""
+    mirror("add", "example", "--type", "binary", "http://example.com")
+    assert spack.config.get("mirrors:example") == {
+        "url": "http://example.com",
+        "source": False,
+        "binary": True,
+    }
+
+    mirror("set", "example", "--type", "source")
+    assert spack.config.get("mirrors:example") == {
+        "url": "http://example.com",
+        "source": True,
+        "binary": False,
+    }
+
+    mirror("set", "example", "--type", "binary")
+    assert spack.config.get("mirrors:example") == {
+        "url": "http://example.com",
+        "source": False,
+        "binary": True,
+    }
+    mirror("set", "example", "--type", "binary", "--type", "source")
+    assert spack.config.get("mirrors:example") == {
+        "url": "http://example.com",
+        "source": True,
+        "binary": True,
+    }
+
+
+def test_mirror_set_2(mutable_config):
+    """Test the mirror set command"""
+    mirror("add", "example", "http://example.com")
+    mirror(
+        "set",
+        "example",
+        "--push",
+        "--url",
+        "http://example2.com",
+        "--s3-access-key-id",
+        "username",
+        "--s3-access-key-secret",
+        "password",
+    )
+
+    assert spack.config.get("mirrors:example") == {
+        "url": "http://example.com",
+        "push": {"url": "http://example2.com", "access_pair": ["username", "password"]},
+    }

lib/spack/spack/test/mirror.py

@@ -132,9 +132,14 @@ def test_all_mirror(mock_git_repository, mock_svn_repository, mock_hg_repository
 
 
 @pytest.mark.parametrize(
-    "mirror", [spack.mirror.Mirror("https://example.com/fetch", "https://example.com/push")]
+    "mirror",
+    [
+        spack.mirror.Mirror(
+            {"fetch": "https://example.com/fetch", "push": "https://example.com/push"}
+        )
+    ],
 )
-def test_roundtrip_mirror(mirror):
+def test_roundtrip_mirror(mirror: spack.mirror.Mirror):
     mirror_yaml = mirror.to_yaml()
     assert spack.mirror.Mirror.from_yaml(mirror_yaml) == mirror
     mirror_json = mirror.to_json()
@@ -291,3 +296,70 @@ 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)
+
+
+def test_update_1():
+    # No change
+    m = spack.mirror.Mirror("https://example.com")
+    assert not m.update({"url": "https://example.com"})
+    assert m.to_dict() == "https://example.com"
+
+
+def test_update_2():
+    # Change URL, shouldn't expand to {"url": ...} dict.
+    m = spack.mirror.Mirror("https://example.com")
+    assert m.update({"url": "https://example.org"})
+    assert m.to_dict() == "https://example.org"
+    assert m.fetch_url == "https://example.org"
+    assert m.push_url == "https://example.org"
+
+
+def test_update_3():
+    # Change fetch url, ensure minimal config
+    m = spack.mirror.Mirror("https://example.com")
+    assert m.update({"url": "https://example.org"}, "fetch")
+    assert m.to_dict() == {"url": "https://example.com", "fetch": "https://example.org"}
+    assert m.fetch_url == "https://example.org"
+    assert m.push_url == "https://example.com"
+
+
+def test_update_4():
+    # Change push url, ensure minimal config
+    m = spack.mirror.Mirror("https://example.com")
+    assert m.update({"url": "https://example.org"}, "push")
+    assert m.to_dict() == {"url": "https://example.com", "push": "https://example.org"}
+    assert m.push_url == "https://example.org"
+    assert m.fetch_url == "https://example.com"
+
+
+@pytest.mark.parametrize("direction", ["fetch", "push"])
+def test_update_connection_params(direction):
+    """Test whether new connection params expand the mirror config to a dict."""
+    m = spack.mirror.Mirror("https://example.com")
+
+    assert m.update(
+        {
+            "url": "http://example.org",
+            "access_pair": ["username", "password"],
+            "access_token": "token",
+            "profile": "profile",
+            "endpoint_url": "https://example.com",
+        },
+        direction,
+    )
+
+    assert m.to_dict() == {
+        "url": "https://example.com",
+        direction: {
+            "url": "http://example.org",
+            "access_pair": ["username", "password"],
+            "access_token": "token",
+            "profile": "profile",
+            "endpoint_url": "https://example.com",
+        },
+    }
+
+    assert m.get_access_pair(direction) == ["username", "password"]
+    assert m.get_access_token(direction) == "token"
+    assert m.get_profile(direction) == "profile"
+    assert m.get_endpoint_url(direction) == "https://example.com"

lib/spack/spack/test/web.py

@@ -267,7 +267,7 @@ def head_object(self, Bucket=None, Key=None):
 
 
 def test_gather_s3_information(monkeypatch, capfd):
-    mirror = spack.mirror.Mirror.from_dict(
+    mirror = spack.mirror.Mirror(
         {
             "fetch": {
                 "access_token": "AAAAAAA",

share/spack/spack-completion.bash

@@ -1290,7 +1290,7 @@ _spack_mirror() {
     then
         SPACK_COMPREPLY="-h --help -n --no-checksum --deprecated"
     else
-        SPACK_COMPREPLY="create destroy add remove rm set-url list"
+        SPACK_COMPREPLY="create destroy add remove rm set-url set list"
     fi
 }
 
@@ -1310,7 +1310,7 @@ _spack_mirror_destroy() {
 _spack_mirror_add() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help --scope --s3-access-key-id --s3-access-key-secret --s3-access-token --s3-profile --s3-endpoint-url"
+        SPACK_COMPREPLY="-h --help --scope --type --s3-access-key-id --s3-access-key-secret --s3-access-token --s3-profile --s3-endpoint-url"
     else
         _mirrors
     fi
@@ -1337,7 +1337,16 @@ _spack_mirror_rm() {
 _spack_mirror_set_url() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help --push --scope --s3-access-key-id --s3-access-key-secret --s3-access-token --s3-profile --s3-endpoint-url"
+        SPACK_COMPREPLY="-h --help --push --fetch --scope --s3-access-key-id --s3-access-key-secret --s3-access-token --s3-profile --s3-endpoint-url"
+    else
+        _mirrors
+    fi
+}
+
+_spack_mirror_set() {
+    if $list_options
+    then
+        SPACK_COMPREPLY="-h --help --push --fetch --type --url --scope --s3-access-key-id --s3-access-key-secret --s3-access-token --s3-profile --s3-endpoint-url"
     else
         _mirrors
     fi