framework mmdebstrap fetch_distro_keyring - ignore dpkg-deb/tar non-essential errors

[tabrisnet]

Description

fix for issue re tar barfing when it can't chmod/chgrp

11:32:11 <+DC-IRC> [Discord] <vid7> tabris: pretty sure your recent keyring changes have busted builds ? (at least on trixie)
11:32:59 <+DC-IRC> [Discord] <vid7> ```
11:33:00 <+DC-IRC> [Discord] <vid7> ...
11:33:01 <+DC-IRC> [Discord] <vid7> tar: ./usr/share/keyrings/debian-archive-trixie-automatic.gpg: Cannot change ownership to uid 0, gid 0: Operation not permitted
11:33:02 <+DC-IRC> [Discord] <vid7> tar: ./usr/share/keyrings/debian-archive-trixie-security-automatic.gpg: Cannot change ownership to uid 0, gid 0: Operation not permitted
11:33:04 <+DC-IRC> [Discord] <vid7> tar: ./usr/share/keyrings/debian-archive-trixie-stable.gpg: Cannot change ownership to uid 0, gid 0: Operation not permitted
11:33:05 <+DC-IRC> [Discord] <vid7> tar: Exiting with failure status due to previous errors
11:33:06 <+DC-IRC> [Discord] <vid7> dpkg-deb: error: tar subprocess returned error exit status 2
11:33:07 <+DC-IRC> [Discord] <vid7> [?|?] error! [ fetch_distro_keyring dpkg-deb -x /armbian/cache/keyrings/debian/debian-archive-keyring_2025.1_all.deb failed ]

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.

• This error doesn't occur in my builds

[coderabbitai]

Walkthrough

The fetch_distro_keyring function in lib/functions/rootfs/distro-specific.sh has been modified to change its error-handling strategy for dpkg-deb extraction. Instead of failing immediately on extraction errors, the extraction step now suppresses failures with || /bin/true. The function then shifts to a staged validation approach that explicitly checks for the presence of keyring files (debian-archive-keyring.pgp/.gpg for Debian, debian-ports-archive-keyring.pgp/.gpg for Debian Ports, and ubuntu-archive-keyring.gpg for Ubuntu) before proceeding. Failures are now deferred until keyring presence validation occurs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Error handling logic changes: The shift from fail-fast to deferred-error detection requires careful verification that all error paths are properly covered
• Multi-branch validation: Debian, Debian Ports, and Ubuntu branches apply similar but distinct validation patterns that each need verification
• Keyring file precedence: The logic for checking .pgp vs .gpg variants and their precedence should be verified for correctness

Possibly related PRs

• Framework - add support for mmdebstrap #8785: Introduced the fetch_distro_keyring function that this PR modifies for error handling and keyring validation

Suggested reviewers

• rpardini
• prahal
• clee
• igorpecovnik

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: modifying fetch_distro_keyring to ignore non-essential dpkg-deb/tar errors.
Description check	✅ Passed	The description is directly related to the changeset, providing context about the tar chmod/chgrp error and explaining how the PR addresses this issue.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

lib/functions/rootfs/distro-specific.sh (3)

116-126: Error suppression + file validation is reasonable, but consider logging extraction warnings.

Suppressing all dpkg-deb exit codes will hide genuine failures (disk full, corrupted .deb, etc.), though the file existence check (lines 118–126) provides a fallback validation gate. However, if extraction fails for a legitimate reason, users will see only "unable to find debian-archive-keyring.gpg" without diagnostic context about the actual extraction error.

Consider piping stderr to a log file so failures are recorded even when exit codes are ignored:

- dpkg-deb -x "${CACHEDIR}/${KEYRING_DEB}" "${CACHEDIR}" || /bin/true # ignore failures, we'll check a few lines down
+ dpkg-deb -x "${CACHEDIR}/${KEYRING_DEB}" "${CACHEDIR}" 2>&1 | tee -a /tmp/keyring-extraction.log || /bin/true

This preserves diagnostics without breaking the flow when chmod/chgrp errors occur.

---

134-142: Comment referencing "see above" reduces clarity when reading this section independently.

While DRY principles apply, the comment on line 134 saying "see above" requires readers to scroll back to the Debian branch to understand the rationale. Since Debian Ports is logically separate, consider either:

• Repeating the brief explanation inline for clarity
• Extracting the extraction logic into a helper function

Minor readability issue; not blocking.

---

159-162: Ubuntu validation uses inverted logic; consider aligning with Debian pattern for consistency.

The Ubuntu branch checks if [[ ! -e ... ]] exit_with_error, while Debian branches use if [[ -e ... ]] cp; elif ...; else exit_with_error. Both are correct, but the inconsistent pattern across branches may confuse maintainers.

Consider applying the same if/elif/else pattern as Debian for structural consistency, or document why Ubuntu differs:

- dpkg-deb -x "${CACHEDIR}/${KEYRING_DEB}" "${CACHEDIR}" || /bin/true # see above in debian block about ignoring errors
- if [[ ! -e "${CACHEDIR}/usr/share/keyrings/ubuntu-archive-keyring.gpg" ]]; then
-   exit_with_error "fetch_distro_keyring" "unable to find ubuntu-archive-keyring.gpg"
- fi
+ if [[ -e "${CACHEDIR}/usr/share/keyrings/ubuntu-archive-keyring.gpg" ]]; then
+   cp -l "${CACHEDIR}/usr/share/keyrings/ubuntu-archive-keyring.gpg" "${CACHEDIR}/"
+ else
+   exit_with_error "fetch_distro_keyring" "unable to find ubuntu-archive-keyring.gpg"
+ fi

This aligns the Ubuntu block with the Debian/Debian Ports structure.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b762945 and 9b4e44c.

📒 Files selected for processing (1)

• lib/functions/rootfs/distro-specific.sh (3 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: In lib/functions/rootfs/rootfs-create.sh, the FIXME comment about mmdebstrap usage with --aptopt is a future note related to PR #8785, which hasn't been merged yet.

Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: The `debootstrap` tool doesn't use `apt`, which is important context when reviewing apt-related configuration in rootfs creation code.

📚 Learning: 2025-10-24T04:46:22.901Z

Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: In lib/functions/rootfs/rootfs-create.sh, the FIXME comment about mmdebstrap usage with --aptopt is a future note related to PR #8785, which hasn't been merged yet.

Applied to files:

• lib/functions/rootfs/distro-specific.sh

[vidplace7]

RELEASE=trixie is compiling happily again for me with this fix 👍

[github-actions]

✅ This PR has been reviewed and approved — all set for merge!