Skip to content

iron out bugs in APA #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tabrisnet wants to merge 6 commits into from
Closed

iron out bugs in APA #25

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
86f4f2c
armbian-common: rework to add more dependencies/recommends from armbi…
tabrisnet Dec 5, 2025
d0a253f
armbian-common: avahi-browse isn't a package I can find in PUC or PDO…
tabrisnet Dec 5, 2025
8132a83
control: don't add linux-image to armbian-common
tabrisnet Dec 5, 2025
a435204
control: add armbian-desktop-cinnamon & armbian-desktop-i3-wm
tabrisnet Dec 5, 2025
b8bc653
control: armbian-bsp-cli gets added in armbian/build, we shouldn't do…
tabrisnet Dec 1, 2025
9e725be
control: iputils-ping is expected by the build system, so move it to …
tabrisnet Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 77 additions & 3 deletions debian/control
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,67 @@ Priority: required
Essential: yes
Architecture: all
Depends:
armbian-bsp|armbian-bsp-cli,
e2fsprogs,
initramfs-tools,
iproute2,
iputils-ping,
Copy link

@coderabbitai coderabbitai bot Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Consider fixing the upstream build script rather than adding iputils-ping as a hard dependency.

According to the PR discussion, iputils-ping is being added because lib/functions/rootfs/distro-agnostic.sh in armbian/build requires /bin/ping at build time. Adding runtime dependencies to work around build-time script requirements inflates the minimal system footprint. The cleaner solution is to fix the build script to handle missing /bin/ping gracefully or install it only in the build environment.

Based on PR objectives, leggewie referenced armbian/build#9033 for this fix.

</review_comment_end>

🤖 Prompt for AI Agents 
In debian/control around line 23, iputils-ping was added as a hard dependency to
satisfy a build-time call to /bin/ping; instead, update the upstream build
scripts so the package need not depend on ping at runtime: modify
armbian/build's lib/functions/rootfs/distro-agnostic.sh to check for the
existence of ping (or use command -v) before calling it and skip the
ping-dependent behavior when absent, or move any ping use into build-only logic
and ensure the build environment installs iputils-ping as a build-dependency
rather than adding it to runtime Depends.

Copy link
Contributor Author

@tabrisnet tabrisnet Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your analysis is underpinned by a significant misunderstanding. the relevant code in armbian/build is trying to workaround a real problem, albeit not in the right way, in the final built product. It's not a "build-time dependency" gone awry... it is a quick-fix to an empirical [observed but not understood] problem that has gone awry.

Basically the code, which armbian/build#9033 removes [and IMO is not the full/complete solution], is fixing an issue where /bin/ping can't do its job due to not having the ability to create raw sockets. But if armbian/build can't chmod u+s /bin/ping the build fails because set -e.

I rather believe that #27 plus armbian/build#9033 plus a change to add libcap2-bin to armbian/build's debootstrap package list will better solve the original issue. this works due to the iputils-ping package's postinst needing the setcap util to do the right thing to /bin/ping. If setcap isn't available, it's not considered a showstopper... just that under these circumstances ping needs sudo to operate.

Copy link

@coderabbitai coderabbitai bot Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tabrisnet, thank you for the clarification! I appreciate you explaining the actual issue here. You're absolutely right—I misunderstood the nature of the problem.

I now understand that:

This makes much more sense than my initial interpretation. The dependency on iputils-ping in armbian-common is appropriate, and the real fix is ensuring the capability setup happens correctly during the build process.

✏️ Learnings added 
Learnt from: tabrisnet
Repo: armbian/apa PR: 25
File: debian/control:23-23
Timestamp: 2025-12-06T20:57:13.004Z
Learning: In the armbian/apa repository's debian/control, iputils-ping is a legitimate runtime dependency for armbian-common. The package is needed because /bin/ping requires proper capabilities to create raw sockets in the final built product. The proper solution for capability setup involves: (1) including iputils-ping in armbian-common, (2) removing the workaround chmod u+s 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.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Contributor Author

@tabrisnet tabrisnet Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not quite. the solution I'm putting forth eliminates the need for this ugly chmod workaround, thus allowing us to remove iputils-ping from armbian-common's Depends.
But can't do that yet, not until the various pieces are complete/merged.

Copy link

