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

testsuite: add LC_ALL default to util.sh #3552

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

GitMensch
Copy link
Contributor

@GitMensch GitMensch commented Jul 7, 2023

The testsuite should run on any system with different settings, so create a reasonable default for all tests.

fixes #3545

@bernhardu
Copy link
Contributor

Seems CI reports with this change for madvise_dontfork a "timeout 120 exceeded".

@GitMensch
Copy link
Contributor Author

yay, I only get a timeout if I don't use that :-/

@dzaima
Copy link
Contributor

dzaima commented Jul 7, 2023

This is in the log:

warning: setlocale: LC_ALL: cannot change locale (C.utf8): No such file or directory

So the CI doesn't have the C.utf8 locale. So an option that'd at least ignore that would be putting the LC_ALL=C.utf8 behind a check of locale -a | grep C.utf8 or something. Of course that'd make it not properly set it on systems where it doesn't exist, but that'd at least not be a guaranteed failure.

@bernhardu
Copy link
Contributor

bernhardu commented Jul 7, 2023

I did parallel a few tests and this is the output of a ls -lisah /usr/lib/locale and locale -a from a CI system.
And it looks like at the CI system there is just a "C.UTF-8" while e.g. at my local Debian system there is just a "C.utf8".
I guess a solution could be to use export LC_ALL=$(locale -a | grep -i -E "^C.utf"), at least my last attempt in my test PR 3554 succeeded.

Edit: corrected last link.

@GitMensch
Copy link
Contributor Author

Thank you for inspecting this further! I'll do some tests later, then amend the commit to use that approach (also testing for 8 to ensure we don't set it to utf16).

@bernhardu
Copy link
Contributor

also testing for 8 to ensure we don't set it to utf16

Then maybe locale -a | grep -i -E "^C\.utf.*8$" might do it? (Also added \ for the dot.)

@bernhardu
Copy link
Contributor

But now I see it - the environment shows LANG=en_US.UTF-8, but it looks like the available locale is en_US.utf8. Is this maybe the original cause?

@bernhardu
Copy link
Contributor

Now I see at my local system I have the same difference, just with de_DE....
And the test works with LANG set in both ways in my Debian Bookworm system.
From reading this discussion it looks like en_US.UTF-8 should be valid ...

In run 1255 I even found CI defines LANG=en_US.UTF-8, which is reported by locales -a,
but does not even exist below /usr/lib/locale.

@bernhardu
Copy link
Contributor

Can you provide the values of your LANG, LANGUAGE, LC_ALL, LCTYPE, and some details about your system?

@rocallahan
Copy link
Collaborator

FYI I moved this Unicode stuff to its own test in 9e1101a

@GitMensch
Copy link
Contributor Author

Should the LC setting only be done in unicode.run or is it still fine to add that to util.sh?

@rocallahan
Copy link
Collaborator

Probably only unicode.run if it requires running a subprocess to figure out which setting to use.

The testsuite should run on any system with different settings, so create a reasonable default for all tests, fixing rr-debugger#3545.
@GitMensch
Copy link
Contributor Author

The normal build now works fine, so it seems the change to util.sh is fine.

@GitMensch
Copy link
Contributor Author

@rocallahan Thanks for splitting the unicode test out. As we found that the "simple" setting works and we don't need to run any subcommands I've opted to add it to util.h, leading to both GH CI and the local build to work fine.

@rocallahan rocallahan merged commit b5bf0a8 into rr-debugger:master Aug 21, 2023
1 check passed
@GitMensch GitMensch deleted the patch-3 branch August 21, 2023 14:06
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.

Test 'madvise_dontfork' FAILED: debug script failed
4 participants