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

Don't force "en_US.utf8" locale #73

Merged
merged 1 commit into from Jul 26, 2018
Merged

Don't force "en_US.utf8" locale #73

merged 1 commit into from Jul 26, 2018

Conversation

niklas88
Copy link
Member

@niklas88 niklas88 commented Jul 24, 2018

Instead of using setlocale(LC_CTYPE, "en_US.utf8") and thus forcing "en_US" we
now use setlocale(LC_CTYPE, "") which configures the program to use the default
locale's character set instead of defaulting to the uber-portable "C" or "POSIX"
locales. (see "man 3 setlocale").

On a sane system this will use UTF-8, we do not care about insane systems.

The main advantage of this change is that it allows to use docker containers
without running "locale-gen" in the container thus making the image smaller
(~150 MB instead of ~220 MB using a Ubuntu 18.04 base)

Instead of using setlocale(LC_CTYPE, "en_US.utf8") and thus forcing "en_US" we
now use setlocale(LC_CTYPE, "") which configures the program to use the default
locale's character set instead of forcing the uber-portable "C" or "POSIX"
locale.  (see "man 3 setlocale").

On a sane system this will use UTF-8, we do not care about insane systems.

The main advantage of this change is that it allows to use docker containers
without running "locale-gen" in the container thus making the image smaller.
@floriankramer
Copy link
Member

QueryPlanningCostFactors::readFromFile uses atof which returns wrong values if the program's and the system's locale don't match.
Apart from that everything seems to be fine.

@niklas88
Copy link
Member Author

Arghh, there are few things I feel as profound hatred for as for whoever thought it was a good idea to ever internationalize decimal separators. Instead everyone should have just let "." win worldwide and be done with this nonsense.

That said, I only did setlocale(LC_CTYPE, "") i.e. only set the LC_CTYPE to the system default. So LC_NUMERIC should stay "C". I just tried this with printing it on an en_US.UTF-8 system.

@floriankramer or can you counter confirm that it breaks on your system?

Copy link
Member

@floriankramer floriankramer left a comment

Choose a reason for hiding this comment

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

I did a quick test and had no problems after setting the LC_CTYPE locale. I thought we set the LC_ALL locale somewhere but apparently was mistaken.

@niklas88 niklas88 merged commit 78e6410 into ad-freiburg:master Jul 26, 2018
@niklas88 niklas88 deleted the simplify-utf8 branch October 31, 2018 09:35
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.

None yet

2 participants