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

Spawn processes with LANG that's guaranteed to exist #506

Merged
merged 2 commits into from
Sep 6, 2024

Conversation

imphil
Copy link
Contributor

@imphil imphil commented Aug 19, 2024

LANG=en_US is not guaranteed to exist on any system (it does not, for
strange reasons, exist on mine). Choose LANG=C, which is guaranteed to
exist. It has the side-effect of typically using an encoding other than
UTF-8 for the tool output (e.g., ISO 8859-1). Apart from messages
printed into the log in this encoding that should hopefully have no
negative side-effects.

See #505 for a case where
code was assuming that output would always be English, and failed if it
wasn't.

`LANG=en_US` is not guaranteed to exist on any system (it does not, for
strange reasons, exist on mine). Choose `LANG=C`, which is guaranteed to
exist. It has the side-effect of typically using an encoding other than
UTF-8 for the tool output (e.g., ISO 8859-1). Apart from messages
printed into the log in this encoding that should hopefully have no
negative side-effects.

See sorenlouv#505 for a case where
code was assuming that output would always be English, and failed if it
wasn't.
@sorenlouv
Copy link
Owner

This LGTM. Thanks for the contribution. Is #505 still needed if this is merged?

@sorenlouv sorenlouv enabled auto-merge (squash) September 5, 2024 17:07
@imphil
Copy link
Contributor Author

imphil commented Sep 6, 2024

Is #505 still needed if this is merged?

No.

However, #505 adds additional robustness to the process, so I'd personally just go with both of them to have less surprises going forward.

@sorenlouv sorenlouv disabled auto-merge September 6, 2024 06:57
@sorenlouv sorenlouv merged commit c824547 into sorenlouv:main Sep 6, 2024
2 of 3 checks passed
@sorenlouv
Copy link
Owner

Released in 9.6.0

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.

2 participants