-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
config: Print host glibc version [Linux] #3530
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
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.
When is it nil and when is it null? I'd say it'd be better to return a consistent value.
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's always null?. I was being unnecessarily paranoid.
Library/Homebrew/os.rb
Outdated
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.
Does this need to be required globally? I wish we didn't do that for os/mac
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.
No, it doesn't. I was just following the same pattern. Fixed.
Library/Homebrew/os/linux/glibc.rb
Outdated
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.
Could this just be required from system_config or even just be in that file itself if it's only used there?
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.
Yes, it can be required by system_config. Done.
This method will be used later by the bottle-pouring logic to ensure that a new enough glibc is installed to use bottles.
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.
Cool, that's fine.
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.
Is this method going to be used in other PRs? Otherwise can you just roll it into host_glibc_version with a return "N/A" if version.null?
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.
I've rolled maybe_na into host_glibc_version.
Library/Homebrew/os/linux/glibc.rb
Outdated
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.
Cool, that's fine.
|
Thanks again @sjackman! |
|
Thanks for merging, Mike! |
brew testswith your changes locally?