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

Force the Bazel server Java runtime to use the root locale #17702

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Mar 9, 2023

This ensures consistent behavior of string operations even if the individual operations do not set a locale.

Without this change, Bazel can't operate in e.g. a Turkish locale, where it fails with error messages such as:

In rule 'test', size 'medium' is not a valid size.

This is because Turkish case mapping rules make it so that a capital ASCII 'I' lowercases to a non-ASCII variant of 'i'.

Fixes #17541

This ensures consistent behavior of string operations even if the
individual operations do not set a locale.

Without this change, Bazel can't operate in e.g. a Turkish locale, where
it fails with error messages such as:

In rule 'test', size 'medium' is not a valid size.

This is because Turkish case mapping rules make it so that a capital
ASCII 'I' lowercases to a non-ASCII variant of 'i'.
@fmeum fmeum marked this pull request as ready for review March 9, 2023 10:51
@fmeum
Copy link
Collaborator Author

fmeum commented Mar 9, 2023

@lberki @meteorcloudy

@sgowroji sgowroji added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Mar 9, 2023
@meteorcloudy meteorcloudy requested a review from lberki March 9, 2023 11:07
@fmeum
Copy link
Collaborator Author

fmeum commented Mar 9, 2023

Do we still want #17687 for clarity even though we now have this PR?

@meteorcloudy
Copy link
Member

@lberki I believe this PR is preferred over #17687?

@meteorcloudy
Copy link
Member

meteorcloudy commented Mar 9, 2023

My feeling is #17687 is no longer necessary unless we want to enable the error prone check (which is not available yet)?

@lberki
Copy link
Contributor

lberki commented Mar 9, 2023

Yeah, I think I prefer this over #17687 . Less opportunity for things to go wrong.

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 9, 2023

Do you think that this is cherry-pickable? I would have been more comfortable suggesting that with the previous approach, but I also don't see why it shouldn't be safe with this one.

@lberki
Copy link
Contributor

lberki commented Mar 9, 2023

On balance, I think the fact that to{Lower,Upper}Case is locale-dependent is a bigger risk than whatever trouble cherry-picking this might cause.

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 9, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Mar 9, 2023
@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Mar 9, 2023
@meteorcloudy
Copy link
Member

@bazel-io fork 6.2.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Mar 9, 2023
@copybara-service copybara-service bot closed this in 3ad3927 Mar 9, 2023
@fmeum fmeum deleted the fix-root-locale branch March 9, 2023 19:04
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 10, 2023
ShreeM01 pushed a commit to ShreeM01/bazel that referenced this pull request Mar 10, 2023
This ensures consistent behavior of string operations even if the individual operations do not set a locale.

Without this change, Bazel can't operate in e.g. a Turkish locale, where it fails with error messages such as:

In rule 'test', size 'medium' is not a valid size.

This is because Turkish case mapping rules make it so that a capital ASCII 'I' lowercases to a non-ASCII variant of 'i'.

Fixes bazelbuild#17541

Closes bazelbuild#17702.

PiperOrigin-RevId: 515339563
Change-Id: I8417d0befd76ba6d140588be5f7e50529af3f6c7
meteorcloudy pushed a commit that referenced this pull request Mar 13, 2023
This ensures consistent behavior of string operations even if the individual operations do not set a locale.

Without this change, Bazel can't operate in e.g. a Turkish locale, where it fails with error messages such as:

In rule 'test', size 'medium' is not a valid size.

This is because Turkish case mapping rules make it so that a capital ASCII 'I' lowercases to a non-ASCII variant of 'i'.

Fixes #17541

Closes #17702.

PiperOrigin-RevId: 515339563
Change-Id: I8417d0befd76ba6d140588be5f7e50529af3f6c7

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
This ensures consistent behavior of string operations even if the individual operations do not set a locale.

Without this change, Bazel can't operate in e.g. a Turkish locale, where it fails with error messages such as:

In rule 'test', size 'medium' is not a valid size.

This is because Turkish case mapping rules make it so that a capital ASCII 'I' lowercases to a non-ASCII variant of 'i'.

Fixes bazelbuild#17541

Closes bazelbuild#17702.

PiperOrigin-RevId: 515339563
Change-Id: I8417d0befd76ba6d140588be5f7e50529af3f6c7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
5 participants