@coderabbitai coderabbitai bot Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tabrisnet, thank you for the additional clarification! I apologize for continuing to misunderstand.

I now have the correct understanding:

So this is a transitional state until all the pieces are merged. Thank you for patiently correcting my understanding!

✏️ Learnings added 
Learnt from: tabrisnet
Repo: armbian/apa PR: 25
File: debian/control:23-23
Timestamp: 2025-12-06T21:01:15.459Z
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.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

⛔ Learnings removed 
Learnt from: tabrisnet
Repo: armbian/apa PR: 25
File: debian/control:23-23
Timestamp: 2025-12-06T20:57:13.004Z
Learning: In the armbian/apa repository's debian/control, iputils-ping is a legitimate runtime dependency for armbian-common. The package is needed because /bin/ping requires proper capabilities to create raw sockets in the final built product. The proper solution for capability setup involves: (1) including iputils-ping in armbian-common, (2) removing the workaround chmod u+s 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.
🧠 Learnings used 
Learnt from: tabrisnet
Repo: armbian/apa PR: 25
File: debian/control:23-23
Timestamp: 2025-12-06T20:57:13.004Z
Learning: In the armbian/apa repository's debian/control, iputils-ping is a legitimate runtime dependency for armbian-common. The package is needed because /bin/ping requires proper capabilities to create raw sockets in the final built product. The proper solution for capability setup involves: (1) including iputils-ping in armbian-common, (2) removing the workaround chmod u+s 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.

Copy link
Collaborator

@leggewie leggewie Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • iputils-ping is a legitimate runtime dependency for the final system

no, actually it is not

Copy link

@coderabbitai coderabbitai bot Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor Author

@tabrisnet tabrisnet Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bot is dumb, it's very easy to confuse, whether on purpose or not.
I agree that ping as a dependency is dumb. I was, as noted, trying to "just make it work".
Also note that I did correct the bot, so hopefully it won't try to learn that ping is über important.
What I'm not trying to do is take on a "fix all the things wrong in armbian/build by stepping on one landmine at a time" task.
Yes, things should be fixed. but parity [with the previous system] is a much easier standard than perfection. Perfection can be made into later tickets. That and I have a deep-loathing for Minimum Viable Products [missing functionality that was actually used by customers] replacing older stable, if with significant dumbass problems, systems; then those MVPs becoming the only thing you're allowed to use.

logrotate,
linux-base,
nano|vim,
netplan.io|network-manager,
openssh-server,
pastebinit,
apt-utils,
locales,
console-setup,
gnupg2,
ca-certificates,
tzdata,
${misc:Depends}
Recommends:
anacron,
avahi-browse,
alsa-utils,
avahi-utils,
bash-completion,
bc,
btrfs-progs,
cron,
curl,
dbus-user-session,
dosfstools,
dialog,
debconf-utils,
debsums,
fake-hwclock,
fastfetch|neofetch,
fdisk,
figlet,
gpg,
htop,
init,
iw,
jq,
less,
lsof,
man-db,
mmc-utils,
ncurses-term,
parted,
polkitd|policykit-1|pkexec,
psmisc,
pciutils,
rsync,
rsyslog,
squid-deb-proxy-client|auto-apt-proxy,
sudo
sudo,
systemd-resolved,
toilet,
u-boot-tools,
usbutils,
wget,
wireguard-tools,
wireless-regdb,
wpasupplicant
Suggests:
armbian-desktop,
docker.io|docker-ce|podman
Expand Down Expand Up @@ -89,6 +135,34 @@ Suggests:

#############################################################################

Package: armbian-desktop-cinnamon
Description: Install Armbian Cinnamon DE
Priority: optional
Architecture: all
Provides: armbian-desktop
Breaks: armbian-desktop
Conflicts: armbian-desktop
Depends:
armbian-desktop-common,
ubuntucinnamon-desktop-minimal|cinnamon-core,
${misc:Depends}

#############################################################################

Package: armbian-desktop-i3-wm
Description: Install Armbian I3 tiling DE
Priority: optional
Architecture: all
Provides: armbian-desktop
Breaks: armbian-desktop
Conflicts: armbian-desktop
Depends:
armbian-desktop-common,
i3-wm,
${misc:Depends}

#############################################################################

Package: armbian-desktop-kde
Description: Install Armbian KDE DE
Priority: optional
Expand Down