From 3d41b71664c626e3f30c10e1670c5d07fee252a7 Mon Sep 17 00:00:00 2001
From: Harmen Stoppels <harmenstoppels@gmail.com>
Date: Thu, 23 Feb 2023 01:44:47 +0100
Subject: [PATCH] buildcache push: ensure bool arguments for include_* (#35632)

Fixes a bug introduced in 44ed0de8c077630148c213d3c7f40a8965eb6f94
where the push method of binary_distribution now takes named args
include_root and include_depedencies, to avoid the **kwarg hole.

But the call site wasn't update and we passed a dict of keys/values instead
of arguments, which resulted in a call like this:

```
push(include_root={"include_root": True, "include_dependencies": False})
```

This commit fixes that, and adds a test to see if we push the correct packages.
---
 lib/spack/spack/binary_distribution.py |  6 ++-
 lib/spack/spack/ci.py                  |  3 +-
 lib/spack/spack/test/bindist.py        | 67 ++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index e9e48b4010..9758e8b9ea 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -1332,7 +1332,7 @@ def nodes_to_be_packaged(specs, root=True, dependencies=True):
         return list(filter(packageable, nodes))
 
 
-def push(specs, push_url, include_root=True, include_dependencies=True, **kwargs):
+def push(specs, push_url, include_root: bool = True, include_dependencies: bool = True, **kwargs):
     """Create a binary package for each of the specs passed as input and push them
     to a given push URL.
 
@@ -1345,6 +1345,10 @@ def push(specs, push_url, include_root=True, include_dependencies=True, **kwargs
         **kwargs: TODO
 
     """
+    # Be explicit about the arugment type
+    if type(include_root) != bool or type(include_dependencies) != bool:
+        raise ValueError("Expected include_root/include_dependencies to be True/False")
+
     nodes = nodes_to_be_packaged(specs, root=include_root, dependencies=include_dependencies)
 
     # TODO: This seems to be an easy target for task
diff --git a/lib/spack/spack/ci.py b/lib/spack/spack/ci.py
index 8be254be23..7afda4babf 100644
--- a/lib/spack/spack/ci.py
+++ b/lib/spack/spack/ci.py
@@ -1437,9 +1437,8 @@ def _push_mirror_contents(env, specfile_path, sign_binaries, mirror_url):
     hashes = env.all_hashes() if env else None
     matches = spack.store.specfile_matches(specfile_path, hashes=hashes)
     push_url = spack.mirror.Mirror.from_url(mirror_url).push_url
-    spec_kwargs = {"include_root": True, "include_dependencies": False}
     kwargs = {"force": True, "allow_root": True, "unsigned": unsigned}
-    bindist.push(matches, push_url, spec_kwargs, **kwargs)
+    bindist.push(matches, push_url, include_root=True, include_dependencies=False, **kwargs)
 
 
 def push_mirror_contents(env, specfile_path, mirror_url, sign_binaries):
diff --git a/lib/spack/spack/test/bindist.py b/lib/spack/spack/test/bindist.py
index d959258c85..81f5f7377c 100644
--- a/lib/spack/spack/test/bindist.py
+++ b/lib/spack/spack/test/bindist.py
@@ -885,3 +885,70 @@ def urlopen(request: urllib.request.Request):
 
     with pytest.raises(bindist.FetchIndexError, match="Could not fetch index"):
         fetcher.conditional_fetch()
+
+
+@pytest.mark.parametrize(
+    "root,deps,expected",
+    [
+        (
+            True,
+            True,
+            [
+                "dttop",
+                "dtbuild1",
+                "dtbuild2",
+                "dtlink2",
+                "dtrun2",
+                "dtlink1",
+                "dtlink3",
+                "dtlink4",
+                "dtrun1",
+                "dtlink5",
+                "dtrun3",
+                "dtbuild3",
+            ],
+        ),
+        (
+            False,
+            True,
+            [
+                "dtbuild1",
+                "dtbuild2",
+                "dtlink2",
+                "dtrun2",
+                "dtlink1",
+                "dtlink3",
+                "dtlink4",
+                "dtrun1",
+                "dtlink5",
+                "dtrun3",
+                "dtbuild3",
+            ],
+        ),
+        (True, False, ["dttop"]),
+        (False, False, []),
+    ],
+)
+def test_correct_specs_are_pushed(
+    root, deps, expected, default_mock_concretization, tmpdir, temporary_store, monkeypatch
+):
+    # Concretize dttop and add it to the temporary database (without prefixes)
+    spec = default_mock_concretization("dttop")
+    temporary_store.db.add(spec, directory_layout=None)
+
+    # Create a mirror push url
+    push_url = spack.mirror.Mirror.from_local_path(str(tmpdir)).push_url
+
+    packages_to_push = []
+
+    def fake_build_tarball(node, push_url, **kwargs):
+        assert push_url == push_url
+        assert not kwargs
+        assert isinstance(node, Spec)
+        packages_to_push.append(node.name)
+
+    monkeypatch.setattr(bindist, "_build_tarball", fake_build_tarball)
+
+    bindist.push([spec], push_url, include_root=root, include_dependencies=deps)
+
+    assert packages_to_push == expected