On 2019-10-04 15:16, Erik Joelsson wrote:
On 2019-10-04 04:37, Magnus Ihse Bursie wrote:
On 2019-10-02 17:40, Erik Joelsson wrote:
Hello Magnus,
The change looks good, but should perhaps also include removal of
the few scattered specific instances of LANG=C and LC_ALL=C in the
makefiles:
make/common/JavaCompilation.gmk: export LC_ALL=C ; ( $(CAT) $$<
&& $(ECHO) "" ) \
make/autoconf/basics.m4: DATE_WHEN_CONFIGURED=`LANG=C date`
Also compare.sh has a bunch which could be replaced with one export
at the top.
Ok, here's an updated version where I've deleted all other LC_* and
LANG assignments in the build system. FWIW, I found a few instances
in test code, but I did not change that.
http://cr.openjdk.java.net/~ihse/JDK-8065704-LC_ALL/webrev.02
The removal in RunTestPrebuilt.gmk reminded me that we do not run
configure before running tests, and in that situation spec.gmk.in is
not used either. The same export needs to be added somewhere in the
run-test-prebuilt chain of invocations.
Good catch! Here's an updated webrev; the only change is in
RunTestsPrebuiltSpec.gmk:
http://cr.openjdk.java.net/~ihse/JDK-8065704-LC_ALL/webrev.03
(Third times's a charm...)
I would expect a full round of builds-infra to be run for this, as
well as explicit compare builds as subtle things could be changing.
Yes, of course. My original patch has already made the rounds on the
builds-infra test suite without any issues, but I'll do a rerun with
this second version of the patch as well.
Thanks!
It turned out that I had forgotten how to properly run COMPARE_BUILD on
our build system, so while I did test that it built fine even on
esoteric platforms and combinations using builds-infra, I did not in
fact verify that there was no changes. :-/ When I reran this properly, I
found two mismatches, one on macOS and one on Windows. I pinpointed the
macOS issue, and Erik helped me with the Windows issue (thanks Erik!).
Both of them turned out to be real (if minor) problems we had, where the
build output on those platform were different due to the lack of LC_ALL.
So this does indeed confirm that this patch helps! :-)
/Magnus
/Erik
/Magnus
/Erik
On 2019-10-02 02:06, Magnus Ihse Bursie wrote:
From the bug report:
We should prefix LC_ALL=C for most, maybe all, tools we use when
building.
This probably means we should run "export LC_ALL=C" early in the
configure script as well.
---
The fix itself is trivial. While I know we've had several issues
regarding localization, I could not find any specific instances now
that I was looking for them. I searched JBS for a while but could
not dig up anything that was reproducible. So, unfortunately, I
have been unable to verify that this solves any actual problems.
That being said, I believe this is a prudent fix that should have
been in place long time ago. But if anyone can give me a concrete
example that breaks so that I can verify that this helps, please
let me know.
Bug: https://bugs.openjdk.java.net/browse/JDK-8065704
WebRev: http://cr.openjdk.java.net/~ihse/JDK-8065704-LC_ALL/webrev.01
/Magnus
