Repository: osbuild/osbuild
Pull request #1716 (2024-04-11 07:52:37)

tools: extract common _test_depsolve_both_dnf_dnf5 helper.
Given that the tests `test_depsolve{,_dnf5}` are identical except the `command` extract a common helper. This will also help to avoid drift between the two resolvers.

mvo5: Sorry, this had conflicts so I had to update it and while doing this tweaked it to actually just use pytest parametrization. Not actually sure if it's a real win and if people prefer the pervious approach I'm fine with that too.

Patch:
diff --git a/Makefile b/Makefile
index 5a51888b..630f5fe9 100644
--- a/Makefile
+++ b/Makefile
@@ -245,7 +245,6 @@ test-run:
 .PHONY: test-all
 test-all:
 	@$(PYTHON3) -m pytest \
-			$(SRCDIR)/test \
 			--rootdir=$(SRCDIR) \
 			-v
 
diff --git a/stages/org.osbuild.bootc.install-to-filesystem b/stages/org.osbuild.bootc.install-to-filesystem
index fde611d3..6edcbf8f 100755
--- a/stages/org.osbuild.bootc.install-to-filesystem
+++ b/stages/org.osbuild.bootc.install-to-filesystem
@@ -40,6 +40,9 @@ def main(options, inputs, paths):
         # customize kernel-args
         for karg in options.get("kernel-args", []):
             pargs.extend(["--karg", karg])
+        target_imgref = options.get("target-imgref")
+        if target_imgref:
+            pargs.extend(["--target-imgref", target_imgref])
         # add target and go
         pargs.append(dst)
         subprocess.run(pargs, env=env, check=True)
diff --git a/stages/org.osbuild.bootc.install-to-filesystem.meta.json b/stages/org.osbuild.bootc.install-to-filesystem.meta.json
index db29e35d..f2367b52 100644
--- a/stages/org.osbuild.bootc.install-to-filesystem.meta.json
+++ b/stages/org.osbuild.bootc.install-to-filesystem.meta.json
@@ -40,6 +40,10 @@
           "items": {
             "type": "string"
           }
+        },
+        "target-imgref": {
+          "description": "Specify the image to fetch for subsequent updates",
+          "type": "string"
         }
       }
     },
diff --git a/stages/org.osbuild.kickstart b/stages/org.osbuild.kickstart
index f3439cfc..21d03ace 100755
--- a/stages/org.osbuild.kickstart
+++ b/stages/org.osbuild.kickstart
@@ -243,6 +243,9 @@ def main(tree, options):  # pylint: disable=too-many-branches
     if reboot:
         config += [reboot]
     config += make_network(options)
+    kargs_append = options.get("bootloader", {}).get("append")
+    if kargs_append:
+        config += [f"bootloader --append='{kargs_append}'"]
 
     target = os.path.join(tree, path)
     base = os.path.dirname(target)
diff --git a/stages/org.osbuild.kickstart.meta.json b/stages/org.osbuild.kickstart.meta.json
index 773c9f09..21ad84b3 100644
--- a/stages/org.osbuild.kickstart.meta.json
+++ b/stages/org.osbuild.kickstart.meta.json
@@ -542,6 +542,17 @@
         "items": {
           "$ref": "#/definitions/network-options-ref"
         }
+      },
+      "bootloader": {
+        "description": "Specifies how the boot loader should be installed",
+        "additionalProperties": false,
+        "type": "object",
+        "properties": {
+          "append": {
+            "description": "Specifies additional kernel parameters",
+            "type": "string"
+          }
+        }
       }
     }
   }
diff --git a/stages/org.osbuild.systemd.unit.create b/stages/org.osbuild.systemd.unit.create
index f4c78181..f0b61016 100755
--- a/stages/org.osbuild.systemd.unit.create
+++ b/stages/org.osbuild.systemd.unit.create
@@ -1,6 +1,5 @@
 #!/usr/bin/python3
 import configparser
-import subprocess
 import sys
 
 import osbuild.api
@@ -48,8 +47,6 @@ def main(tree, options):
     with open(f"{systemd_dir}/{filename}", "w", encoding="utf8") as f:
         config.write(f, space_around_delimiters=False)
 
-    subprocess.run(["systemd-analyze", "verify", f"{systemd_dir}/{filename}"], check=True)
-
 
 if __name__ == '__main__':
     args = osbuild.api.arguments()
diff --git a/stages/org.osbuild.systemd.unit.create.meta.json b/stages/org.osbuild.systemd.unit.create.meta.json
index d64d01ec..8919faab 100644
--- a/stages/org.osbuild.systemd.unit.create.meta.json
+++ b/stages/org.osbuild.systemd.unit.create.meta.json
@@ -18,6 +18,7 @@
     "    - 'DefaultDependencies' - bool",
     "    - 'Requires' - [strings]",
     "    - 'Wants' - [strings]",
+    "    - 'After' - [strings]",
     "  - 'Service' section",
     "    - 'Type' - string",
     "    - 'RemainAfterExit' - bool",
@@ -83,6 +84,12 @@
                   "type": "string"
                 }
               },
+              "After": {
+                "type": "array",
+                "items": {
+                  "type": "string"
+                }
+              },
               "Requires": {
                 "type": "array",
                 "items": {
diff --git a/tools/osbuild-depsolve-dnf b/tools/osbuild-depsolve-dnf
index d652bfe2..adfc4040 100755
--- a/tools/osbuild-depsolve-dnf
+++ b/tools/osbuild-depsolve-dnf
@@ -447,7 +447,7 @@ def validate_request(request):
     if not request.get("releasever"):
         return {
             "kind": "InvalidRequest",
-            "reason": "no 'module_platform_id' specified"
+            "reason": "no 'releasever' specified"
         }
     arguments = request.get("arguments")
     if not arguments:
diff --git a/tools/osbuild-depsolve-dnf5 b/tools/osbuild-depsolve-dnf5
index 8d3885a9..1f060eeb 100755
--- a/tools/osbuild-depsolve-dnf5
+++ b/tools/osbuild-depsolve-dnf5
@@ -573,7 +573,7 @@ def validate_request(request):
     if not request.get("releasever"):
         return {
             "kind": "InvalidRequest",
-            "reason": "no 'module_platform_id' specified"
+            "reason": "no 'releasever' specified"
         }
     arguments = request.get("arguments")
     if not arguments:


Test patch:
diff --git a/stages/test/test_bootc_install_to_fs.py b/stages/test/test_bootc_install_to_fs.py
index 36c53975..f31dc03e 100644
--- a/stages/test/test_bootc_install_to_fs.py
+++ b/stages/test/test_bootc_install_to_fs.py
@@ -53,9 +53,13 @@ FAKE_INPUTS = {
     ({"kernel-args": ["arg1", "arg2"]}, ["--karg", "arg1", "--karg", "arg2"]),
     # all
     ({"root-ssh-authorized-keys": ["key1", "key2"],
-      "kernel-args": ["arg1", "arg2"]},
+      "kernel-args": ["arg1", "arg2"],
+      "target-imgref": "quay.io/img/ref",
+      },
      ["--root-ssh-authorized-keys", "/tmp/fake-named-tmpfile-name",
-      "--karg", "arg1", "--karg", "arg2"],
+      "--karg", "arg1", "--karg", "arg2",
+      "--target-imgref", "quay.io/img/ref",
+      ],
      ),
 ])
 @patch("subprocess.run")
diff --git a/stages/test/test_kickstart.py b/stages/test/test_kickstart.py
index e62f4433..1aeeb305 100644
--- a/stages/test/test_kickstart.py
+++ b/stages/test/test_kickstart.py
@@ -234,6 +234,7 @@ TEST_INPUT = [
         },
         "ostreecontainer --url=http://some-ostree-url.com/foo",
     ),
+    ({"bootloader": {"append": "karg1 karg2=0"}}, "bootloader --append='karg1 karg2=0'"),
 ]
 
 
diff --git a/stages/test/test_systemd_unit_create.py b/stages/test/test_systemd_unit_create.py
index b686261d..a6c22c95 100644
--- a/stages/test/test_systemd_unit_create.py
+++ b/stages/test/test_systemd_unit_create.py
@@ -1,7 +1,7 @@
 #!/usr/bin/python3
 
 import os
-import os.path
+import subprocess
 import textwrap
 
 import pytest
