buildcache sync: manifest-glob with arbitrary destination (#41284)

* buildcache sync: manifest-glob with arbitrary destination

The current implementation of the --manifest-glob is a bit restrictive
requiring the destination to be known by the generation stage of CI.
This allows specifying an arbitrary destination mirror URL.

* Add unit test for buildcache sync with manifest

* Fix test and arguments for manifest-glob with override destination

* Add testing path for unused mirror argument

4 files changed, 103 insertions, 13 deletions

diff --git a/lib/spack/spack/cmd/buildcache.py b/lib/spack/spack/cmd/buildcache.py
index 8a2398b705..ad50cb94a4 100644
--- a/lib/spack/spack/cmd/buildcache.py
+++ b/lib/spack/spack/cmd/buildcache.py
@@ -275,23 +275,37 @@ def setup_parser(subparser: argparse.ArgumentParser):
 
     # Sync buildcache entries from one mirror to another
     sync = subparsers.add_parser("sync", help=sync_fn.__doc__)
-    sync.add_argument(
-        "--manifest-glob", help="a quoted glob pattern identifying copy manifest files"
+
+    sync_manifest_source = sync.add_argument_group(
+        "Manifest Source",
+        "Specify a list of build cache objects to sync using manifest file(s)."
+        'This option takes the place of the "source mirror" for synchronization'
+        'and optionally takes a "destination mirror" ',
     )
-    sync.add_argument(
+    sync_manifest_source.add_argument(
+        "--manifest-glob", help="a quoted glob pattern identifying CI rebuild manifest files"
+    )
+    sync_source_mirror = sync.add_argument_group(
+        "Named Source",
+        "Specify a single registered source mirror to synchronize from. This option requires"
+        "the specification of a destination mirror.",
+    )
+    sync_source_mirror.add_argument(
         "src_mirror",
         metavar="source mirror",
-        type=arguments.mirror_name_or_url,
         nargs="?",
+        type=arguments.mirror_name_or_url,
         help="source mirror name, path, or URL",
     )
+
     sync.add_argument(
         "dest_mirror",
         metavar="destination mirror",
-        type=arguments.mirror_name_or_url,
         nargs="?",
+        type=arguments.mirror_name_or_url,
         help="destination mirror name, path, or URL",
     )
+
     sync.set_defaults(func=sync_fn)
 
     # Update buildcache index without copying any additional packages
@@ -1070,7 +1084,17 @@ def sync_fn(args):
     requires an active environment in order to know which specs to sync
     """
     if args.manifest_glob:
-        manifest_copy(glob.glob(args.manifest_glob))
+        # Passing the args.src_mirror here because it is not possible to
+        # have the destination be required when specifying a named source
+        # mirror and optional for the --manifest-glob argument. In the case
+        # of manifest glob sync, the source mirror positional argument is the
+        # destination mirror if it is specified. If there are two mirrors
+        # specified, the second is ignored and the first is the override
+        # destination.
+        if args.dest_mirror:
+            tty.warn(f"Ignoring unused arguemnt: {args.dest_mirror.name}")
+
+        manifest_copy(glob.glob(args.manifest_glob), args.src_mirror)
         return 0
 
     if args.src_mirror is None or args.dest_mirror is None:
@@ -1121,7 +1145,7 @@ def sync_fn(args):
         shutil.rmtree(tmpdir)
 
 
-def manifest_copy(manifest_file_list):
+def manifest_copy(manifest_file_list, dest_mirror=None):
     """Read manifest files containing information about specific specs to copy
     from source to destination, remove duplicates since any binary packge for
     a given hash should be the same as any other, and copy all files specified
@@ -1135,10 +1159,17 @@ def manifest_copy(manifest_file_list):
                 # Last duplicate hash wins
                 deduped_manifest[spec_hash] = copy_list
 
+    build_cache_dir = bindist.build_cache_relative_path()
     for spec_hash, copy_list in deduped_manifest.items():
         for copy_file in copy_list:
-            tty.debug("copying {0} to {1}".format(copy_file["src"], copy_file["dest"]))
-            copy_buildcache_file(copy_file["src"], copy_file["dest"])
+            dest = copy_file["dest"]
+            if dest_mirror:
+                src_relative_path = os.path.join(
+                    build_cache_dir, copy_file["src"].rsplit(build_cache_dir, 1)[1].lstrip("/")
+                )
+                dest = url_util.join(dest_mirror.push_url, src_relative_path)
+            tty.debug("copying {0} to {1}".format(copy_file["src"], dest))
+            copy_buildcache_file(copy_file["src"], dest)
 
 
 def update_index(mirror: spack.mirror.Mirror, update_keys=False):
diff --git a/lib/spack/spack/test/cmd/buildcache.py b/lib/spack/spack/test/cmd/buildcache.py
index 06d0c1d751..12ef53e856 100644
--- a/lib/spack/spack/test/cmd/buildcache.py
+++ b/lib/spack/spack/test/cmd/buildcache.py
@@ -4,6 +4,7 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 import errno
+import json
 import os
 import shutil
 
@@ -234,10 +235,71 @@ def test_buildcache_sync(
         # Use mirror names to specify mirrors
         mirror("add", "src", src_mirror_url)
         mirror("add", "dest", dest_mirror_url)
+        mirror("add", "ignored", "file:///dummy/io")
 
         buildcache("sync", "src", "dest")
 
         verify_mirror_contents()
+        shutil.rmtree(dest_mirror_dir)
+
+        def manifest_insert(manifest, spec, dest_url):
+            manifest[spec.dag_hash()] = [
+                {
+                    "src": spack.util.url.join(
+                        src_mirror_url,
+                        spack.binary_distribution.build_cache_relative_path(),
+                        spack.binary_distribution.tarball_name(spec, ".spec.json"),
+                    ),
+                    "dest": spack.util.url.join(
+                        dest_url,
+                        spack.binary_distribution.build_cache_relative_path(),
+                        spack.binary_distribution.tarball_name(spec, ".spec.json"),
+                    ),
+                },
+                {
+                    "src": spack.util.url.join(
+                        src_mirror_url,
+                        spack.binary_distribution.build_cache_relative_path(),
+                        spack.binary_distribution.tarball_path_name(spec, ".spack"),
+                    ),
+                    "dest": spack.util.url.join(
+                        dest_url,
+                        spack.binary_distribution.build_cache_relative_path(),
+                        spack.binary_distribution.tarball_path_name(spec, ".spack"),
+                    ),
+                },
+            ]
+
+        manifest_file = os.path.join(tmpdir.strpath, "manifest_dest.json")
+        with open(manifest_file, "w") as fd:
+            test_env = ev.active_environment()
+
+            manifest = {}
+            for spec in test_env.specs_by_hash.values():
+                manifest_insert(manifest, spec, dest_mirror_url)
+            json.dump(manifest, fd)
+
+        buildcache("sync", "--manifest-glob", manifest_file)
+
+        verify_mirror_contents()
+        shutil.rmtree(dest_mirror_dir)
+
+        manifest_file = os.path.join(tmpdir.strpath, "manifest_bad_dest.json")
+        with open(manifest_file, "w") as fd:
+            manifest = {}
+            for spec in test_env.specs_by_hash.values():
+                manifest_insert(
+                    manifest, spec, spack.util.url.join(dest_mirror_url, "invalid_path")
+                )
+            json.dump(manifest, fd)
+
+        # Trigger the warning
+        output = buildcache("sync", "--manifest-glob", manifest_file, "dest", "ignored")
+
+        assert "Ignoring unused arguemnt: ignored" in output
+
+        verify_mirror_contents()
+        shutil.rmtree(dest_mirror_dir)
 
 
 def test_buildcache_create_install(
diff --git a/share/spack/spack-completion.fish b/share/spack/spack-completion.fish
index 8547de9349..47e6fccb31 100755
--- a/share/spack/spack-completion.fish
+++ b/share/spack/spack-completion.fish
@@ -861,7 +861,7 @@ set -g __fish_spack_optspecs_spack_buildcache_sync h/help manifest-glob=
 complete -c spack -n '__fish_spack_using_command buildcache sync' -s h -l help -f -a help
 complete -c spack -n '__fish_spack_using_command buildcache sync' -s h -l help -d 'show this help message and exit'
 complete -c spack -n '__fish_spack_using_command buildcache sync' -l manifest-glob -r -f -a manifest_glob
-complete -c spack -n '__fish_spack_using_command buildcache sync' -l manifest-glob -r -d 'a quoted glob pattern identifying copy manifest files'
+complete -c spack -n '__fish_spack_using_command buildcache sync' -l manifest-glob -r -d 'a quoted glob pattern identifying CI rebuild manifest files'
 
 # spack buildcache update-index
 set -g __fish_spack_optspecs_spack_buildcache_update_index h/help k/keys
diff --git a/var/spack/repos/builtin/packages/icon/package.py b/var/spack/repos/builtin/packages/icon/package.py
index ec5a832935..8d424a7dfb 100644
--- a/var/spack/repos/builtin/packages/icon/package.py
+++ b/var/spack/repos/builtin/packages/icon/package.py
@@ -204,7 +204,6 @@ class Icon(AutotoolsPackage):
             args.append("--disable-gpu")
 
         if self.compiler.name == "gcc":
-
             flags["CFLAGS"].append("-g")
             flags["ICON_CFLAGS"].append("-O3")
             flags["ICON_BUNDLED_CFLAGS"].append("-O2")
@@ -217,7 +216,6 @@ class Icon(AutotoolsPackage):
                 )
 
         elif self.compiler.name in ["intel", "oneapi"]:
-
             args.append("--enable-intel-consistency")
 
             flags["CFLAGS"].extend(["-g", "-ftz", "-fma", "-ip", "-qno-opt-dynamic-align"])
@@ -254,7 +252,6 @@ class Icon(AutotoolsPackage):
                 flags["ICON_ECRAD_FCFLAGS"].extend(["-qno-opt-dynamic-align", "-no-fma", "-fpe0"])
 
         elif self.compiler.name == "nvhpc":
-
             flags["CFLAGS"].extend(["-g", "-O2"])
             flags["FCFLAGS"].extend(
                 ["-g", "-O2", "-Mrecursive", "-Mallocatable=03", "-Mstack_arrays"]