Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (build system)

[David Holmes]

On 13/05/2020 8:50 am, Mikael Vidstedt wrote:

Including hotspot-runtime-dev since the zero patch touches hotspot (os_linux) - 
please review!


On May 12, 2020, at 3:05 PM, John Paul Adrian Glaubitz 
 wrote:

Hi Mikael!

On 5/12/20 12:08 AM, John Paul Adrian Glaubitz wrote:

Adrian, did you have a chance to look at the zero patch? I’m running out of 
things to
address and I’m planning on moving forward with the JEP targeting and 
integration shortly.


I haven't tested the changes by Magnus yet, but they look good to me. Magnus
create a repo on the Linux SPARC porterbox which I can pull from for testing.

I finally have time to look into it tomorrow morning (CEST) so I can
officially ACK the changes.


Changes look fine to me with the additional changes by Magnus squashed
into yours - unless you already have done so in your latest revision.


Thank you for verifying it! Webrev here:

http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.04/zero.incr/open/webrev/


This appears to restore the bits previously removed.

I can't validate whether that is actually enough to build zero on sparc.

Cheers,
David
-



I’m only including the incremental change. It builds on the previously reviewed 
build system and hotspot webrevs:

build: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.02/build/open/webrev/
hotspot: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot/open/webrev/

(Note that I already included the UTIL_DEPRECATED_ARG_ENABLE(deprecated-ports) 
part of Magnus’ change in the build/webrev.02 change)


PS: I was just wondering: Would I be eligible to apply to become a reviewer?


One one and only one condition: Review the webrev to make sure I got it right ;)

Cheers,
Mikael

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (build system)

[Mikael Vidstedt]

Including hotspot-runtime-dev since the zero patch touches hotspot (os_linux) - 
please review!

> On May 12, 2020, at 3:05 PM, John Paul Adrian Glaubitz 
>  wrote:
> 
> Hi Mikael!
> 
> On 5/12/20 12:08 AM, John Paul Adrian Glaubitz wrote:
>>> Adrian, did you have a chance to look at the zero patch? I’m running out of 
>>> things to
>>> address and I’m planning on moving forward with the JEP targeting and 
>>> integration shortly.
>> 
>> I haven't tested the changes by Magnus yet, but they look good to me. Magnus
>> create a repo on the Linux SPARC porterbox which I can pull from for testing.
>> 
>> I finally have time to look into it tomorrow morning (CEST) so I can
>> officially ACK the changes.
> 
> Changes look fine to me with the additional changes by Magnus squashed
> into yours - unless you already have done so in your latest revision.

Thank you for verifying it! Webrev here:

http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.04/zero.incr/open/webrev/


I’m only including the incremental change. It builds on the previously reviewed 
build system and hotspot webrevs:

build: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.02/build/open/webrev/
hotspot: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot/open/webrev/

(Note that I already included the UTIL_DEPRECATED_ARG_ENABLE(deprecated-ports) 
part of Magnus’ change in the build/webrev.02 change)

> PS: I was just wondering: Would I be eligible to apply to become a reviewer?

One one and only one condition: Review the webrev to make sure I got it right ;)

Cheers,
Mikael

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (build system)

[John Paul Adrian Glaubitz]

Hi Mikael!

On 5/12/20 12:08 AM, John Paul Adrian Glaubitz wrote:
>> Adrian, did you have a chance to look at the zero patch? I’m running out of 
>> things to
>> address and I’m planning on moving forward with the JEP targeting and 
>> integration shortly.
> 
> I haven't tested the changes by Magnus yet, but they look good to me. Magnus
> create a repo on the Linux SPARC porterbox which I can pull from for testing.
> 
> I finally have time to look into it tomorrow morning (CEST) so I can
> officially ACK the changes.

Changes look fine to me with the additional changes by Magnus squashed
into yours - unless you already have done so in your latest revision.

PS: I was just wondering: Would I be eligible to apply to become a reviewer?

Thanks,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: JDK-8244844 javac command line is not re-executable

[Erik Joelsson]

Looks good.

/Erik

On 2020-05-12 10:32, Magnus Ihse Bursie wrote:



On 2020-05-12 19:17, Erik Joelsson wrote:


On 2020-05-12 10:08, Magnus Ihse Bursie wrote:

On 2020-05-12 17:38, Erik Joelsson wrote:

