iron out bugs in APA #25
Conversation
github-actions
bot
added
11
WalkthroughThis change updates debian/control only: it expands armbian-common Depends and Recommends/Suggests with many runtime utilities and packages (e.g., e2fsprogs, initramfs-tools, iproute2, apt-utils, locales, gnupg2, ca-certificates, tzdata, systemd-resolved, toilet, and many others), and adds three new desktop meta-packages — armbian-desktop-cinnamon, armbian-desktop-i3-wm, and armbian-desktop-kde. Each new package Provides/Breaks/Conflicts armbian-desktop and Depends on armbian-desktop-common plus DE-specific packages. The KDE stanza’s Recommends no longer includes thunar. Existing desktop blocks such as armbian-desktop-gnome are retained. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
tabrisnet
added
02
![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
🧹 Nitpick comments (3)
debian/control (3)
21-37: Duplicate dependency:console-setupappears in botharmbian-commonandarmbian-cli.Since
armbian-clilikely depends on or is installed alongsidearmbian-common, havingconsole-setupin both is redundant. Consider removing it fromarmbian-cli(line 72) since it's already pulled in byarmbian-common.
67-67: ConsiderArchitecture: allfor this meta-package.Meta-packages that only declare dependencies without containing architecture-specific binaries should use
Architecture: all. This is consistent witharmbian-desktop-gnome,armbian-desktop-kde, and other meta-packages in this file.-Architecture: any +Architecture: all
69-101: Remove dependencies already provided byarmbian-common.Several packages in
armbian-cliDepends are already inarmbian-commonDepends:
console-setup(line 72) — duplicates line 34nano(line 91) — duplicates line 27 (nano|vim)openssh-server(line 92) — duplicates line 29Since
armbian-commonisEssential: yesandPriority: required, it will always be installed. These duplicates add noise without benefit.Depends: bash-completion, bc, - console-setup, cron, curl, dbus-user-session, dialog, debconf-utils, debsums, fake-hwclock, figlet, htop, iputils-ping, init, iw, jq, less, lsof, man-db, mmc-utils, ncurses-term, - nano, - openssh-server, parted, psmisc, rsync, rsyslog, sudo, toilet, u-boot-tools, usbutils, wget
📜 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(4 hunks)
🔇 Additional comments (2)
debian/control (2)
46-48: LGTM!The Suggests field now correctly offers
armbian-desktoporarmbian-clias alternatives, aligning with the new CLI package introduction.
115-126: AI summary inconsistency:kde-plasma-desktopis NOT added toarmbian-desktop-gnome.The AI summary incorrectly states that
kde-plasma-desktopwas added toarmbian-desktop-gnomeDepends. The actual code correctly keeps GNOME dependencies separate from KDE. No action needed on the code itself.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
debian/control (1)
66-66: Remove redundant Essential field.The
Essential: nodeclaration is redundant sincenois the default value for the Essential field in Debian package control files.Apply this diff to remove the redundant line:
Description: basic commandline environment Priority: optional -Essential: no Architecture: any
📜 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(4 hunks)
🔇 Additional comments (7)
debian/control (7)
28-28: Resolve the FIXME comment about networkd.The FIXME comment suggests uncertainty about whether networkd should be used instead of or in addition to netplan.io|network-manager. This decision should be resolved before merging.
Could you clarify the network management strategy for Armbian? Should systemd-networkd be an alternative option here, or is the current netplan.io|network-manager choice correct?
47-47: Good addition of armbian-cli as an alternative suggestion.This change appropriately reflects the new armbian-cli package as an alternative to armbian-desktop in the Suggests field.
69-111: Good package organization with Depends and Recommends.The armbian-cli package is well-structured with:
- Essential CLI tools in Depends
- Optional but commonly wanted packages (alsa-utils, wpasupplicant, etc.) in Recommends
This aligns with the PR objectives where the author notes that many boards may not need these packages but prefers to include them by default for users who do want them.
162-166: Indentation fixed correctly.The indentation issue from the previous review has been addressed. The Depends section now uses consistent single-space indentation for continuation lines, matching the file's conventions.
124-126: Flag AI summary inconsistency.The AI-generated summary incorrectly states that armbian-desktop-gnome was updated to depend on kde-plasma-desktop. The annotated code shows armbian-desktop-gnome's Depends section (lines 124-126) contains only
armbian-desktop-commonand${misc:Depends}, with no kde-plasma-desktop dependency.Additionally, the AI summary mentions updates to a "Package: armbian-desktop" that doesn't exist in this file. The actual packages are armbian-desktop-gnome, armbian-desktop-kde, armbian-desktop-xfce, etc., which Provide the virtual package armbian-desktop but are not themselves named armbian-desktop.
166-166: The review comment's recommendation is technically sound and should be applied.Based on my verification:
- Confirmed: Dolphin is KDE Plasma's native file manager, while Thunar is XFCE's native file manager
- Confirmed: In Debian Trixie,
kde-plasma-desktopdoes not include Dolphin as a hard dependency—it is a separate package (dolphin 4:25.04.3-1) that users must install independently- Implication: Since
kde-plasma-desktopdoesn't provide a default file manager, recommending Dolphin (KDE's native option) is more appropriate than recommending Thunar (XFCE's option)The suggested diff to replace
Recommends: thunarwithRecommends: dolphinis appropriate for any KDE-focused package that needs a file manager recommendation. The alternative suggestion to omit the recommendation entirely is less practical given that kde-plasma-desktop itself doesn't provide one.-Recommends: thunar +Recommends: dolphin
67-67: Verify Architecture field for metapackage — Architecture: all is standard for pure metapackages.Based on Debian packaging best practices, metapackages that contain only dependency metadata (Depends/Recommends/Conflicts) and no architecture-specific files or scripts should use
Architecture: all. UsingArchitecture: anycreates unnecessary separate binary packages for each architecture, which is not appropriate for a pure metapackage that has no architecture-dependent content or build behavior.The dependencies listed (bash-completion, bc, console-setup, cron, curl, dbus-user-session, dialog, etc.) appear to be standard, architecture-independent packages. Unless armbian-cli includes architecture-specific maintainer scripts, compiled binaries, or has dependencies that are only available on certain architectures,
Architecture: allis the correct choice per Debian Policy.
tabrisnet
force-pushed
the
tabrisnet_cli
branch
from
ff18aac to
20edfc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
debian/control (2)
160-163: KDE desktop Depends refactor is correct; consider Thunar recommendationThe multi-line
Dependsforarmbian-desktop-kde(armbian-desktop-common,${misc:Depends},kde-plasma-desktop) now matches the formatting used elsewhere and is semantically fine.Minor nit:
Recommends: thunarunder a KDE meta-package is a bit surprising (Dolphin is more typical), so double‑check that this is intentional and not a leftover from XFCE.
225-226: Desktop-common now stacks on armbian-common; consider also pulling in armbian-cliHaving
armbian-desktop-commonDepends: armbian-commonis a good layering change so all desktops share the same minimal base.Design question: if you intend every desktop profile to also have the CLI baseline (htop, man-db, rsync, etc.), you might want
armbian-desktop-commonto depend on (or at least recommend)armbian-cliinstead of each DE meta maintaining its own CLI-ish list. If desktops are meant to be independent ofarmbian-cli, this is fine as-is.
📜 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(5 hunks)
🔇 Additional comments (1)
debian/control (1)
63-112: Newarmbian-clistanza and Recommends block look consistentThe
armbian-clipackage definition is syntactically correct and layered cleanly on top ofarmbian-common. TheDependslist matches the described “basic commandline environment”, and theRecommendsblock (with the explanatory# allow these to be uninstalled…comment on its own line) is valid Debian control syntax.No changes needed here from a packaging standpoint.
tabrisnet
force-pushed
the
tabrisnet_cli
branch
from
3242b2e to
91317fe
Compare
tabrisnet
force-pushed
the
tabrisnet_cli
branch
from
5e70adb to
fbc24ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
debian/control (2)
66-66: RemoveEssential: no– this field is only valid when set toyes.The
Essentialfield should only be present when a package is essential (i.e.,Essential: yes). Omitting the field is the standard way to indicate a package is not essential. HavingEssential: nois non-standard and may trigger lintian warnings.Package: armbian-cli Description: basic commandline environment Priority: optional -Essential: no Architecture: all
167-178: LGTM – minimal i3 package.The package is intentionally minimal. Consider adding a
Recommendsfor common i3 accessories:Recommends: dmenu|rofi, i3lock, i3status|polybarThis keeps the base install minimal while suggesting useful additions.
📜 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(5 hunks)
🔇 Additional comments (8)
debian/control (8)
52-59: Acknowledge ongoing architectural discussion regardingEssential: yesandlinux-imageplacement.Per the previous discussion, marking
armbian-bspasEssential: yeswhile having it depend onlinux-imagecan interfere with the rootfs caching strategy in the build system. The author has acknowledged this needs coordination witharmbian/build.For now, this structure will work, but consider the previously suggested approach of:
- Removing
Essential: yesfromarmbian-bsp- Ensuring the build system explicitly installs BSP/kernel packages after cache extraction
69-99: LGTM – sensible CLI utility selection.The dependency list covers essential CLI tools for system administration: networking (
curl,wget,iw), system monitoring (htop,lsof,psmisc), storage (parted,mmc-utils), and general utilities (jq,rsync,sudo). Good separation fromarmbian-common.
113-149: No new issues in the GNOME desktop package changes.The structure with
Provides: armbian-desktop,Breaks/Conflicts, and dependency onarmbian-desktop-commonfollows the pattern established for desktop variants.
153-164: LGTM – Cinnamon desktop package follows established pattern.The alternative dependency
ubuntucinnamon-desktop-minimal|cinnamon-coreprovides compatibility across Ubuntu and Debian.
188-191: LGTM – KDE package dependencies updated correctly.The multi-line format is now consistent with other stanzas, and
kde-plasma-desktopprovides a minimal KDE installation as described in the PR objectives.
252-257: LGTM –armbian-desktop-commonnow properly chains toarmbian-common.This establishes the correct dependency hierarchy: desktop variants →
armbian-desktop-common→armbian-common, ensuring all desktop installations include the base system packages.
68-68: Ensure comment line starts at column 1 (no leading whitespace).In debian/control, comments must begin at column 1. If the
#character is indented, the line becomes part of theDependsfield value and breaks package parsing. Verify line 68 has no leading whitespace.
100-109: No leading whitespace issue detected at line 101.The comment
# allow these to be uninstalled on systems that don't need/want them.starts at column 1 without leading whitespace, which is the correct format for debian/control. The subsequent package lines are properly indented with leading spaces as continuation lines.
tabrisnet
changed the title
There was a problem hiding this comment.
Hello tabrisnet,
thank you for your suggestions. I will cherry-pick the commits that can go in unchanged and we can then discuss the other ones.
armbian-cli package is not necessary. As it says in the README, "Packages from Depends of armbian-common define Armbian minimal images (network, sshd and succesful boot). Adding the Recommends defines the CLI images."
You dropped a couple of lines and in most if not all cases, I don't think that was correct.
I tend to add $(misc:Depends) as the very last line in Depends.
Let's not add packages (unless we have to) that are installed by default via debootstrap.
nitpick: if you can, try to follow the style in git commit messages, most notably in designating as the first word what area of APA you are touching.
If at all possible, add to Recommends instead of Depends. Depends means that a user who does not want to have a package listed in Depends installed on their machine has to uninstall the APA package. For example, I do not use vim and I am not happy it is a hard Depends in many of the Ubuntu packages with no alternatives allowed.
Many firmware engineers are also using it on fully featured Ubuntu desktop. I even experienced this kind of situation. A guy sitting next to me for years, while using Code most of the time, to deal with the code ... he almost always used VIM to deal with quick config edits. When you have 10+ terminal windows and you don't know which are local and you don't have time to deal with banality ... you know VIM is always there. Also on the embedded machine. Perhaps reasoning is different, but anyhow, IHMO VIM is must have on Linux systems. Also it cost close to nothing. |
Sure, I understand. And I am not saying it shouldn't be installed. My issue with the Ubuntu meta packages was that if for whatever reason you did NOT want it installed, it forced you to remove ubuntu-minimal. That is over the top. I never use vim and ubuntu-minimal works just fine without it. So, in MY packages, vim is either "Depends: vim|nano|$bunchofeditors" or in Recommends. |
Yeah, that is weird / wrong. |
|
I thought your addition of the cinnamon and i3 packages was uncontroversial and I was going to cherry-pick that commit. But then I paused. Those packages currently do not add anything IMO and those two DE are not officially supported by Armbian, no images for them are currently being built as far as I am aware. The meta packages just depend on another meta package already in Ubuntu and then our armbian-desktop-common. I think we need more justification and more actual functionality before we add those meta packages to APA. |
|
XFCE, Gnome, Cinnamon and perhaps KDE has to be done well. The rest optionally / low priority / later ... never. |
My suggestion is to focus on the main three DE: KDE, Gnome and XFCE. Certainly for now. And let the user install another DE if that is important to them. Let's stay focused. Alternatively, I am happy to tweak armbian-desktop-generic a bit to get some integration with armbian by adding something like "Recommends: ubuntucinnamon-desktop-minimal|cinnamon-core|i3-wm|BUDGIE" to its stanza. |
That's definitely wrong. APA is not just about image creation, but about the entire life cycle. armbian-common should keep the armbian-bsp package installed. I should be able to mark it as automatically installed without risking it to be removed (resulting in an unbootable system). Ideally, I want my systems to have a very, very short output to |
|
please explain why iputils-ping needs to be in armbian-common Depends? Only thing I could find was line 137 in lib/functions/rootfs/distro-agnostic.sh |
exactly that. without the pkg, the build process bails out. |
I was skeptical for sure, but the current problem is this: |
Colour me very surprised when Otherwise, can you be more specific? |
tabrisnet
force-pushed
the
tabrisnet_cli
branch
from
8f29d06 to
5b7d1fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
debian/control (1)
87-94: Consider droppingEssential: yesfromarmbian-bspnow thatlinux-imagemoved hereMoving
linux-imageintoarmbian-bspnicely addresses the “don’t put a kernel/initrd into the cached rootfs” concern. However, keepingarmbian-bspmarkedEssential: yesis atypical for a board-specific package and makes it effectively irremovable/irreplaceable once installed. Given the build system already installs BSP/kernel explicitly at the board stage, you likely don’t needEssential: yeshere and can rely on explicit installation + normal Depends.A minimal change would be:
Package: armbian-bsp Description: board-specific hardware support Priority: required -Essential: yes Architecture: any Depends: linux-image, ${misc:Depends}This keeps the dependency structure but relaxes the Essential semantics. (Same direction as earlier discussion; repeating here against the current stanza.)
🧹 Nitpick comments (2)
debian/control (2)
138-148: Cinnamon metapackage wiring looks correct; verify target suite coverage for the alternativesThe new
armbian-desktop-cinnamonstanza structurally matches the other DE metas (Provides/Breaks/Conflictsarmbian-desktop, depends onarmbian-desktop-common, clean dependency list). Theubuntucinnamon-desktop-minimal|cinnamon-corealternative pair is a reasonable way to cover Ubuntu/Debian, but it’d be good to double‑check both names exist across all suites you intend APA to support so installs don’t fail unexpectedly on some targets. You can always add Cinnamon‑specific Recommends later once you converge on a minimal app set.
152-162: i3 metapackage is very minimal; consider whether it should pull a usable desktop stack
armbian-desktop-i3-wmcurrently only guaranteesarmbian-desktop-common+i3-wm. For a package described as “Install Armbian I3 tiling DE”, that may surprise users if no display manager/X stack is pulled in and they end up with just the window manager on top of whatever is already there. If you want this to behave more like a “desktop task”, you might eventually add Recommends for at least a DM and the basic i3 companions (e.g.i3status/i3blocks, a terminal, etc.), following the existing pattern you’re using for GNOME/XFCE. Fine to defer if the current intent is just to register i3 as an experimental DE skeleton.
📜 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(2 hunks)
🔇 Additional comments (1)
debian/control (1)
20-36: Base dependency expansion looks sound; confirm this “minimal set” is intentionalThe added Depends/Recommends for
armbian-common(filesystem tools, initramfs, networking, logging, apt/locales/crypto/time, audio, btrfs, mmc-utils, wireless stack, etc.) are syntactically correct and align with the goal of having a bootable, networked, reasonably functional default system. Note that becausearmbian-commonisEssential: yes, everything in Depends is effectively part of the non-removable base from a packaging perspective; if there are any candidates here that you’d prefer to be truly optional, they probably belong in Recommends instead. From a packaging/syntax standpoint I don’t see blockers.Also applies to: 39-45, 47-51, 53-54, 56-61, 63-65, 67-67, 69-70, 72-80
|
3242b2e) the thunar change used to be a separate commit. Odd / Interesting that you then merged it with other changes later on. |
|
8e03b56 pushed, thank you That Recommends was really just a placeholder. There's probably more like that in APA. |
…an/build's CLI config
tabrisnet
force-pushed
the
tabrisnet_cli
branch
from
5b7d1fc to
6789807
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
debian/control (2)
57-72: Consider movinginitandsudoto Depends.
init(line 57) provides the init system which is fundamental for booting. While debootstrap typically installs an init system, having it inRecommendsmeans it could be skipped with--no-install-recommends, potentially leaving a non-bootable system.Similarly,
sudo(line 72) is commonly expected on interactive systems and the SSH access described in the package description typically relies on it.Depends: e2fsprogs, initramfs-tools, + init, iproute2, ... openssh-server, pastebinit, + sudo, apt-utils,Then remove them from
Recommends.
152-162: Consider addingRecommendsfor i3 ecosystem packages.The i3-wm package provides only the window manager. For a usable i3 desktop, users typically need additional components. Consider adding a
Recommendssection:Depends: armbian-desktop-common, i3-wm, ${misc:Depends} +Recommends: + dmenu|rofi, + i3status|polybar, + i3lock, + feh|nitrogenThese provide an application launcher, status bar, screen locker, and wallpaper setter—common components for an i3 setup. The alternatives syntax allows flexibility between minimal and feature-rich options.
📜 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(2 hunks)
🔇 Additional comments (4)
debian/control (4)
20-35: Reasonable additions for a minimal bootable system.The new dependencies (
e2fsprogs,initramfs-tools,iproute2,iputils-ping,logrotate,linux-base,apt-utils,locales,console-setup,gnupg2,ca-certificates,tzdata) are appropriate for ensuring a bootable system with network access, package management, and proper localization.
87-94: Architectural issue:linux-imageinarmbian-bspwithEssential: yesstill present.Per the extensive discussion in past comments, this combination causes
linux-imageand the generated initrd to be placed in the rootfs cache during debootstrap, which should be deferred to the per-board stage. The proposed solutions included:
- Remove
Essential: yesfromarmbian-bsp- Move
linux-imagedependency here (already done) but ensure build system doesn't install this package during rootfs caching phaseThis requires coordination with the
armbian/buildrepository as noted by @tabrisnet.Is there a tracking issue for this cross-repo coordination? If not, consider opening one to ensure this architectural concern is addressed before the 2026.5 goal mentioned in the PR discussion.
166-176: LGTM: Thunar removal is appropriate.Removing
Recommends: thunarmakes sense since KDE Plasma includes Dolphin as its native file manager. The minimalkde-plasma-desktopdependency aligns with the Debian Trixie approach mentioned in the PR objectives.
138-148: Package dependencyubuntucinnamon-desktop-minimalis correctly specified.The Cinnamon package dependency on
ubuntucinnamon-desktop-minimal|cinnamon-coreis appropriate, asubuntucinnamon-desktop-minimalis available in Ubuntu Noble's universe repository for both amd64 and arm64 architectures, with a fallback tocinnamon-corefor Debian-only systems.
tabrisnet
force-pushed
the
tabrisnet_cli
branch
2 times, most recently
from
41224ed to
a719cc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
debian/control (1)
19-36: Re‑check which of the newarmbian-commonDepends must be hard vs RecommendsThe added core tools (e2fsprogs, initramfs-tools, iproute2, iputils-ping, linux-base, locales, gnupg2, ca-certificates, tzdata, etc.) make sense for a usable base. The only ones I’d consider candidates for Recommends instead of Depends, if you still want to keep the rootfs as lean as possible, are things like
logrotate,apt-utils, and possiblyconsole-setup(especially for headless/serial-only boards).If your goal is “boots, has network, can apt/ssh in” rather than “fully provisioned CLI environment”, it might be worth double-checking whether those three truly need to be hard requirements or could be softened to Recommends in a follow-up.
📜 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(2 hunks)
🔇 Additional comments (1)
debian/control (1)
138-148: New Cinnamon/i3 desktop metapackages look structurally sound; just ensure coordination with build supportBoth new stanzas are syntactically and semantically sane:
armbian-desktop-cinnamonpulls inarmbian-desktop-commonplusubuntucinnamon-desktop-minimal|cinnamon-core, which gives a good cross‑Debian/Ubuntu story.armbian-desktop-i3-wmis a thin wrapper aroundarmbian-desktop-commonandi3-wm.- The Provides/Breaks/Conflicts pattern for
armbian-desktopmatches the existing GNOME/KDE/XFCE/generic metapackages.Given earlier discussion that Cinnamon and i3 aren’t yet fully supported with official images, I’d just suggest double‑checking that:
- armbian/build’s aggregation/hooks don’t accidentally start treating these as first‑class DEs before you’re ready, and
- their “preview/unsupported” status is documented somewhere user‑visible (e.g., docs or release notes), rather than encoded via stricter Depends here.
No changes needed in this file for that; it’s more about cross‑repo coordination.
Also applies to: 152-162
the proper fix is to drop those lines not add unnecessary dependencies in APA -> armbian/build#9033 I hope you can see how mixing a bunch of unrelated topics in a single PR made this more complicated than necessary. There's a lot of good stuff in here but simply too much for a single PR. |
…. the rabbit is concerned too
tabrisnet
force-pushed
the
tabrisnet_cli
branch
from
a719cc2 to
9e725be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
debian/control (2)
26-26: Consider reordering the editor alternative or moving to Recommends.The current
nano|vimsyntax makes nano the preferred editor. According to the PR discussion, one reviewer (igorpecovnik) views vim as essential and small, while another (leggewie) prefers either reordering tovim|nano|...(to make vim preferred) or moving editors to Recommends to avoid forcing a specific editor choice.Moving to Recommends would give users more flexibility while still installing an editor by default for most users.
</review_comment_end>
138-163: Reconsider the timing of adding Cinnamon and i3-wm meta-packages.The package definitions are technically correct and follow the established pattern for desktop meta-packages. However, according to the PR discussion, there are strategic concerns:
- No official support: These desktop environments are not officially supported by Armbian and no images are currently being built for them
- Limited immediate value: Adding meta-packages before establishing the infrastructure to build/test/support them may create maintenance overhead
- Alternative approach: The discussion suggested having
armbian-desktop-genericrecommend these DEs (e.g.,ubuntucinnamon-desktop-minimal|cinnamon-core|i3-wm|...) rather than creating separate meta-packages immediately- Coordination timeline: The PR discussion mentions broader coordination with a 2026.5 timeline for resolving desktop environment declarations
The PR objectives note that leggewie recommends focusing on KDE, GNOME, and XFCE first (which are actively supported), then expanding to additional DEs once the support infrastructure is in place.
Consider deferring these packages to a future PR aligned with the broader desktop environment coordination effort.
</review_comment_end>
📜 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(2 hunks)
🔇 Additional comments (4)
debian/control (4)
37-80: Recommends expansion looks well-structured.The expanded Recommends list provides a comprehensive set of system utilities (alsa-utils, bash-completion, btrfs-progs, wpasupplicant, systemd-resolved, etc.) that will be installed by default for most users while remaining optional for minimal/specialized boards. The "installed by default but easy to drop" strategy mentioned in the PR discussion is appropriately implemented here.
</review_comment_end>
166-177: Appropriate removal of thunar from KDE recommendations.Removing the
thunarrecommendation from the KDE desktop package is sensible, as Dolphin (KDE's native file manager) is included inkde-plasma-desktop. The thunar file manager is more appropriate for XFCE-based desktops.</review_comment_end>
81-83: Suggests field is correctly structured.The Suggests field correctly lists armbian-desktop and container runtime alternatives (docker.io|docker-ce|podman) with proper comma separation. These are appropriate optional additions for users who want a desktop environment or containerization capabilities.
</review_comment_end>
87-94: Cannot verify review comment without access to referenced context.The comment makes specific architectural claims about Essential:yes and rootfs caching that require examining past review discussions, the actual debian/control file, and armbian/build scripts. These cannot be accessed. The review relies heavily on "past review comments" and "past discussion" that are not provided in the verification context, making it impossible to confirm whether the architectural concerns are valid or whether the proposed fix is appropriate.
| e2fsprogs, | ||
| initramfs-tools, | ||
| iproute2, | ||
| iputils-ping, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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:
iputils-pingis a legitimate runtime dependency for the final system- The issue is about
/bin/pingneeding proper capabilities to create raw sockets in the built product - The proper solution is the three-part approach you described: PR armbian-common: add libcap2-bin for setcap #27, lib / distro-agnostic.sh: drop chmod on /bin/ping build#9033, and adding
libcap2-binto the debootstrap package list - This ensures that
iputils-ping's postinst script can usesetcapto properly configure/bin/pingwith the necessary capabilities
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@tabrisnet, thank you for the additional clarification! I apologize for continuing to misunderstand.
I now have the correct understanding:
iputils-pingis currently inDependsas a temporary workaround for thechmod u+sissue- Once the proper solution is complete (PR armbian-common: add libcap2-bin for setcap #27 + lib / distro-agnostic.sh: drop chmod on /bin/ping build#9033 + adding
libcap2-binto debootstrap),iputils-pingcan and should be removed fromarmbian-common'sDepends - The proper solution eliminates the need for this dependency by ensuring
setcapis available during the build so thatiputils-ping's postinst can properly configure capabilities
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.
There was a problem hiding this comment.
iputils-pingis a legitimate runtime dependency for the final system
no, actually it is not
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
Odd that you say that, since from where I stand, it's exactly for that reason that it is a hard Depends. To make sure that the system boots properly. Remember the definition of the minimal image? But I understand now that this might get in the way of a board-agnostic rootfs. This will need to be looked into. I shall open a new ticket for that. Frankly, this PR was trying to address too many issues at the same time. Some code was merged, some problems carved out. And then some other things were piled on top in the process. I feel like this is now unmanageable, what was cherry-pickable has been cherry-picked. To move forward, I suggest we move over to tickets, describe problems there and then discuss potential solutions instead of going backwards starting with a proposed solution. |
|
with regards to 86f4f2c I am hesitant to add so many packages. I understand you want to get them pulled in during the build and that goal has merit. My aim is to boot the board and allow a remote login via ssh and not much else for the minimal image so, adding those packages should be discussed, maybe in chat or in a ticket |
I don't care what package it goes in [I started with |
3 participants
armbian-common: copy some deps/recs/suggests from armbian/build's CLI configsarmbian-desktop-cinnamonandarmbian-common-i3-wmi3-wmis part of armbian/build, but there are reasons to consider abandoning it, leave it to the users to do themselves. To do this, remove it fromconfig/desktop/*/environments/first.armbian-bspshouldn't be required byarmbian-commonas it contains hardware-specific componentsiputils-pingis a build dependency, but only for the existence, not use, of/bin/pingarmbian/buildside first.re
Recommends, many boards don't need/wantwpasupplicant,alsa,btrfs-progsetc. but we want to install them by default b/c many ppl do want them.re
kde: https://packages.debian.org/trixie/kde-plasma-desktop says its the minimal set of applications. I am taking them at their word.Yes I'm a KDE user, but I haven't put enough thought as to a minimal set.
Do the same kind of dependency for
cinnamon&i3-wm.Also note the change around
linux-image, this needs to never be installed in therootfs.