Skip to content

Fix locale fallback and guard tests against invalid locale settings #5950

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 3 commits into from
Apr 1, 2015
Merged

Fix locale fallback and guard tests against invalid locale settings #5950

laanwj merged 3 commits into from
Apr 1, 2015

Conversation

@dexX7
Copy link
Contributor

@dexX7 dexX7 commented Mar 27, 2015

Unfortually a fallback to "C" locale via std::locale::global does not cover all scenarios with messed up environment locale settings and on Ubuntu 14.01 (with LANG=en_US.UTF-8, LANGUAGE=en_US, LC_* empty) setting LANG=invalid triggers a crash right at the start of bitcoind and bitcoin-qt.

This also affects test_bitcoin and test_bitcoin-qt, which were not guarded at all.

The PR expands the scope of the locale fallback and prevents crashes due to invalid locale settings of bitcoind, bitcoin-qt, test_bitcoin and test_bitcoin-qt. I didn't want to create a seperate PR for the 0.10 branch, but a conflict free version is available: 0.10...dexX7:0.10-init-locale-fallback-tests-fix

@dexX7
Initialization: set fallback locale as environment variable
ba0fa0d 
The scope of `std::locale::global` appears to be smaller than `setenv("LC_ALL", ...)` and insufficient to fix messed up locale settings for the whole application.
@laanwj
Copy link
Member

laanwj commented Mar 27, 2015

This appears to revert part of #5877.

Can you add a concrete test case for this? This more and more looks like just some occult incantation to ward off the evil spirits of bad locales :)

Diapolo
Diapolo reviewed Mar 27, 2015
View reviewed changes
src/test/test_bitcoin.cpp Outdated
Copy link

@Diapolo Diapolo Mar 27, 2015

Choose a reason for hiding this comment

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

Nit: Wrong indentation, also true for bitdb.MakeMock(); below.

dexX7 added 2 commits March 29, 2015 10:35
@dexX7
Initialization: setup environment before starting tests
fc3979a 
The environment is prepared by the main thread to guard against invalid locale settings and to prevent deinitialization issues of Boost path, which can result in app crashes.
@dexX7
Initialization: setup environment before starting QT tests
3a3ecc0 
The environment is prepared by the main thread to guard against invalid locale settings.
@dexX7 dexX7 force-pushed the master-init-locale-fallback-tests-fix branch from c2ce063 to 3a3ecc0 Compare March 29, 2015 08:36
@dexX7
Copy link
Contributor Author

dexX7 commented Mar 29, 2015

@laanwj: Yeah, I'm truely sorry.. :/ I focused on addressing the deinitialization issue, which caused the crashes on Windows, probably with two steps forward, where only one was appropriate, as it seems.

I further moved TestingSetup() from TestingSetup() to BasicTestingSetup(), after running a proper test.

The RPC test is available here:
https://gist.github.com/dexX7/ab3f7411a9040e96d55c

Travis build with the test, based on unfixed master (with delayed error assertion):
https://travis-ci.org/dexX7/bitcoin/builds/56274510

Travis build with the test, based on the 0.10 branch:
https://travis-ci.org/dexX7/bitcoin/builds/56281938

Travis build with the test, based on this pull request:
https://travis-ci.org/dexX7/bitcoin/builds/56281999

May I add the test to qa/rpc-tests as part of this PR, but without adding it to qa/pull-tester/run-rpc-tests.sh, as it is somewhat time consuming?

@Diapolo: I'd be happy to format src/test/test_bitcoin.cpp and update the copyright header in src/qt/test/test_main.cpp, if that would be considered as appropriate for the scope of this PR?

@laanwj
Copy link
Member

laanwj commented Apr 1, 2015

May I add the test to qa/rpc-tests as part of this PR, but without adding it to qa/pull-tester/run-rpc-tests.sh, as it is somewhat time consuming?

Sure! Although I'd prefer making the test consume less time, and adding it to the pull tester. What is so time consuming? The environment setup is one of the first things it does, after all, so at least in theory it could be a matter of quickly invoking the executables.

In any case you've convinced me enough to merge this, adding the test could be a further PR.

@laanwj laanwj merged commit 3a3ecc0 into bitcoin:master Apr 1, 2015
laanwj added a commit that referenced this pull request Apr 1, 2015
@laanwj
Merge pull request #5950
41113e3 
3a3ecc0 Initialization: setup environment before starting QT tests (dexX7)
fc3979a Initialization: setup environment before starting tests (dexX7)
ba0fa0d Initialization: set fallback locale as environment variable (dexX7)
@dexX7
Copy link
Contributor Author

dexX7 commented Apr 1, 2015

Although I'd prefer making the test consume less time, and adding it to the pull tester. What is so time consuming?

The test is probably far away from being optimal. :) While bitcoind is stopped as soon as possible, right now test_bitcoin and test_bitcoin-qt are executed four times each (with different locale settings), only to check, if the process terminates with exit status 0.

Edit: test_bitcoin does not terminate immediately without the environment setup/locale guard, but instead a few tests fail.

@gavinandresen
Copy link
Contributor

gavinandresen commented Apr 1, 2015

RE: making the test faster: you can run just one bitcoind test suites or test cases using test_bitcoin --run_test=FOO (run all tests in FOO test suite) or --run_test=FOO/BAR (run the BAR test in the FOO test suite).

I believe multiple --run_test's can be given if you want to run more than one.

@dexX7
Copy link
Contributor Author

dexX7 commented Apr 2, 2015

@gavinandresen: thanks for the hint, this is very useful!

But just to clarify: the errors were not visible right after starting the Boost tests, and not all tests fail, but only a few, which use boost::path in one way or another. Further, the actual failures are sort of opaque, and the underlying issue is expressed in more than one way (see: bitcoin/jobs/56274515#L6165-L6228).

@dexX7
Copy link
Contributor Author

dexX7 commented Apr 17, 2015

@laanwj: would you like a similar fix for 0.10, too? I sort of have a bad feeling, because I initially changed setenv("LC_ALL", "C", 1) to std::locale::global(std::locale("C")), which then turned out to be less reliable, and 0.10 currently still fails the test.

@laanwj
Copy link
Member

laanwj commented Apr 18, 2015 via email

@dexX7 dexX7 mentioned this pull request Apr 18, 2015
Merged
@dexX7 dexX7 mentioned this pull request May 4, 2015
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dexX7 @laanwj @gavinandresen @Diapolo