-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Use BSD tar's libarchive LZMA support if available #1279
Conversation
tar_flags << "z" | ||
elsif cached_location.compression_type == :bzip2 | ||
elsif type == :bzip2 | ||
tar_flags << "j" | ||
end |
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.
Do the "older versions of tar" that "require an explicit format flag" happen to be tar on 10.6 or lower? In any case, even if it's unnecessary, IMO it would be better to add a clause for :xz
:
elsif type == :xz
tar_flags << "J"
end
which is arguably safer (no one needs to ask the older versions question I raised above), and at least feels more complete.
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.
They could be, I'll double-check. Couldn't find the J
in the manpage.
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.
Yeah, I don't see it in the manpage either, but it is in the --help
string:
$ tar --help | grep '\-J'
-z, -j, -J, --lzma Compress archive with gzip/bzip2/xz/lzma
if type == :xz && MacOS.version < :lion | ||
pipe_to_tar(xzpath) | ||
else | ||
safe_system "tar", tar_flags, cached_location |
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.
What about /usr/bin/tar
? (xar
path is hard-coded too.)
tar
in pipe_to_tar
(below, outside the current set of diffs) should be hard-coded too.
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.
It should definitely be /usr/bin/tar
. We had a bug or two a while back when there were plain tar
invocations.
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 is to make Linuxbrew's life a bit easier. We haven't seen any bugs with this for a long time. Once we have environment sanitisation we can hopefully start to filter a lot more PATHs out.
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.
Care to list a Linux distro without a full blown /usr/bin/tar
(other than embedded distros, e.g. busybox
based ones liks OpenWrt)?
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.
You're proposing making a change to additional code so the onus is not on me to research why I shouldn't change the existing code. It's not a bug we're seeing and I don't think speculative fixing like that is a sensible approach as it may break things for forks like Linuxbrew and it's not gaining us anything today. Again, happy to have a longer-term PATH-filtering solution that addresses all these cases and avoids us having to hard-code anything.
Avoid using an unnecessary `xz` dependency when it's not needed.
Refactored so this works properly on non-macOS and confirmed that Lion was indeed the first version to support this. This adds additional weight to using |
Um
|
💣 #1299. |
brew tests
with your changes locally?Avoid using an unnecessary
xz
dependency when it's not needed.This may well require some refactoring and I need to confirm that these macOS versions are correct. Thanks to @zmwangx for pointing out that this was possible; I had no idea.