-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
pythonPackages.Babel: Fix use of glibcLocales, skip musl tests #75676
pythonPackages.Babel: Fix use of glibcLocales, skip musl tests #75676
Conversation
@GrahamcOfBorg build python27Packages.Babel python3Packages.Babel pkgsMusl.python27Packages.Babel pkgsMusl.python3Packages.Babel Can somebody please ofborg-build this on OSX for me? |
Looks like |
…ixOS#74904. The test failure: File "/build/Babel-2.7.0/babel/messages/pofile.py", line 325, in _invalid_pofile print(u"WARNING: Problem on line {0}: {1}".format(lineno + 1, line)) UnicodeEncodeError: 'ascii' codec can't encode character u'\xe0' in position 84: ordinal not in range(128) occurs in either of these cases: * glibc is used and `glibcLocales` is not a `buildInput` * musl is used and tests are run `python3Packages.Babel` does not have this problem, on either glibc or musl. I also found that the `preCheck` `export LC_ALL="en_US.UTF-8"` is not necessary in any case. With this commit, python27Packages.Babel python3Packages.Babel pkgsMusl.python27Packages.Babel pkgsMusl.python3Packages.Babel all build.
525c135
to
9cfb480
Compare
@GrahamcOfBorg build python27Packages.Babel python3Packages.Babel pkgsMusl.python27Packages.Babel pkgsMusl.python3Packages.Babel |
OK I could repro it at least on one of my machines now -.- |
preCheck = if isPy3k then null else '' | ||
export LC_ALL=C.UTF-8 | ||
''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also be:
preCheck = lib.optionalString isPy27 ''
export LC_ALL=C.UTF-8
'';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no opinion on that; I wanted to express that "versions < 3" get this wrong, and isPy27
seems more specific than that. But at the same time, is the only < 3 version available in nixpkgs and no 2.8 is supposed to be released, so it may as well be equivalent.
Whichever you think is better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion either
I suspect the unicode error difference occurs depending on the version of With nix 2.0.4 on NixOS 18.03 in the nixpkgs checkout of this PR, I can reproduce:
But if I build with nix 2.3.1 by running, on the same machine:
then the build of the same @edolstra Are you aware of any changes to |
The difference is likely somewhere between nix 2.2.2 and nix 2.3:
|
Preliminary bisection results yield:
Full bisection log (initial
|
I have checked manually and found that this
@grahamc confirmed on IRC that ofborg runs What I'm not sure on is: Is this now a nix bug, or a Python / Babel bug? Should the test work even when not in a pseudo-terminal? Should I set that locale? |
TBH I think the bug is squarely in Babel's side: builds shouldn't depend upon a build-time PTY and it is useful for builds to continue working in older versions of Nix. That said, ofborg needs a system upgrade anyway, which may happen today (or next month...). |
I buy this argument. What should we do to ensure that though, given that Nix 2.3, and soon ofborg, may hide these problems? |
@jonringer I've confirmed now that adding any Even before this PR, on current
I have most likely cleared this up as well now: I used remote |
this is now passing on master |
going to close if no one has any reservations |
I pushed 296b2ab which (partially) fixed this. |
also, conflicts |
I have figured out what the precise difference is.
To verify it, I printed In the Babel test suite (here): import sys, locale, os
print(('sys.stdout.encoding', sys.stdout.encoding))
print(('sys.stdout.isatty()', sys.stdout.isatty()))
print(('locale.getpreferredencoding()', locale.getpreferredencoding()))
print(('sys.getfilesystemencoding()', sys.getfilesystemencoding()))
print(('os.environ["PYTHONIOENCODING"]', os.environ.get("PYTHONIOENCODING"))) and got: # nix 2.2.2 with Python 2.7
('sys.stdout.encoding', None)
('sys.stdout.isatty()', False)
('locale.getpreferredencoding()', 'UTF-8')
('sys.getfilesystemencoding()', 'UTF-8')
('os.environ["PYTHONIOENCODING"]', None)
# nix 2.3.2 with Python 2.7
('sys.stdout.encoding', 'UTF-8')
('sys.stdout.isatty()', True)
('locale.getpreferredencoding()', 'UTF-8')
('sys.getfilesystemencoding()', 'UTF-8')
('os.environ["PYTHONIOENCODING"]', None)
# nix 2.2.2 with Python 3.7
('sys.stdout.encoding', 'UTF-8')
('sys.stdout.isatty()', False)
('locale.getpreferredencoding()', 'UTF-8')
('sys.getfilesystemencoding()', 'utf-8')
('os.environ["PYTHONIOENCODING"]', None)
# nix 2.3.2 with Python 3.7
('sys.stdout.encoding', 'UTF-8')
('sys.stdout.isatty()', True)
('locale.getpreferredencoding()', 'UTF-8')
('sys.getfilesystemencoding()', 'utf-8')
('os.environ["PYTHONIOENCODING"]', None) As you can see:
When on Python 2.7 with Nix 2.2 this line from the tests containing the
is run through
and that cannot be The issue can be reproduced on Ubuntu 18.04 by running (I used Babel commit So it's a Babel |
@grahamc I've PR'd a fix to Babel: python-babel/babel#691 |
I've also added a comment explaining the above in #78644. I have confirmed that I may still want to make a PR to apply the upstream patch I submitted to fix the test, because I use some remote builders that use Nix < 2.2. |
❤️ 🚀 Thanks for the great work chasing this down!! |
(cherry picked from commit 401649d)
Fixes #74904.
The test failure:
occurs in either of these cases:
glibcLocales
is not abuildInput
python3Packages.Babel
does not have this problem, on either glibcor musl.
I also found that the
preCheck
export LC_ALL="en_US.UTF-8"
is notnecessary in any case.
With this commit,
all build.
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
No maintainers listed.
FYI @dtzWill from #74904.