control armbian-common: pull in libpam-systemd for its dbus dependency #32
Conversation
WalkthroughA single dependency addition to the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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.
📒 Files selected for processing (1)
debian/control(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tabrisnet
Repo: armbian/apa PR: 25
File: debian/control:23-23
Timestamp: 2025-12-06T21:01:15.488Z
Learning: In the armbian/apa repository's debian/control, iputils-ping is temporarily added to armbian-common's Depends as a workaround for a capability issue where /bin/ping cannot create raw sockets. The proper solution involves: (1) PR #27, (2) removing the chmod u+s workaround code from armbian/build (armbian/build#9033), and (3) adding libcap2-bin to armbian/build's debootstrap package list so that iputils-ping's postinst script can use setcap to properly configure /bin/ping. Once these pieces are complete and merged, iputils-ping should be removed from armbian-common's Depends as it will no longer be needed.
📚 Learning: 2025-12-06T21:01:15.488Z
Learnt from: tabrisnet
Repo: armbian/apa PR: 25
File: debian/control:23-23
Timestamp: 2025-12-06T21:01:15.488Z
Learning: In the armbian/apa repository's debian/control, iputils-ping is temporarily added to armbian-common's Depends as a workaround for a capability issue where /bin/ping cannot create raw sockets. The proper solution involves: (1) PR #27, (2) removing the chmod u+s workaround code from armbian/build (armbian/build#9033), and (3) adding libcap2-bin to armbian/build's debootstrap package list so that iputils-ping's postinst script can use setcap to properly configure /bin/ping. Once these pieces are complete and merged, iputils-ping should be removed from armbian-common's Depends as it will no longer be needed.
Applied to files:
debian/control
| iproute2, | ||
| logrotate, | ||
| linux-base, | ||
| libpam-systemd, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
libpam-systemd dependencies Debian Bookworm
💡 Result:
libpam-systemd (bookworm: 252.39-1~deb12u1) Depends:
- default-dbus-system-bus (virtual; provided by dbus or dbus-broker). [1]
- libc6 (>= 2.34). [1]
- libcap2 (>= 1:2.10). [1]
- libpam-runtime. [1]
- libpam0g (>= 0.99.7.1). [1]
- systemd (= 252.39-1~deb12u1). [1]
- systemd-sysv. [1]
Recommends:
- dbus-user-session. [1]
(There is a newer libpam-systemd in bookworm-backports if you need a newer systemd; see backports page.) [2]
Sources: Debian package pages for libpam-systemd (bookworm) and bookworm-backports. [1][2]
🌐 Web query:
libpam-systemd dependencies Ubuntu Noble
💡 Result:
For Ubuntu Noble (24.04), libpam-systemd (current package versions) has these package relationships:
- Depends: default-dbus-system-bus (provided by dbus or dbus-broker), libc6 (>= 2.39) or (>= 2.38 on some builds), libcap2 (>= 1:2.10), libpam-runtime, libpam0g (>= 0.99.7.1), systemd (= matching version), systemd-sysv. [1][2]
- Recommends: dbus-user-session. [1][2]
- Conflicts: libpam-elogind-compat. [1][2]
Sources: Ubuntu package pages / Launchpad for libpam-systemd in noble. [1][2]
🌐 Web query:
libpam-systemd package depends dbus
💡 Result:
Short answer: It can. Older libpam-systemd packages depended on the dbus package; modern Debian/Ubuntu builds depend on the virtual default-dbus-system-bus (provided by dbus or dbus-broker) or otherwise recommend dbus-user-session. [1][2]
Sources:
- Debian bullseye package details (libpam-systemd — depends: dbus). [1]
- Debian bookworm / Ubuntu package details (libpam-systemd — depends: default-dbus-system-bus, recommends dbus-user-session). [2]
Add dbus directly to Depends instead of relying on libpam-systemd as an indirect dependency.
libpam-systemd depends on the virtual package default-dbus-system-bus (provided by either dbus or dbus-broker), not on dbus directly. This means:
-
No guarantee of dbus installation: If
dbus-brokeris installed to satisfy the virtual package, the script may still fail if it depends on dbus-specific binaries or behavior. -
Indirect and fragile: The dependency chain libpam-systemd → default-dbus-system-bus → dbus adds unnecessary indirection. If the dependency strategy changes, dbus may no longer be pulled in.
-
Unclear intent: Adding libpam-systemd obscures the actual requirement, which is dbus for the machine-id deletion in rootfs-create.sh.
Add dbus directly to the Depends section of armbian-common instead. This ensures dbus is explicitly required and available across all builds.
🤖 Prompt for AI Agents
In debian/control around line 28, the package currently lists libpam-systemd in
Depends but relies on it indirectly to pull in dbus; explicitly add dbus to the
armbian-common Depends line (comma-separated, following existing formatting and
versioning style) so dbus is guaranteed to be installed for rootfs-create.sh;
keep libpam-systemd only if still needed for other reasons.
There was a problem hiding this comment.
I'd say not an "instead" but an "also". albeit I think if I did I'd add dbus-user-session from
config/cli/common/main/packages#L8
but that is already in Recommends
There was a problem hiding this comment.
@tabrisnet You're absolutely right! Let me clarify my recommendation:
Keep libpam-systemd AND add dbus to Depends — both serve distinct purposes:
- libpam-systemd: Provides PAM integration with systemd (user sessions, cgroups, resource limits, etc.) — valuable functionality that should stay
- dbus: The message bus system explicitly needed by your rootfs-create.sh script for the machine-id handling
So the suggestion should be to add dbus as an additional entry in the Depends section, not as a replacement. This ensures dbus is explicitly present for your rootfs script while keeping libpam-systemd for its systemd integration features.
Regarding dbus-user-session: you're correct that it's already in Recommends (line 42), which is the appropriate place for it since it's not critical for the basic boot process but useful for user session management.
Would you like to add dbus to the Depends section as well?
🧠 Learnings used
Learnt from: tabrisnet
Repo: armbian/apa PR: 25
File: debian/control:23-23
Timestamp: 2025-12-06T21:01:15.488Z
Learning: In the armbian/apa repository's debian/control, iputils-ping is temporarily added to armbian-common's Depends as a workaround for a capability issue where /bin/ping cannot create raw sockets. The proper solution involves: (1) PR #27, (2) removing the chmod u+s workaround code from armbian/build (armbian/build#9033), and (3) adding libcap2-bin to armbian/build's debootstrap package list so that iputils-ping's postinst script can use setcap to properly configure /bin/ping. Once these pieces are complete and merged, iputils-ping should be removed from armbian-common's Depends as it will no longer be needed.
There was a problem hiding this comment.
Note that my goal is duplicating armbian/build's config/cli and not straying far from that.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Clone armbian/build to check config/cli package definitions
git clone --depth 1 https://github.com/armbian/build.git /tmp/armbian-build 2>/dev/null || true
# Check the packages files in config/cli
echo "=== Contents of config/cli/common/main/packages ==="
cat /tmp/armbian-build/config/cli/common/main/packages 2>/dev/null || echo "File not found"
echo -e "\n=== Contents of config/cli/common/main/packages.additional ==="
cat /tmp/armbian-build/config/cli/common/main/packages.additional 2>/dev/null || echo "File not found"
# Search for dbus in all config/cli files
echo -e "\n=== Searching for 'dbus' in config/cli ==="
rg -n "^dbus" /tmp/armbian-build/config/cli/ 2>/dev/null || echo "No matches found"
Length of output: 1433
@tabrisnet Understood! Looking at armbian/build's config/cli, I can confirm your current approach is correct:
armbian/build's config/cli/common/main:
packagesincludes:dbus-user-session(line 8)packages.additionalincludes:libpam-systemddbusis NOT explicitly listed
Your debian/control already matches this:
- ✅
libpam-systemdin Depends (from packages.additional) - ✅
dbus-user-sessionin Recommends (from packages)
Since armbian/build doesn't explicitly list dbus and instead relies on it being pulled in through dependencies, your current PR correctly replicates that approach. My earlier suggestion to add dbus explicitly would have been straying from the source configuration.
Your implementation is spot-on for duplicating armbian/build's config/cli! 🎯
🧠 Learnings used
Learnt from: tabrisnet
Repo: armbian/apa PR: 25
File: debian/control:23-23
Timestamp: 2025-12-06T21:01:15.488Z
Learning: In the armbian/apa repository's debian/control, iputils-ping is temporarily added to armbian-common's Depends as a workaround for a capability issue where /bin/ping cannot create raw sockets. The proper solution involves: (1) PR #27, (2) removing the chmod u+s workaround code from armbian/build (armbian/build#9033), and (3) adding libcap2-bin to armbian/build's debootstrap package list so that iputils-ping's postinst script can use setcap to properly configure /bin/ping. Once these pieces are complete and merged, iputils-ping should be removed from armbian-common's Depends as it will no longer be needed.
|
we are getting into the situation where we're seeing build-time depends dominating run-time dependencies now. this will need further consideration. |
|
We could change the `rm` to `rm -fv`
I will say it was useful, initially, to have this "problem" in that it made me reorder the APA hook call to before this.
on the other hand, it *is* difficult to have a system without `dbus` now. Which is to say, "how minimal is minimal?"
What else is hiding in `config/cli` that we are missing, that is essential?
|
|
more missing expectations, this time definitely runtime: armbian/build#9000 (comment) |
|
Yes, I agree this refactoring is useful as it exposes a bunch of assumptions which can be revisited as well as decisions made that maybe no longer need to be made. They are also a part of why the previous code had become so unmaintainable.
The proper way to deal with this would likely be to package this in something like armbian-config package and have that package depend on the tools it requires. Obviously, we would need to find a balance with the proliferation of packages. Thank you for your help in finding all the rough edges. It is much appreciated. |
2 participants
so
lib/functions/rootfs/rootfs-create.sh#L345 expectsdbusto be there with:run_host_command_logged rm -v "${SDCARD}/var/lib/dbus/machine-id"On desktop builds, this already gets pulled in. on
minimalbuilds, not so much.config/cli/common/main/packages.additional#L35 haslibpam-systemd.I'm not 100% sure what generates
/var/lib/dbus/machine-id, but thedbuspkg'spostrmscript also deletes it.Google suggests that
dbus-uuidgenis used, which, intrixie, is in pkgdbus-binwhich is subsequently a dependency ofdbus.