Skip to content
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.

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

prepare-host: add squid-deb-proxy-client to host dependencies #8027

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leggewie
Copy link
Collaborator

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.

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.
@leggewie leggewie requested a review from a team as a code owner March 29, 2025 11:08
Copy link
Contributor

coderabbitai bot commented Mar 29, 2025

Walkthrough

The changes modify the adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh by adding the package squid-deb-proxy-client to the list of host dependencies. This addition augments the repository of packages that are installed when the function is executed, alongside previously included dependencies like patchutils, pkg-config, and rsync. The overall logic and control flow of the function remain unchanged, and no alterations have been made to the declarations of exported or public entities.

Suggested labels

Ready to merge, 05, size/small, Framework

Suggested reviewers

  • igorpecovnik
✨ Finishing Touches
  • 📝 Docstrings were successfully generated. (🔄 Check again to generate docstrings again)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added size/small PR with less then 50 lines Needs review Seeking for review Framework Framework components labels Mar 29, 2025
@coderabbitai coderabbitai bot added 05 Milestone: Second quarter release Ready to merge Reviewed, tested and ready for merge labels Mar 29, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 entry squid-deb-proxy-client is 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.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 756dff9 and aa6a5fb.

📒 Files selected for processing (1)
  • lib/functions/host/prepare-host.sh (1 hunks)

Copy link
Member

@rpardini rpardini left a comment

Choose a reason for hiding this comment

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

Hmm. This ignores the fact we handle our own apt through chroot, and that we still have aptcacherng lying around. Sounds like a nice idea that needs a bit developing?

@rpardini
Copy link
Member

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.

coderabbitai bot added a commit that referenced this pull request Mar 29, 2025
Docstrings generation was requested by @leggewie.

* #8027 (comment)

The following files were modified:

* `lib/functions/host/prepare-host.sh`
Copy link
Contributor

coderabbitai bot commented Mar 29, 2025

Note

Generated docstrings for this pull request at #8028

@leggewie
Copy link
Collaborator Author

leggewie commented Mar 29, 2025

Hmm. This ignores the fact we handle our own apt through chroot, and that we still have aptcacherng lying around. Sounds like a nice idea that needs a bit developing?

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.

@leggewie leggewie requested a review from rpardini March 29, 2025 12:38
@rpardini
Copy link
Member

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 squid-deb-proxy-client to the prepare-host hostdeps, even it it works optimally, will bring very few benefits.

  1. It would only impact hostdep installation. (host-side)
  2. By the time it is installed (together with all other hostdeps), all other hostdeps will also already have been installed.
  3. It will not impact the usage or not of proxy for debootstrap or for apt calls inside the chroot. We btw already have local caching (using mounts) for that scenario. (It doesn't benefit from a LAN cache, but shouldn't ever download the same package/version from the same distro twice, unless you manually clean the cache).

And finally: squid-deb-proxy-client is not available in Debian Bookworm. https://packages.debian.org/search?keywords=squid-deb-proxy-client -- this is a complete deal-breaker here, even if everything else made sense (which, it doesn't).

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.

@leggewie
Copy link
Collaborator Author

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"

@leggewie
Copy link
Collaborator Author

But it's probably best to start looking for an alternative to this transparent discovery mechanism like auto-apt-proxy.

auto-apt-proxy does not detect my LAN acng instance when squid-deb-proxy-client has no issue :-(

@leggewie
Copy link
Collaborator Author

leggewie commented Mar 31, 2025

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 igorpecovnik added Work in progress Unfinished / work in progress and removed Ready to merge Reviewed, tested and ready for merge labels Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
05 Milestone: Second quarter release Framework Framework components Needs review Seeking for review size/small PR with less then 50 lines Work in progress Unfinished / work in progress
Development

Successfully merging this pull request may close these issues.

3 participants