On 2020-05-12 08:29, Magnus Ihse Bursie wrote:
We've broken our golden rule in SetupJavaCompilation, that all 
command lines should be copy/paste:able to re-execute them from 
the command line. The reason is that the file containing the 
source code file names has a temporary name, which is renamed 
after the compilation. This is just a messy way to use the file 
both as input to javac and as a marker that the compilation has 
succeeded. By splitting up these two roles, we get more readable 
code and a re-executable command line.


Bug: https://bugs.openjdk.java.net/browse/JDK-8244844
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8244844-make-javac-re-executable/webrev.01


I really like seeing this fixed, but not sure about this patch. By 
moving the filelist to a separate rule and only having the source 
files as prereq, there will be cases where the source files 
timestamps have not changed, but the list of files may have. I 
think this would be safely covered if you moved VARDEPS_FILE as 
prereq to FILELIST.

I see your point. Will do that.

I was also a bit divided about whether the actual javac invocation 
rule should keep $$($1_SRCS) as a dependency. Technically, it's not 
needed, since changes in the sources will result in the filelist 
being updated, but maybe the intention will be better expressed. And 
if the set of files has not changed, then we will write the same 
files back to the filelist, which effectively just updates the 
timestamp in a roundabout way. If we keep the list of source files 
as dependency, we are free to implement a future optimization where 
the filelist is not updated if the set of files has not changed.


Otoh, I don't know if a huge list of dependencies (the number of 
sources files for e.g. java.base is non-trivial!) affects make 
performance, so listing it twice just to make it "look good" maybe 
isn't a good idea.


Do you have any opinion?

In general I like to list prereqs if they logically are direct 
prereqs. The sources are used as input in the compilation recipe, so 
that would be a direct prereq. If a prereq is really only transitive, 
then I don't want to list it. I don't know what the performance 
impact would be, I'm guessing it's not even measurable, but if it is, 
then maybe save some processing by not adding the sources to both 
rules. No strong opinion in this case.
Ok, I'll do it that way. I think we have generally the same preference 
here.


New webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8244844-make-javac-re-executable/webrev.02


/Magnus


/Erik


/Magnus


/Erik

Re: RFR: JDK-8244844 javac command line is not re-executable

[Magnus Ihse Bursie]

On 2020-05-12 19:17, Erik Joelsson wrote:


On 2020-05-12 10:08, Magnus Ihse Bursie wrote:

On 2020-05-12 17:38, Erik Joelsson wrote:

On 2020-05-12 08:29, Magnus Ihse Bursie wrote:
We've broken our golden rule in SetupJavaCompilation, that all 
command lines should be copy/paste:able to re-execute them from the 
command line. The reason is that the file containing the source 
code file names has a temporary name, which is renamed after the 
compilation. This is just a messy way to use the file both as input 
to javac and as a marker that the compilation has succeeded. By 
splitting up these two roles, we get more readable code and a 
re-executable command line.


Bug: https://bugs.openjdk.java.net/browse/JDK-8244844
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8244844-make-javac-re-executable/webrev.01


I really like seeing this fixed, but not sure about this patch. By 
moving the filelist to a separate rule and only having the source 
files as prereq, there will be cases where the source files 
timestamps have not changed, but the list of files may have. I think 
this would be safely covered if you moved VARDEPS_FILE as prereq to 
FILELIST.

I see your point. Will do that.

I was also a bit divided about whether the actual javac invocation 
rule should keep $$($1_SRCS) as a dependency. Technically, it's not 
needed, since changes in the sources will result in the filelist 
being updated, but maybe the intention will be better expressed. And 
if the set of files has not changed, then we will write the same 
files back to the filelist, which effectively just updates the 
timestamp in a roundabout way. If we keep the list of source files as 
dependency, we are free to implement a future optimization where the 
filelist is not updated if the set of files has not changed.


Otoh, I don't know if a huge list of dependencies (the number of 
sources files for e.g. java.base is non-trivial!) affects make 
performance, so listing it twice just to make it "look good" maybe 
isn't a good idea.


Do you have any opinion?

In general I like to list prereqs if they logically are direct 
prereqs. The sources are used as input in the compilation recipe, so 
that would be a direct prereq. If a prereq is really only transitive, 
then I don't want to list it. I don't know what the performance impact 
would be, I'm guessing it's not even measurable, but if it is, then 
maybe save some processing by not adding the sources to both rules. No 
strong opinion in this case.

Ok, I'll do it that way. I think we have generally the same preference here.

New webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8244844-make-javac-re-executable/webrev.02


/Magnus


/Erik


/Magnus


/Erik

Re: RFR: JDK-8244844 javac command line is not re-executable

[Erik Joelsson]

On 2020-05-12 10:08, Magnus Ihse Bursie wrote:

