RFR: JDK-8065704 Set LC_ALL=C for all relevant commands in the build system

[Magnus Ihse Bursie]

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
>>