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

brew.sh: handle Linux systems with no 'locale' #8324

Merged
merged 4 commits into from
Aug 17, 2020

Conversation

maxim-belkin
Copy link
Member

such as Alpine Linux

Closes Linuxbrew/docker#83

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

@maxim-belkin maxim-belkin requested a review from sjackman August 12, 2020 21:14
@maxim-belkin maxim-belkin self-assigned this Aug 12, 2020
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Copy link
Member

@sjackman sjackman left a comment

Choose a reason for hiding this comment

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

Use command -v rather than which.

Co-authored-by: Shaun Jackman <sjackman@gmail.com>
@maxim-belkin maxim-belkin requested a review from sjackman August 13, 2020 12:02
@maxim-belkin
Copy link
Member Author

Let me know which commit to keep:

  • 9abe3ee: suppresses error stream but should have the same effect,
  • 376227b: checks for locale explicitly.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

👏🏻 Easy! Thanks again!

then
export LC_ALL=C
elif [[ "$(locale charmap 2>/dev/null)" != "UTF-8" ]]
if [[ "$(locale charmap 2>/dev/null)" != "UTF-8" ]]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of suppressing all error messages from locale in this way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you suggest removing 2>/dev/null here?

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why @sjackman? When we're just using the output to decide on a variable and we have an else: what's the upside of showing errors here?

Copy link
Member

@sjackman sjackman Aug 13, 2020

Choose a reason for hiding this comment

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

For troubleshooting. We do not expect locale to fail, and if it were to fail for some unexpected reason, and the user were to report an issue, we would have no basis on which to troubleshoot that failure, because stderr was unconditionally suppressed, which is the shell equivalent of catching all exceptions and ignoring them.

Copy link
Member

Choose a reason for hiding this comment

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

We're not checking the exit code of locale as-is just checking (twice) if the output contains a string. It doesn't matter to us whether the output doesn't contain that string due to a failure or due to the locale being missing. Considering how we've been printing these warnings at users in the past and causing unnecessary confusion: I'd rather we didn't continue to do so.

Copy link
Member

Choose a reason for hiding this comment

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

We have a difference of opinion here, and that's okay. I prefer not to suppress stderr, but I'm fine merging either.

Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
@maxim-belkin maxim-belkin force-pushed the no-locale-no-problemo branch from 9abe3ee to bab43d4 Compare August 13, 2020 17:06
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Co-authored-by: Shaun Jackman <sjackman@gmail.com>
Copy link
Member

@sjackman sjackman left a comment

Choose a reason for hiding this comment

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

Thank you, Maxim!

if [[ "$(locale charmap 2>/dev/null)" != "UTF-8" ]]
if ! command -v locale >/dev/null
then
export LC_ALL=C
Copy link
Member

Choose a reason for hiding this comment

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

This isn't correct behaviour on macOS, unfortunately.

Copy link
Member

Choose a reason for hiding this comment

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

Under what circumstances does command -v locale fail on macOS? I believe that situation is impossible with SIP enabled.

Copy link
Member

Choose a reason for hiding this comment

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

It is hence I'd like to avoid the check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can rewrite this part using if [[ -n "$HOMEBREW_MACOS" ]]; then ... else ... {complex logic}; fi -- would that work?

Copy link
Member

Choose a reason for hiding this comment

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

Yeh, go for it. I'm still of the belief that checking for locale is pointless where we're correctly handling the case when it doesn't exist anyway if we hide the errors.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

I'll defer to @sjackman on Linux behaviour but macOS needs to be the same as before.

Copy link
Member

@sjackman sjackman left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you, Maxim.

@maxim-belkin
Copy link
Member Author

Thank you both!

@maxim-belkin maxim-belkin changed the title brew.sh: handle systems with no 'locale' brew.sh: handle Linux systems with no 'locale' Aug 17, 2020
@maxim-belkin maxim-belkin merged commit a72c971 into Homebrew:master Aug 17, 2020
@maxim-belkin maxim-belkin deleted the no-locale-no-problemo branch August 17, 2020 16:16
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 17, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dockerfile for alpine not working
4 participants