On 2020-05-12 17:38, Erik Joelsson wrote:

On 2020-05-12 08:29, Magnus Ihse Bursie wrote:
We've broken our golden rule in SetupJavaCompilation, that all 
command lines should be copy/paste:able to re-execute them from the 
command line. The reason is that the file containing the source code 
file names has a temporary name, which is renamed after the 
compilation. This is just a messy way to use the file both as input 
to javac and as a marker that the compilation has succeeded. By 
splitting up these two roles, we get more readable code and a 
re-executable command line.


Bug: https://bugs.openjdk.java.net/browse/JDK-8244844
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8244844-make-javac-re-executable/webrev.01


I really like seeing this fixed, but not sure about this patch. By 
moving the filelist to a separate rule and only having the source 
files as prereq, there will be cases where the source files 
timestamps have not changed, but the list of files may have. I think 
this would be safely covered if you moved VARDEPS_FILE as prereq to 
FILELIST.

I see your point. Will do that.

I was also a bit divided about whether the actual javac invocation 
rule should keep $$($1_SRCS) as a dependency. Technically, it's not 
needed, since changes in the sources will result in the filelist being 
updated, but maybe the intention will be better expressed. And if the 
set of files has not changed, then we will write the same files back 
to the filelist, which effectively just updates the timestamp in a 
roundabout way. If we keep the list of source files as dependency, we 
are free to implement a future optimization where the filelist is not 
updated if the set of files has not changed.


Otoh, I don't know if a huge list of dependencies (the number of 
sources files for e.g. java.base is non-trivial!) affects make 
performance, so listing it twice just to make it "look good" maybe 
isn't a good idea.


Do you have any opinion?

In general I like to list prereqs if they logically are direct prereqs. 
The sources are used as input in the compilation recipe, so that would 
be a direct prereq. If a prereq is really only transitive, then I don't 
want to list it. I don't know what the performance impact would be, I'm 
guessing it's not even measurable, but if it is, then maybe save some 
processing by not adding the sources to both rules. No strong opinion in 
this case.


/Erik


/Magnus


/Erik

Re: RFR: JDK-8244844 javac command line is not re-executable

[Magnus Ihse Bursie]

On 2020-05-12 17:38, Erik Joelsson wrote:

On 2020-05-12 08:29, Magnus Ihse Bursie wrote:
We've broken our golden rule in SetupJavaCompilation, that all 
command lines should be copy/paste:able to re-execute them from the 
command line. The reason is that the file containing the source code 
file names has a temporary name, which is renamed after the 
compilation. This is just a messy way to use the file both as input 
to javac and as a marker that the compilation has succeeded. By 
splitting up these two roles, we get more readable code and a 
re-executable command line.


Bug: https://bugs.openjdk.java.net/browse/JDK-8244844
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8244844-make-javac-re-executable/webrev.01


I really like seeing this fixed, but not sure about this patch. By 
moving the filelist to a separate rule and only having the source 
files as prereq, there will be cases where the source files timestamps 
have not changed, but the list of files may have. I think this would 
be safely covered if you moved VARDEPS_FILE as prereq to FILELIST.

I see your point. Will do that.

I was also a bit divided about whether the actual javac invocation rule 
should keep $$($1_SRCS) as a dependency. Technically, it's not needed, 
since changes in the sources will result in the filelist being updated, 
but maybe the intention will be better expressed. And if the set of 
files has not changed, then we will write the same files back to the 
filelist, which effectively just updates the timestamp in a roundabout 
way. If we keep the list of source files as dependency, we are free to 
implement a future optimization where the filelist is not updated if the 
set of files has not changed.


Otoh, I don't know if a huge list of dependencies (the number of sources 
files for e.g. java.base is non-trivial!) affects make performance, so 
listing it twice just to make it "look good" maybe isn't a good idea.


Do you have any opinion?

/Magnus


/Erik

Re: RFR: JDK-8244844 javac command line is not re-executable

[Erik Joelsson]

On 2020-05-12 08:29, Magnus Ihse Bursie wrote:
We've broken our golden rule in SetupJavaCompilation, that all command 
lines should be copy/paste:able to re-execute them from the command 
line. The reason is that the file containing the source code file 
names has a temporary name, which is renamed after the compilation. 
This is just a messy way to use the file both as input to javac and as 
a marker that the compilation has succeeded. By splitting up these two 
roles, we get more readable code and a re-executable command line.


Bug: https://bugs.openjdk.java.net/browse/JDK-8244844
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8244844-make-javac-re-executable/webrev.01


