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

Set the stream locale to "C" locally #724

Merged
merged 5 commits into from
Mar 2, 2024

Conversation

sbarzowski
Copy link
Collaborator

So that even if there is a global locale
set by the library user, Jsonnet output
is still the same.

@sbarzowski
Copy link
Collaborator Author

Fixes #722.

It makes sense to add a regression test and check that we don't have the same problem in other places. Perhaps we can have a helper function to create stringstreams and clear the locale?

@sparkprime
Copy link
Contributor

The test failed on mac gcc. It looked like a legitimate failure with the locale stuff but I didn't understand the error. I ran it again anyway, just in case.

@sparkprime
Copy link
Contributor

Same thing:

terminate called after throwing an instance of 'std::runtime_error'
  what():  locale::facet::_S_create_c_locale name not valid

@sparkprime
Copy link
Contributor

My guess would be that it needs some packaged installed to get the right locale files. However I'm not sure why it works on clang but not gcc? Maybe they use different standard libraries and the clang library bundles it.

@sbarzowski
Copy link
Collaborator Author

I tried getting to the bottom of this again, but I'm still very confused. One idea that I haven't checked yet is that the locale is just not there, but clang-compiled-version falls back to something "gracefully" and gcc-compiled-version crashes.

@johnbartholomew
Copy link
Collaborator

This still seems worth fixing. std::locale::classic / C locale should always be available I think. We just need the test itself to not rely on a particular locale that might not be available on the system where the test is running.

I've rebased to master and made some adjustments to the test, see master...johnbartholomew:jsonnet:sbarzowski-fix-locale-dependence

@johnbartholomew
Copy link
Collaborator

Re: the test failure on Mac (#724 (comment)), it might be this? https://stackoverflow.com/a/58385001

Anyway, I think it can/should be avoided by just defining a custom locale facet for the test.

@johnbartholomew
Copy link
Collaborator

As this PR has been open for a long time (4+ years), I'll just go ahead and rebase and merge this (with a few adjustments/additions)

This adds it to the 'all' target and also (more importantly) adds it to
the list of files that are removed by 'make clean'
@johnbartholomew johnbartholomew merged commit dfe5dfd into google:master Mar 2, 2024
6 checks passed
sbarzowski pushed a commit to sbarzowski/jsonnet that referenced this pull request Jun 10, 2024
…fmt (google#724)

* Gracefully handle encountered regular expression when running jsonnetfmt, adding tests.

* Do not validate verbatim strings.

* Also do not validate string blocks.

* Change golden prefix for formatter tests to fmt.golden to be consistant with cpp version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants