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: fixes for UTF-8 #8072

Merged
merged 1 commit into from Jul 28, 2020
Merged

Conversation

maxim-belkin
Copy link
Member

  • 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?

Fix Homebrew detection mechanism of usable locale.

Order of locales #8047 (comment):

  1. The user's current locale if it's UTF-8
  2. C.UTF-8
  3. en_US.UTF-8
  4. First available UTF-8 locale
  5. C

Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

brew style is failing due to shellcheck: https://github.com/Homebrew/brew/pull/8072/checks?check_run_id=904644217

@maxim-belkin
Copy link
Member Author

brew style is failing due to shellcheck: https://github.com/Homebrew/brew/pull/8072/checks?check_run_id=904644217

Yeh, my bad -- I noticed that but had to leave and left the PR without a note that I'll continue working on it.
By the way, brew style doesn't fail for me in a Homebrew/brew Docker container, which is weird.

@maxim-belkin
Copy link
Member Author

Ready for a second review.

Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
then
export LC_ALL=C
else
echo "Warning: Could not find usable locale." >&2
Copy link
Member

Choose a reason for hiding this comment

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

What's the action item for the user here?

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 wondering if this should be not output or be a hard-stop error).

Copy link
Member Author

Choose a reason for hiding this comment

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

install either a UTF-8 locale of choice or C locale.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, yeh, I'd make this an odie with instructions on how to resolve.

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good to me but I'd like to run this by @sjackman 👀

Copy link
Member

Choose a reason for hiding this comment

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

The user is pretty much guaranteed to have a C locale. If they do not, the world is burning. It's more likely that locale failed to run (they don't have this executable in their PATH, which itself is pretty unlikely) than they don't have a C locale. Rather than emit a warning or error in this else case, I'd just set LC_ALL=C and move on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rather than emit a warning or error in this else case, I'd just set LC_ALL=C and move on.

OK

Copy link
Member Author

Choose a reason for hiding this comment

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

Shall we remove c_regex then?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Comment on lines 16 to 19
c_utf_regex='\bC\.(utf|UTF-)8'
en_us_regex='en_US\.(utf|UTF-)8'
utf_regex='[a-z][a-z]_[A-Z][A-Z]\.(utf|UTF-)8'
c_regex='\bC\b'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
c_utf_regex='\bC\.(utf|UTF-)8'
en_us_regex='en_US\.(utf|UTF-)8'
utf_regex='[a-z][a-z]_[A-Z][A-Z]\.(utf|UTF-)8'
c_regex='\bC\b'
c_utf_regex='^C\.(utf8|UTF-8)$'
en_us_regex='^en_US\.(utf8|UTF-8)$'
utf_regex='^[a-z][a-z]_[A-Z][A-Z]\.(utf8|UTF-8)$'
c_regex='^C$'

For style more than correctness. ^…$ is more common and easier to read than \b. Anchor all patterns for consistency. Super minor but I found (utf8|UTF-8) easier to read than (utf|UTF-)8.

Copy link
Member Author

Choose a reason for hiding this comment

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

^...$ don't work as one may expect because ^ and $ match the beginning and end of locale -a rather than the beginning and end of each word.

# In Homebrew/brew Docker image
$ locale -a
C
C.UTF-8
POSIX
en_US.utf8
$ locales=$(locale -a)
$ c_utf_regex='\bC\.(utf8|UTF-8)\b'
$ [[ $locales =~ $c_utf_regex ]] && echo ${BASH_REMATCH[0]}
C.UTF-8
$ c_utf_regex='^C\.(utf8|UTF-8)$'
$ [[ $locales =~ $c_utf_regex ]] && echo ${BASH_REMATCH[0]}
# nothing is printed; return status == 1

Anchor all patterns for consistency.

OK to use \b?

Super minor but I found (utf8|UTF-8) easier to read than (utf|UTF-)8.

OK.

Copy link
Member

Choose a reason for hiding this comment

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

OK to use \b?

Yep! Thanks for the explanation.

@sjackman
Copy link
Member

Thank you for this PR, Maxim!

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.

Thanks, Maxim!

@maxim-belkin
Copy link
Member Author

Thanks for the reviews, @MikeMcQuaid and @sjackman!

@maxim-belkin maxim-belkin merged commit 06f078f into Homebrew:master Jul 28, 2020
@maxim-belkin maxim-belkin deleted the utf8-fix branch July 28, 2020 14:02
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 21, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 21, 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.

None yet

4 participants