@@ -66,14 +66,27 @@ def test_systemd_unit_create(tmp_path, stage_module, unit_type, unit_path, expec
                 ],
                 "ConditionPathIsDirectory": [
                     "|!/etc/mydir"
-                ]
+                ],
+                # need to use real units otherwise the validation will fail
+                "Wants": [
+                    "basic.target",
+                    "sysinit.target",
+                ],
+                "Requires": [
+                    "paths.target",
+                    "sockets.target",
+                ],
+                "After": [
+                    "sysinit.target",
+                    "default.target",
+                ],
             },
             "Service": {
                 "Type": "oneshot",
                 "RemainAfterExit": True,
                 "ExecStart": [
-                    "mkdir -p /etc/mydir",
-                    "touch /etc/myfile"
+                    "/usr/bin/mkdir -p /etc/mydir",
+                    "/usr/bin/touch /etc/myfile"
                 ],
                 "Environment": [
                     {
@@ -100,24 +113,53 @@ def test_systemd_unit_create(tmp_path, stage_module, unit_type, unit_path, expec
             }
         }
     }
+
+    # create units in the tree for systemd-analyze verify
+    systemd_dir = "usr/lib/systemd/system/"
+    # if a unit file is empty, it gets detected as "masked" and fails verification
+    testutil.make_fake_tree(tmp_path, {
+        systemd_dir + "basic.target": "[Unit]\n",
+        systemd_dir + "sysinit.target": "[Unit]\n",
+        systemd_dir + "paths.target": "[Unit]\n",
+        systemd_dir + "sockets.target": "[Unit]\n",
+        "usr/bin/mkdir": "fake-mkdir",
+        "usr/bin/touch": "fake-touch",
+    })
+    for bin_name in ["mkdir", "touch"]:
+        os.chmod(tmp_path / "usr/bin" / bin_name, mode=0o755)
+
     expected_unit_path = tmp_path / expected_prefix / "create-directory.service"
-    # should the stage create the dir?
-    expected_unit_path.parent.mkdir(parents=True)
+    expected_unit_path.parent.mkdir(parents=True, exist_ok=True)
 
     stage_module.main(tmp_path, options)
     assert os.path.exists(expected_unit_path)
+    if unit_type == "system":
+        # When verifying user units, systemd expects runtime directories and more generally a booted system. Setting
+        # something up like that to verify it is more trouble than it's worth, especially since the system units are
+        # identical.
+        subprocess.run(["systemd-analyze", "verify",
+                        "--man=no",  # disable manpage checks
+                        f"--root={tmp_path}",  # verify commands and units in the root tree
+                        expected_unit_path],
+                       check=True)
     assert expected_unit_path.read_text(encoding="utf-8") == textwrap.dedent("""\
     [Unit]
     Description=Create directory
     DefaultDependencies=False
     ConditionPathExists=|!/etc/myfile
     ConditionPathIsDirectory=|!/etc/mydir
+    Wants=basic.target
+    Wants=sysinit.target
+    Requires=paths.target
+    Requires=sockets.target
+    After=sysinit.target
+    After=default.target
 
     [Service]
     Type=oneshot
     RemainAfterExit=True
-    ExecStart=mkdir -p /etc/mydir
-    ExecStart=touch /etc/myfile
+    ExecStart=/usr/bin/mkdir -p /etc/mydir
+    ExecStart=/usr/bin/touch /etc/myfile
     Environment="DEBUG=1"
     Environment="TRACE=1"
     EnvironmentFile=/etc/example.env
diff --git a/tools/test/test_depsolve.py b/tools/test/test_depsolve.py
index cf282bc9..47c5459e 100644
--- a/tools/test/test_depsolve.py
+++ b/tools/test/test_depsolve.py
@@ -27,6 +27,10 @@ def has_dnf5():
     return bool(importlib.util.find_spec("libdnf5"))
 
 
+def has_dnf():
+    return bool(importlib.util.find_spec("dnf"))
+
+
 def depsolve(pkgs, repos, root_dir, cache_dir, command):
     req = {
         "command": "depsolve",
@@ -256,24 +260,17 @@ def cache_dir_fixture(tmpdir_factory):
 
 
 @pytest.mark.parametrize("test_case", test_cases)
-def test_depsolve(tmp_path, repo_servers, test_case, cache_dir):
-    pks = test_case["packages"]
+@pytest.mark.parametrize("dnf_cmd, detect_fn", [
+    ("./tools/osbuild-depsolve-dnf", has_dnf),
+    ("./tools/osbuild-depsolve-dnf5", has_dnf5),
+])
+def test_depsolve(tmp_path, cache_dir, repo_servers, dnf_cmd, detect_fn, test_case):
+    if not detect_fn():
+        pytest.skip(f"cannot import support for {dnf_cmd}")
 
-    for repo_configs, root_dir in config_combos(tmp_path, repo_servers):
-        res = depsolve(pks, repo_configs, root_dir, cache_dir, "./tools/osbuild-depsolve-dnf")
-        assert {pkg["name"] for pkg in res["packages"]} == test_case["results"]["packages"]
-        assert res["repos"].keys() == test_case["results"]["reponames"]
-        for repo in res["repos"].values():
-            assert repo["gpgkeys"] == [TEST_KEY + repo["id"]]
-
-
-@pytest.mark.skipif(not has_dnf5(), reason="libdnf5 not available")
-@pytest.mark.parametrize("test_case", test_cases)
-def test_depsolve_dnf5(tmp_path, repo_servers, test_case, cache_dir):
     pks = test_case["packages"]
-
-    for repo_configs, repos_dir in config_combos(tmp_path, repo_servers):
-        res = depsolve(pks, repo_configs, repos_dir, cache_dir, "./tools/osbuild-depsolve-dnf5")
+    for repo_configs, root_dir in config_combos(tmp_path, repo_servers):
+        res = depsolve(pks, repo_configs, root_dir, cache_dir, dnf_cmd)
         assert {pkg["name"] for pkg in res["packages"]} == test_case["results"]["packages"]
         assert res["repos"].keys() == test_case["results"]["reponames"]
         for repo in res["repos"].values():