I really like seeing this fixed, but not sure about this patch. By 
moving the filelist to a separate rule and only having the source files 
as prereq, there will be cases where the source files timestamps have 
not changed, but the list of files may have. I think this would be 
safely covered if you moved VARDEPS_FILE as prereq to FILELIST.


/Erik

RFR: JDK-8244844 javac command line is not re-executable

[Magnus Ihse Bursie]

We've broken our golden rule in SetupJavaCompilation, that all command 
lines should be copy/paste:able to re-execute them from the command 
line. The reason is that the file containing the source code file names 
has a temporary name, which is renamed after the compilation. This is 
just a messy way to use the file both as input to javac and as a marker 
that the compilation has succeeded. By splitting up these two roles, we 
get more readable code and a re-executable command line.


Bug: https://bugs.openjdk.java.net/browse/JDK-8244844
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8244844-make-javac-re-executable/webrev.01


/Magnus

Re: RFR: JDK-8244757 Introduce SetupTarget in Main.gmk

[Magnus Ihse Bursie]

On 2020-05-12 14:52, Erik Joelsson wrote:

On 2020-05-12 00:35, Magnus Ihse Bursie wrote:

On 2020-05-11 19:42, Magnus Ihse Bursie wrote:

On 2020-05-11 18:16, Magnus Ihse Bursie wrote:
To be able to support JDK-8244410, we need to have a better way to 
handle dependencies. For this reason, the macro SetupTarget is 
introduced. It will take as input everything that is needed to 
create a top-level target in Main.gmk.


As a positive side-effect, it will finally once again be possible 
to list the dependencies for a rule together with the recipe, 
fixing a shortcoming that has plagues Main.gmk for a long time.


In this patch, I create a trivial version of SetupTarget, and 
switch trivial recipes to use this function. By this, I mean 
recipes that just delegate to another makefile, possible setting a 
specific target and/or make arguments. Specifically, this excludes 
generated target patterns, as for the module phases.


Trivial dependencies are also moved into this function. By this, I 
mean "literal" dependencies. Dependencies that are calculated using 
variables or macros might change if they are moved around in the 
file, and need a more thorough analysis before they can be moved.


My intention is to continue moving targets in Main.gmk into the 
scope of SetupTarget, but to avoid regressions and difficult code 
reviews, I will do this step by step.


Bug: https://bugs.openjdk.java.net/browse/JDK-8244757
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8244757-introduce-SetupTarget/webrev.01


... aand I forgot to add the TARGET argument after I split it 
out from ARGS. MainSupport.gmk is updated:


http://cr.openjdk.java.net/~ihse/JDK-8244757-introduce-SetupTarget/webrev.02/ 

