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
unpack_strategy/zip: allow unzip formula to be used #12947
Conversation
Review period skipped due to |
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.
Some comments, not blocking, nice work!
unzip = begin | ||
Formula["unzip"] | ||
rescue FormulaUnavailableError | ||
nil |
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 happens if there's no system unzip
and this is nil
?
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.
PATH.new
ignores nil so it'll just use ENV["PATH"]
.
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.
Oh misread your question. The command will fail to execute if there's no system unzip and this is nil (as it did before - so no change here). But this really should only be nil if homebrew-core is not installed, and I imagine there are many other issues in that case. All the other unpack strategies will actually error since they don't have a FormulaUnavailableError
check (we do here because it's needed for the rspec tests).
If there's a different way we want to handle this, I think it should be done in a change applying to all unpack strategies, for consistency.
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.
Gotcha, that all seems fine to me then 👍🏻
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This is necessary for some Linux systems where there is no system unzip.