-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
dev-cmd/bottle: use system GNU tar if possible #18604
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me 👍🏻
I believe the reason for using Homebrew Makes sense to use system tar when bottling gnu-tar itself though. In the longer term, a Ruby implementation (e.g. |
Agreed 👍🏻 |
Sounds reasonable. I was seeing a diff when trying to bottle
GNU tar does make it easy to potentially change the compression format. Although it wasn't something worked on (#13621), there may be value in allowing different compression formats per-formula Mainly useful if noticeable improvement and if dependency is already met. For example, I was able to use
❯ brew reinstall --quiet --display-times ./llvm--19.1.2.arm64_sequoia.bottle.1.tar.zst
...
==> Summary
🍺 /opt/homebrew/Cellar/llvm/19.1.2: 8,041 files, 1.9GB
==> Running `brew cleanup llvm`...
Disable this behaviour by setting HOMEBREW_NO_INSTALL_CLEANUP.
Hide these hints with HOMEBREW_NO_ENV_HINTS (see `man brew`).
==> Installation times
llvm 6.580 s
❯ brew reinstall --quiet --display-times llvm
...
🍺 /opt/homebrew/Cellar/llvm/19.1.2: 8,041 files, 1.9GB
==> Running `brew cleanup llvm`...
Disable this behaviour by setting HOMEBREW_NO_INSTALL_CLEANUP.
Hide these hints with HOMEBREW_NO_ENV_HINTS (see `man brew`).
==> Installation times
llvm 7.522 s |
Would only want to do this if we can do so for all supported macOS versions (and typical Ubuntu runners) with no extra Homebrew or system dependencies. |
The compression algorithm is really a completely different thing. Using Ruby Gzip has also been a good improvement as we don't need to worry about the system anymore (particularly GNU Gzip's different implementation) so that does set a fairly high quality bar, whereas meanwhile we've had to adjust this |
Probably won't be possible for all formulae unless Apple decides to ship a copy. If only handling a subset of formulae, then possible if For performance, we may need to look into relocation handling first.
Letting On side note, using equivalent of |
Yes, I consider this the requirement here. |
One problem with I know we've broken reproducibility a lot in the last year, though I think it's still something we aim to keep fairly stable. On macOS, we do have options like LZFSE but I suspect it's not too big of an impact. |
Curious what Ubuntu, Fedora and Arch Linux do here given they use Though either way not possible right now in Homebrew/core. macOS tar can only decompress
Seems like it would require more effort/code to use. And attempting Anyway, will probably get back to PR soon. Still looking at best place to handle logic, i.e. whether worth moving to Linux-specific code or keeping the GNU tar check and just adding a |
Given the branched approach to many distros, they might not care too much about reproducibility between branches but do within a branch. I don't think zstd patch releases ever get backported in Ubuntu - only security update commits. Generally speaking it's usually reproducible with a fixed version (the CPU thing is seen as a bug and won't affect everyone). For Homebrew, we're rolling release so we don't really have that distinction so we break as and when it makes sense to but ultimately try to have some reproducibility. I do agree zstd is ideal, though any global dependency on a formula always has been awful (see attestations). If we go down that route it would probably have to ship with Portable Ruby like zlib already is, though that would still require code to handle system Ruby scenarios. |
We could write a pure Ruby library for Zstd. Though by the time we've done that we might lose most of the benefits of using Zstd. |
I guess in theory you only need a decompressor which is an easier port given most magic is done on the compressor side. |
tar = if system_gnu_tar? | ||
"tar" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplest way to update this PR is probably following with additional input parameter for formula
:
tar = if system_gnu_tar? | |
"tar" | |
tar = if formula.name == "gnu-tar" && system_gnu_tar? | |
"tar" |
Though this won't handle case if a user runs brew bottle gnu-tar
on an old LTS Linux. Seems like a very unlikely situation.
A more complete option would be to add OS-specific logic to only check this on Linux-only as macOS doesn't have the same problem in relation to rewritten ld.so
.
Either way, if we do want to consider pure Ruby alternative, then these changes would be a stopgap measure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to me 👍🏻
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?I switched to non-reproducible args for building
gnu-tar
bottle in #18138 due to problems bottlinggnu-tar
withgnu-tar
.Looking into this again to restore reproducibility. Incomplete PR as back-to-back bottles are still producing a diff on Linux.
The main issue for
gnu-tar
is running with placeholders, particularly for the interpreter.On Linux, there are 2 options:
gnu-tar
(unlikely) and maybe the version number difference.On macOS,
gnu-tar
has no dependencies and doesn't have interpreter handling like Linux so can just bottlegnu-tar
withgnu-tar
: