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

2019-10-09 Thread Erik Joelsson 

Looks good.

/Erik

On 2019-10-04 04:52, Magnus Ihse Bursie wrote:

On 2019-10-02 17:45, Martin Buchholz wrote:
I recall years ago running into troubles with regex character ranges, 
e.g.
https://unix.stackexchange.com/questions/15980/does-should-lc-collate-affect-character-ranges 


but my build script wrapper has been setting LC_ALL=C for a long time,
and I set LC_COLLATE=C in my normal use shell environment
Hah, that's a funny (i.e. very unexpected, and not particularly funny 
at all) side effect of localization. :) We're doing quite a lot of 
"a-z" in the build systems; we should probably change them to 
"[[:lower:]]".

(do regular humans care deeply about getting localized collation order?)
But oh yes! I'd *hate* it for my åäö to be sorted anywhere but after 
xyz. Probably just as much as the Germans would hate to *not* have the 
ä, ö and ü sorted alongside the a, o and u. You're just having the 
perspective of privilege from US-ASCII being considered the universal 
default. ;-)


/Magnus


On Wed, Oct 2, 2019 at 2:09 AM 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

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

2019-10-09 Thread 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

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

2019-10-04 Thread Erik Joelsson 

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.


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!

/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

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

2019-10-04 Thread Magnus Ihse Bursie 

On 2019-10-02 17:45, Martin Buchholz wrote:
I recall years ago running into troubles with regex character ranges, 
e.g.

https://unix.stackexchange.com/questions/15980/does-should-lc-collate-affect-character-ranges
but my build script wrapper has been setting LC_ALL=C for a long time,
and I set LC_COLLATE=C in my normal use shell environment
Hah, that's a funny (i.e. very unexpected, and not particularly funny at 
all) side effect of localization. :) We're doing quite a lot of "a-z" in 
the build systems; we should probably change them to "[[:lower:]]".

(do regular humans care deeply about getting localized collation order?)
But oh yes! I'd *hate* it for my åäö to be sorted anywhere but after 
xyz. Probably just as much as the Germans would hate to *not* have the 
ä, ö and ü sorted alongside the a, o and u. You're just having the 
perspective of privilege from US-ASCII being considered the universal 
default. ;-)


/Magnus


On Wed, Oct 2, 2019 at 2:09 AM Magnus Ihse Bursie 
mailto:magnus.ihse.bur...@oracle.com>> 
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

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

2019-10-04 Thread Magnus Ihse Bursie 

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

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.


/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

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

2019-10-02 Thread Martin Buchholz 
I recall years ago running into troubles with regex character ranges, e.g.
https://unix.stackexchange.com/questions/15980/does-should-lc-collate-affect-character-ranges
but my build script wrapper has been setting LC_ALL=C for a long time,
and I set LC_COLLATE=C in my normal use shell environment
(do regular humans care deeply about getting localized collation order?)

On Wed, Oct 2, 2019 at 2:09 AM Magnus Ihse Bursie <
magnus.ihse.bur...@oracle.com> 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
>

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

2019-10-02 Thread Erik Joelsson 

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.


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.


/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

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

2019-10-02 Thread naoto . sato 

Hi Magnus,

Looks good to me. Although I cannot provide any reproducible problematic 
instance, there have been instances where native tools, such as `date` 
command had produced localized date, and the build failed to parse it in 
US format. Anyways, it is a good preventive measure to me.


Naoto

On 10/2/19 2:06 AM, 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

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

2019-10-02 Thread Magnus Ihse Bursie 

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