frontend: prefer dnf over tdnf to work around tdnf GPG and forcearch limitations#991
Open
cpuguy83 wants to merge 2 commits intoproject-dalec:mainfrom
Open
frontend: prefer dnf over tdnf to work around tdnf GPG and forcearch limitations#991cpuguy83 wants to merge 2 commits intoproject-dalec:mainfrom
cpuguy83 wants to merge 2 commits intoproject-dalec:mainfrom
Conversation
cpuguy83
force-pushed
the
fix_signing_with_base_img
branch
from
d465f16 to
97c0856
Compare
…limitations tdnf fails when installing signed local RPMs (from the cmdline virtual repo) into an installroot with a populated RPM database, because it requires a gpgkey entry for cmdline which is a synthetic repo with no config. This manifests when building containers with a custom base image on azlinux/mariner distros. Rather than working around individual tdnf bugs, prefer dnf when it is available. The install script now checks for dnf at runtime and switches from tdnf transparently. The same-platform worker bootstrap is updated to install dnf as a separate first step (mirroring the cross-arch path) so that subsequent installs benefit from dnf. Also remove hardcoded GPG email from test key ID lookups in favor of selecting the first available key. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
cpuguy83
force-pushed
the
fix_signing_with_base_img
branch
from
97c0856 to
08fb0cd
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands RPM-based distro support for a “deps-only” container target, adjusts RPM install behavior to prefer dnf over tdnf when available, and adds coverage for a signed-RPM install edge case when using a custom base image.
Changes:
- Add
container/depsonlytarget plumbing to the RPM distro implementation and exercise it in integration tests. - Prefer
dnfovertdnfat install time whendnfis present (and bootstrapdnfearlier in worker creation). - Add a signing-focused regression test that signs built RPMs and validates container builds with a custom base image.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
test/target_ubuntu_test.go |
Updates GPG key ID extraction used when signing an APT repo in tests. |
test/target_rockylinux_test.go |
Adds deps-only target config and an extra signing regression test invocation. |
test/target_azlinux_test.go |
Adds deps-only target config, switches repo signing hook used by tests, and runs the signing regression test with a custom base image. |
test/target_almalinux_test.go |
Adds deps-only target config and an extra signing regression test invocation. |
test/signing_test.go |
Introduces RPM signing helper + regression test for installing signed RPMs into a custom base image. |
test/linux_target_test.go |
Adds DepsOnly to targetConfig and adds deps-only integration tests validating runtime deps-only behavior. |
targets/linux/rpm/distro/worker.go |
Bootstraps dnf earlier (when needed) and introduces slices usage. |
targets/linux/rpm/distro/dnf_install.go |
Removes unused download-only options and prefers dnf over tdnf automatically when dnf exists. |
targets/linux/rpm/distro/container.go |
Adds BuildContainerWithPackages and updates deps-only handler to install package names rather than local RPMs. |
docker-bake.hcl |
Removes the bake-based deps-only test target from the test group. |
| if samePlatform(targetPlat, buildPlat) { | ||
| if slices.Contains(cfg.BuilderPackages, "dnf") { | ||
| // Install dnf first since this will be bootstrapped with a different package manager | ||
| // This keeps the package cache for the bootstrap mananager separate from the other base packages we use. |
There was a problem hiding this comment.
Typo in comment: "bootstrap mananager" should be "bootstrap manager".
Suggested change
| // This keeps the package cache for the bootstrap mananager separate from the other base packages we use. | |
| // This keeps the package cache for the bootstrap manager separate from the other base packages we use. |
Comment on lines
+677
to
+679
| // This triggers skipBase=true in BuildContainer, meaning the RPMs | ||
| // are installed via "tdnf install /path/to/signed.rpm --installroot=/tmp/rootfs" | ||
| // into the custom base image's rootfs. |
There was a problem hiding this comment.
These comments describe the install path specifically as tdnf install ..., but this helper is also called from dnf-based distros (e.g., AlmaLinux/Rocky). Consider rewording to describe the generic local-RPM install path (package manager varies) or constrain the helper to tdnf-based distros only.
Suggested change
| // This triggers skipBase=true in BuildContainer, meaning the RPMs | |
| // are installed via "tdnf install /path/to/signed.rpm --installroot=/tmp/rootfs" | |
| // into the custom base image's rootfs. | |
| // This triggers skipBase=true in BuildContainer, meaning the RPMs are | |
| // installed from the local filesystem into the custom base image's | |
| // rootfs using the target distro's package manager (for example, | |
| // tdnf or dnf with an appropriate --installroot). |
invidian
requested changes
Mar 12, 2026
cpuguy83
force-pushed
the
fix_signing_with_base_img
branch
from
08fb0cd to
c590d2d
Compare
The way depsonly worked was always a bit janky and didn't actually support the full dependency constraint specification. Additionally I found the shift to dnf from tdnf broke due to `--alldeps` being missing (possibly just in mariner2, but still missing). This shifts depsonly to use BuildPkg and BuildContainer where we create a meta package with just the runtime deps. Because depsonly allows specifying a partial spec (ie missing things like name, license, other normally required fields) we have to fill in those details so rpmbuild can succeed. Add deps-only integration tests for all RPM distros with two sub-tests: - minimal spec: only runtime deps, verifies curl is installed - full spec: includes sources, build steps, and a shell script artifact; verifies runtime deps are installed and build artifacts are excluded - replaces the "e2e" test in docker-bake.hcl and tets all relevant distros Signed-off-by: Brian Goff <cpuguy83@gmail.com>
cpuguy83
force-pushed
the
fix_signing_with_base_img
branch
from
c590d2d to
0a95d13
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
tdnf fails when installing signed local RPMs (from the cmdline virtual repo) into an installroot with a populated RPM database, because it requires a gpgkey entry for cmdline which is a synthetic repo with no config. This manifests when building containers with a custom base image on azlinux/mariner distros.
Rather than working around individual tdnf bugs, prefer dnf when it is available. The install script now checks for dnf at runtime and switches from tdnf transparently. The same-platform worker bootstrap is updated to install dnf as a separate first step (mirroring the cross-arch path) so that subsequent installs benefit from dnf.
This also exposed a couple of other issues:
This fixes a real issue we saw in our builds where azlinux3/container is used but a custom base image is set instead of building one from scratch (as is the default).