Skip to content

brew.sh: fixes for UTF-8#8072

Merged
maxim-belkin merged 1 commit intoHomebrew:masterfrom
maxim-belkin:utf8-fix
Jul 28, 2020
Merged

brew.sh: fixes for UTF-8#8072
maxim-belkin merged 1 commit intoHomebrew:masterfrom
maxim-belkin:utf8-fix

Conversation

@maxim-belkin
Copy link
Copy Markdown
Contributor

  • 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

Comment thread Library/Homebrew/brew.sh Outdated
Comment thread Library/Homebrew/brew.sh Outdated
Comment thread Library/Homebrew/brew.sh Outdated
Comment thread Library/Homebrew/brew.sh Outdated
Comment thread Library/Homebrew/brew.sh Outdated
Comment thread Library/Homebrew/brew.sh Outdated
@MikeMcQuaid
Copy link
Copy Markdown
Member

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

@maxim-belkin
Copy link
Copy Markdown
Contributor 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
Copy Markdown
Contributor Author

Ready for a second review.

Comment thread Library/Homebrew/brew.sh Outdated
Comment thread Library/Homebrew/brew.sh Outdated
Comment thread Library/Homebrew/brew.sh Outdated
Comment thread Library/Homebrew/brew.sh Outdated
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense.

Comment thread Library/Homebrew/brew.sh Outdated
Comment thread Library/Homebrew/brew.sh Outdated
Comment on lines 16 to 19
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

Thank you for this PR, Maxim!

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor 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.

4 participants