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

Exec: use C instead of en_US.UTF-8 to set the Exec locale. #9391

Merged
merged 4 commits into from
Nov 6, 2023

Conversation

tmds
Copy link
Member

@tmds tmds commented Nov 2, 2023

The en_US locale can't be used on systems where it is not installed. This is common in container images.

On such systems, setting the locale to en_US.UTF-8 causes unexpected warnings to be written to standard error.

When Exec.LogStandardErrorAsError is set, these warnings cause the Task to fail due to logging errors.

This changes to use the 'Computer English' C.UTF-8 locale, which is always available.

Fixes #4194

@rainersigwald @wfurt @janvorli ptal

cc @mthalman @omajid

The en_US locale can't be used on systems where it is not installed.
This is common in container images.

On such systems, setting the locale to en_US.UTF-8 causes unexpected
warnings to be written to standard error.

When Exec.LogStandardErrorAsError is set, these warnings cause
the Task to fail due to logging errors.

This changes to use the 'Computer English' C.UTF-8 locale, which is
always available.
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM. thanks.

@tmds tmds changed the title Exec: use C.UTF-8 instead of en_US.UTF-8 to set the UTF-8 encoding. Exec: use C instead of en_US.UTF-8 to set the Exec locale. Nov 3, 2023
@JaynieBai JaynieBai merged commit 7a6b2dd into dotnet:main Nov 6, 2023
8 checks passed
@tmds
Copy link
Member Author

tmds commented Nov 6, 2023

This was not meant to be merged yet, as we were still discussing the options in #4194. I should have made it more apparent in this PR.

Based on the discussion there, leaving out the LANG/LC_ALL (what was merged) is a good option.

@rainersigwald @wfurt @janvorli @danmoseley @KalleOlaviNiemitalo if it is good for you, I think we can keep this and see what feedback we get during the .NET 9 development.

@rainersigwald
Copy link
Member

Our main is currently flowing into 8.0.200 builds as well as 9.0.100 builds so I think we should be cautious and revert for now. Sorry for the confusion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exec tasks warns: bash : warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
5 participants