prepare-host: add squid-deb-proxy-client to host dependencies #8027
Conversation
squid-deb-proxy-client is a small script to transparently detect and use a caching packages proxy in the LAN like apt-cacher-ng. It is incredibly convenient and useful to cut down on unnecessary downloads and speed up the build.
WalkthroughThe changes introduce an environment variable, Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
size/small
coderabbitai
bot
added
05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/functions/host/prepare-host.sh (1)
200-200: Dependency Addition: Squid-Deb-Proxy-Client
The new package entrysquid-deb-proxy-clientis correctly added to the array of host dependencies. This change straightforwardly fulfills the PR objective of enhancing the caching mechanism for package downloads.It would be helpful to add an inline comment explaining the role of this package (e.g., "Client utility for detecting and leveraging local caching proxies") so that future maintainers understand its inclusion.
|
Guess my point is: make this an extension for now; once very well proven and the rest clean-ed up, we can consider core-ing it. |
Docstrings generation was requested by @leggewie. * #8027 (comment) The following files were modified: * `lib/functions/host/prepare-host.sh`
|
Note Generated docstrings for this pull request at #8028 |
thank you for your review. I hope I don't misunderstand you but this change does in fact not ignore the fact that the chroot/docker image handles their own apt pulls and local caching at all. Indeed, that is the very "problem" this PR tries to address. My laptop host already has squid-deb-proxy-client installed, of course. But the armbian docker build container will not benefit from the LAN acng instance, nonetheless. Yet, it should, there is no downside to it. squid-deb-proxy-client is completely transparent. if no acng instance is present in the LAN at run-time of the chroot, nothing changes. if there is, it currently is ignored, loosing the benefits from local LAN-wide caching. all this change will do is that acng instances in the LAN will automatically start working. as such, I don't think that increasing complexity through an extension and making it optional is a good idea at all. having this package installed is a sane default and does not add all that much to the footprint. |
|
I really appreciate your argument. So please don't misunderstand, caching is an area Armbian has been working on for many years, we've a long trail of tears with acng, squid, etc until we got to the point we are at, which works reasonably-ish for "most cases". (We've still a lot of cleanups to do, ref acng, dead code, etc). Adding
And finally: I must insist any changes will need to be proven in different environments (CI, build farm, local dev workstations, Docker, lxc, etc) before they land in core -- we've been burned too many times. That's why I suggested extension: it's a way you can land your change, I can get my clean core (lol.), time goes by, proof is established, and we can decide. |
|
Good point on squid-deb-proxy-client being dropped from unstable because squid-deb-proxy was in such bad shape. Ubuntu has reintroduced it and I kept it around. But it's probably best to start looking for an alternative to this transparent discovery mechanism like auto-apt-proxy. Too bad, the way things are currently set up in armbuild/build don't allow an easy way to set up an alternative dependency like I can do with a real package at "Depends: auto-apt-proxy | squid-deb-proxy-client" |
auto-apt-proxy does not detect my LAN acng instance when squid-deb-proxy-client has no issue :-( |
|
good point also on the caching not propagating further down into the chroot. I do have a hook for pbuilder to do this when building debian packages, so I guess, we might want to try and do this for armbian, too. |
igorpecovnik
added
Work in progress
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
extensions/apa.sh (1)
5-5: Document the purpose of APA_IS_ACTIVE
Add a comment explaining that this flag is consumed increate_sources_list_and_deploy_repo_keyto skip key deployment, improving clarity for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
extensions/apa.sh(1 hunks)lib/functions/rootfs/distro-specific.sh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Shell script analysis
| @@ -80,6 +80,8 @@ function create_sources_list_and_deploy_repo_key() { | |||
|
|
|||
| declare distro="" | |||
|
|
|||
| APT_SIGNING_KEY_FILE="/usr/share/keyrings/armbian-archive-keyring.gpg" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Scope and freeze the signing key file variable
The variable APT_SIGNING_KEY_FILE is set without scope or protection. Make it readonly within the function for safety:
- APT_SIGNING_KEY_FILE="/usr/share/keyrings/armbian-archive-keyring.gpg"
+ declare -r APT_SIGNING_KEY_FILE="/usr/share/keyrings/armbian-archive-keyring.gpg"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| APT_SIGNING_KEY_FILE="/usr/share/keyrings/armbian-archive-keyring.gpg" | |
| declare -r APT_SIGNING_KEY_FILE="/usr/share/keyrings/armbian-archive-keyring.gpg" |
🤖 Prompt for AI Agents
In lib/functions/rootfs/distro-specific.sh at line 83, the variable
APT_SIGNING_KEY_FILE is assigned without scope or protection. To fix this,
declare the variable as readonly within the function by using the readonly
keyword before or after the assignment to prevent it from being modified later.
| # code to be made obsolete by making APA part of Armbian Core. #XXX | ||
| if [ ! $APA_IS_ACTIVE = "true" ]; then | ||
| # add armbian key | ||
| display_alert "Adding Armbian repository and authentication key" "${when} :: /etc/apt/sources.list.d/armbian.sources" "info" | ||
| mkdir -p "${basedir}"/usr/share/keyrings | ||
| # change to binary form | ||
| gpg --dearmor < "${SRC}"/config/armbian.key > "${basedir}${APT_SIGNING_KEY_FILE}" | ||
|
|
||
| # lets link to the old file as armbian-config uses it and we can't set there to new file | ||
| # we user force linking as some old caches still exists | ||
| chroot "${basedir}" /bin/bash -c "ln -fs armbian-archive-keyring.gpg /usr/share/keyrings/armbian.gpg" | ||
|
|
||
| # lets keep old way for old distributions | ||
| if [[ "${RELEASE}" =~ (focal|bullseye) ]]; then | ||
| cp "${SRC}"/config/armbian.key "${basedir}" | ||
| chroot "${basedir}" /bin/bash -c "cat armbian.key | apt-key add - > /dev/null 2>&1" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Fix unquoted conditional and symlink path
The test if [ ! $APA_IS_ACTIVE = "true" ]; then is unquoted and brittle when the variable is unset. Also, the symlink invocation uses a relative target. Apply this diff:
-if [ ! $APA_IS_ACTIVE = "true" ]; then
+if [[ "${APA_IS_ACTIVE:-}" != "true" ]]; then
display_alert "Adding Armbian repository and authentication key" "${when} :: /etc/apt/sources.list.d/armbian.sources" "info"
mkdir -p "${basedir}/usr/share/keyrings"
- gpg --dearmor < "${SRC}"/config/armbian.key > "${basedir}${APT_SIGNING_KEY_FILE}"
- chroot "${basedir}" /bin/bash -c "ln -fs armbian-archive-keyring.gpg /usr/share/keyrings/armbian.gpg"
+ gpg --dearmor < "${SRC}/config/armbian.key" > "${basedir}${APT_SIGNING_KEY_FILE}"
+ chroot "${basedir}" ln -fs "${APT_SIGNING_KEY_FILE}" "/usr/share/keyrings/armbian.gpg"This makes the conditional robust and ensures the symlink points to the correct absolute path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # code to be made obsolete by making APA part of Armbian Core. #XXX | |
| if [ ! $APA_IS_ACTIVE = "true" ]; then | |
| # add armbian key | |
| display_alert "Adding Armbian repository and authentication key" "${when} :: /etc/apt/sources.list.d/armbian.sources" "info" | |
| mkdir -p "${basedir}"/usr/share/keyrings | |
| # change to binary form | |
| gpg --dearmor < "${SRC}"/config/armbian.key > "${basedir}${APT_SIGNING_KEY_FILE}" | |
| # lets link to the old file as armbian-config uses it and we can't set there to new file | |
| # we user force linking as some old caches still exists | |
| chroot "${basedir}" /bin/bash -c "ln -fs armbian-archive-keyring.gpg /usr/share/keyrings/armbian.gpg" | |
| # lets keep old way for old distributions | |
| if [[ "${RELEASE}" =~ (focal|bullseye) ]]; then | |
| cp "${SRC}"/config/armbian.key "${basedir}" | |
| chroot "${basedir}" /bin/bash -c "cat armbian.key | apt-key add - > /dev/null 2>&1" | |
| fi | |
| fi | |
| # code to be made obsolete by making APA part of Armbian Core. #XXX | |
| if [[ "${APA_IS_ACTIVE:-}" != "true" ]]; then | |
| # add armbian key | |
| display_alert "Adding Armbian repository and authentication key" "${when} :: /etc/apt/sources.list.d/armbian.sources" "info" | |
| mkdir -p "${basedir}/usr/share/keyrings" | |
| # change to binary form | |
| gpg --dearmor < "${SRC}/config/armbian.key" > "${basedir}${APT_SIGNING_KEY_FILE}" | |
| chroot "${basedir}" ln -fs "${APT_SIGNING_KEY_FILE}" "/usr/share/keyrings/armbian.gpg" | |
| # lets keep old way for old distributions | |
| if [[ "${RELEASE}" =~ (focal|bullseye) ]]; then | |
| cp "${SRC}"/config/armbian.key "${basedir}" | |
| chroot "${basedir}" /bin/bash -c "cat armbian.key | apt-key add - > /dev/null 2>&1" | |
| fi | |
| fi |
🤖 Prompt for AI Agents
In lib/functions/rootfs/distro-specific.sh around lines 162 to 179, the
conditional test for APA_IS_ACTIVE is unquoted, which can cause errors if the
variable is unset, and the symlink command uses a relative path that may be
incorrect. Fix this by quoting the variable in the if test to ensure it is
safely evaluated even if unset, and change the symlink target to an absolute
path to guarantee it points correctly within the chroot environment.
squid-deb-proxy-client is a small script to transparently detect and use a caching packages proxy in the LAN like apt-cacher-ng. It is incredibly convenient and useful to cut down on unnecessary downloads and speed up the build.