And even this version was buggy. I apologize for the rushed work, it 
was definitely not up to my normal standard. :-(


Here's yet another version. This one fixes cross-compilation. And, as 
Erik made me aware off-list, it adds the conditional for evaluating 
dependencies in SetupTarget.


http://cr.openjdk.java.net/~ihse/JDK-8244757-introduce-SetupTarget/webrev.04 



Third time's a charm, they say.


Looks good. Missed indentation at the top of SetupTargetBody.

Oops. Thanks for catching that. Fixed before pushing.

/Magnus


/Erik

Re: [8u] RFR(XS): 8243059: Build fails when --with-vendor-name contains a comma

[Andrew Haley]

On 5/7/20 1:22 PM, Severin Gehwolf wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8243059
> webrev: 
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243059/jdk8/01/webrev/
> 
> Testing: building with --with-vendor-name="foo, bar". Fails before,
> passes after patch.
> 
> Thoughts?

OK, thanks.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: JDK-8244757 Introduce SetupTarget in Main.gmk

[Erik Joelsson]

On 2020-05-12 00:35, Magnus Ihse Bursie wrote:

On 2020-05-11 19:42, Magnus Ihse Bursie wrote:

On 2020-05-11 18:16, Magnus Ihse Bursie wrote:
To be able to support JDK-8244410, we need to have a better way to 
handle dependencies. For this reason, the macro SetupTarget is 
introduced. It will take as input everything that is needed to 
create a top-level target in Main.gmk.


As a positive side-effect, it will finally once again be possible to 
list the dependencies for a rule together with the recipe, fixing a 
shortcoming that has plagues Main.gmk for a long time.


In this patch, I create a trivial version of SetupTarget, and switch 
trivial recipes to use this function. By this, I mean recipes that 
just delegate to another makefile, possible setting a specific 
target and/or make arguments. Specifically, this excludes generated 
target patterns, as for the module phases.


Trivial dependencies are also moved into this function. By this, I 
mean "literal" dependencies. Dependencies that are calculated using 
variables or macros might change if they are moved around in the 
file, and need a more thorough analysis before they can be moved.


My intention is to continue moving targets in Main.gmk into the 
scope of SetupTarget, but to avoid regressions and difficult code 
reviews, I will do this step by step.


Bug: https://bugs.openjdk.java.net/browse/JDK-8244757
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8244757-introduce-SetupTarget/webrev.01


... aand I forgot to add the TARGET argument after I split it out 
from ARGS. MainSupport.gmk is updated:


http://cr.openjdk.java.net/~ihse/JDK-8244757-introduce-SetupTarget/webrev.02/ 

And even this version was buggy. I apologize for the rushed work, it 
was definitely not up to my normal standard. :-(


Here's yet another version. This one fixes cross-compilation. And, as 
Erik made me aware off-list, it adds the conditional for evaluating 
dependencies in SetupTarget.


http://cr.openjdk.java.net/~ihse/JDK-8244757-introduce-SetupTarget/webrev.04 



Third time's a charm, they say.


Looks good. Missed indentation at the top of SetupTargetBody.

/Erik

RE: RFR(XS): 8244756: Build broken with some awk version after JDK-8244248

[Doerr, Martin]

Thanks for the reviews! Pushed.

Best regards,
Martin


> -Original Message-
> From: Baesken, Matthias 
> Sent: Dienstag, 12. Mai 2020 11:01
> To: build-dev@openjdk.java.net
> Cc: Doerr, Martin 
> Subject: RE: RFR(XS): 8244756: Build broken with some awk version after JDK-
> 8244248
> 
> 
> Reviewed !
> 
> Best regards, Matthias
> 
> >thank you for proposing a fix which allows building with old awk on AIX.
> Works fine.
> >
> >Here's the webrev:
> >http://cr.openjdk.java.net/~mdoerr/8244756_AIX_fix_awk_expr/webrev.0
> 0/
> >
> 
> 
> 

RE: RFR(XS): 8244756: Build broken with some awk version after JDK-8244248

[Baesken, Matthias]

Reviewed !

Best regards, Matthias

>thank you for proposing a fix which allows building with old awk on AIX. Works 
>fine.
>
>Here's the webrev:
>http://cr.openjdk.java.net/~mdoerr/8244756_AIX_fix_awk_expr/webrev.00/
>

Re: RFR: JDK-8244757 Introduce SetupTarget in Main.gmk

[Magnus Ihse Bursie]

On 2020-05-11 19:42, Magnus Ihse Bursie wrote:

On 2020-05-11 18:16, Magnus Ihse Bursie wrote:
To be able to support JDK-8244410, we need to have a better way to 
handle dependencies. For this reason, the macro SetupTarget is 
introduced. It will take as input everything that is needed to create 
a top-level target in Main.gmk.


As a positive side-effect, it will finally once again be possible to 
list the dependencies for a rule together with the recipe, fixing a 
shortcoming that has plagues Main.gmk for a long time.


In this patch, I create a trivial version of SetupTarget, and switch 
trivial recipes to use this function. By this, I mean recipes that 
just delegate to another makefile, possible setting a specific target 
and/or make arguments. Specifically, this excludes generated target 
patterns, as for the module phases.


Trivial dependencies are also moved into this function. By this, I 
mean "literal" dependencies. Dependencies that are calculated using 
variables or macros might change if they are moved around in the 
file, and need a more thorough analysis before they can be moved.


My intention is to continue moving targets in Main.gmk into the scope 
of SetupTarget, but to avoid regressions and difficult code reviews, 
I will do this step by step.


Bug: https://bugs.openjdk.java.net/browse/JDK-8244757
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8244757-introduce-SetupTarget/webrev.01


... aand I forgot to add the TARGET argument after I split it out 
from ARGS. MainSupport.gmk is updated:


http://cr.openjdk.java.net/~ihse/JDK-8244757-introduce-SetupTarget/webrev.02/ 

And even this version was buggy. I apologize for the rushed work, it was 
definitely not up to my normal standard. :-(


Here's yet another version. This one fixes cross-compilation. And, as 
Erik made me aware off-list, it adds the conditional for evaluating 
dependencies in SetupTarget.


http://cr.openjdk.java.net/~ihse/JDK-8244757-introduce-SetupTarget/webrev.04

Third time's a charm, they say.

/Magnus




/Magnus



/Magnus