Re: RFR: JDK-8242302 : Refactor jpackage native code

[Alexey Semenyuk]

Thank you for the review!

Whitespace issues will be fixed in the final patch.

- Alexey

On 4/23/2020 2:51 PM, Erik Joelsson wrote:

Thanks, that looks good!

Only two minor whitespace nits.

Line 32-33, 102-103: continuation indent 4 spaces

Line 101: extra space

No need for new webrev.

/Erik

On 2020-04-23 11:38, Alexey Semenyuk wrote:

Erik,

Please review updated patch with the suggested improvements at [1]. 
The webrev includes only incremental changes to 
Lib-jdk.incubator.jpackage.gmk for clarity.


[1] 
http://cr.openjdk.java.net/~asemenyuk/8242302/webrev.01/webrev.01/index.html


- Alexey

On 4/20/2020 1:17 PM, Erik Joelsson wrote:

(adding build-dev)

Hello Alexey,

The SetupJdkExecutable and SetupJdkLibrary macros are designed to 
find the correct source dirs automatically as long as they follow 
the standard naming. In this case you are adding some extra with the 
"common" dir as well as reusing some src dirs between multiple calls 
to the macros. There are constructs prepared for handling these 
situations too. We introduced these specialized macros to avoid 
repeated setting up of almost identical source dirs like in your 
patch, and to help enforce a standard in directory naming.


Upon closer inspection, I see now that what's said above only 
applies to SetupJdkLibrary while SetupJdkExecutable is lacking most 
of these features. Adding them should be pretty straight forward 
though and should be fixed. Not asking you to do it though. So for 
those cases, I recommend using the FindSrcDirsForComponent macro to 
find the source dirs needed for now. It's defined in 
make/common/JdkNativeCompilation.gmk. You can also extract the exact 
resolved SRC dirs from a call to either macro using for example 
BUILD_JPACKAGE_APPLAUNCHEREXE_SRC (after the macro has been evaled). 
This would be recommended for BUILD_JPACKAGE_APPLAUNCHERWEXE.


For the ones where the NAME field matches the source sub dir (which 
should be almost all unless there is a good reason), you don't need 
to specify SRC at all. If you need to add the "common" subdir, then 
you can use EXTRA_SRC and the special designation 
"jdk.incubator.jpackage:common" which will get parsed into all 
existing common directories for your current build target. In the 
case of jpackageapplauncherw, you can similarly use 
"jdk.incubator.jpackage:applauncher" in the SRC field.


For SetupJdkLibrary, there is no need to explicitly add source dirs 
to -I paths. All source dirs are by default added unless 
HEADERS_FROM_SRC is set to false. Again this should be fixed for 
SetupJdkExecutable.


For macros that we intend to call with parameters, our style 
convention is to declare them with camel case and always on a new 
line. This is to make them stand out clearly from variables which we 
assign with :=, and make them look a little bit more like function 
declarations. See make/common/Utils.gmk for examples of how this 
looks. So something like:


JpackageWithStaticCrt = \
    $(filter-out -MD, $1) -MT

/Erik


On 2020-04-15 13:13, Alexey Semenyuk wrote:

Please review fix [2] for jpackage bug [1].

Refactor jpackage native code.

- Improve code reuse between different platforms.
- Replace custom string classes with the standard std::basic_string.
- Merge functionality of libapplauncher.dll(.so) library with 
jpackageapplauncher(.exe) binary. There is no point to keep two 
different executables.
- Link jpackageapplauncher.exe with MSVC Run-Time library 
statically to avoid copying of MSVC Run-Time dlls to app's bin folder.

- Remove unused code.

- Alexey

[1] https://bugs.openjdk.java.net/browse/JDK-8242302

[2] http://cr.openjdk.java.net/~asemenyuk/8242302/webrev.00

Re: RFR: JDK-8242302 : Refactor jpackage native code

[Erik Joelsson]

Thanks, that looks good!

Only two minor whitespace nits.

Line 32-33, 102-103: continuation indent 4 spaces

Line 101: extra space

No need for new webrev.

/Erik

On 2020-04-23 11:38, Alexey Semenyuk wrote:

Erik,

Please review updated patch with the suggested improvements at [1]. 
The webrev includes only incremental changes to 
Lib-jdk.incubator.jpackage.gmk for clarity.


[1] 
http://cr.openjdk.java.net/~asemenyuk/8242302/webrev.01/webrev.01/index.html


- Alexey

On 4/20/2020 1:17 PM, Erik Joelsson wrote:

(adding build-dev)

Hello Alexey,

The SetupJdkExecutable and SetupJdkLibrary macros are designed to 
find the correct source dirs automatically as long as they follow the 
standard naming. In this case you are adding some extra with the 
"common" dir as well as reusing some src dirs between multiple calls 
to the macros. There are constructs prepared for handling these 
situations too. We introduced these specialized macros to avoid 
repeated setting up of almost identical source dirs like in your 
patch, and to help enforce a standard in directory naming.


Upon closer inspection, I see now that what's said above only applies 
to SetupJdkLibrary while SetupJdkExecutable is lacking most of these 
features. Adding them should be pretty straight forward though and 
should be fixed. Not asking you to do it though. So for those cases, 
I recommend using the FindSrcDirsForComponent macro to find the 
source dirs needed for now. It's defined in 
make/common/JdkNativeCompilation.gmk. You can also extract the exact 
resolved SRC dirs from a call to either macro using for example 
BUILD_JPACKAGE_APPLAUNCHEREXE_SRC (after the macro has been evaled). 
This would be recommended for BUILD_JPACKAGE_APPLAUNCHERWEXE.


For the ones where the NAME field matches the source sub dir (which 
should be almost all unless there is a good reason), you don't need 
to specify SRC at all. If you need to add the "common" subdir, then 
you can use EXTRA_SRC and the special designation 
"jdk.incubator.jpackage:common" which will get parsed into all 
existing common directories for your current build target. In the 
case of jpackageapplauncherw, you can similarly use 
"jdk.incubator.jpackage:applauncher" in the SRC field.


For SetupJdkLibrary, there is no need to explicitly add source dirs 
to -I paths. All source dirs are by default added unless 
HEADERS_FROM_SRC is set to false. Again this should be fixed for 
SetupJdkExecutable.


For macros that we intend to call with parameters, our style 
convention is to declare them with camel case and always on a new 
line. This is to make them stand out clearly from variables which we 
assign with :=, and make them look a little bit more like function 
declarations. See make/common/Utils.gmk for examples of how this 
looks. So something like:


JpackageWithStaticCrt = \
    $(filter-out -MD, $1) -MT

/Erik


On 2020-04-15 13:13, Alexey Semenyuk wrote:

Please review fix [2] for jpackage bug [1].

Refactor jpackage native code.

- Improve code reuse between different platforms.
- Replace custom string classes with the standard std::basic_string.
- Merge functionality of libapplauncher.dll(.so) library with 
jpackageapplauncher(.exe) binary. There is no point to keep two 
different executables.
- Link jpackageapplauncher.exe with MSVC Run-Time library statically 
to avoid copying of MSVC Run-Time dlls to app's bin folder.

- Remove unused code.

- Alexey

[1] https://bugs.openjdk.java.net/browse/JDK-8242302

[2] http://cr.openjdk.java.net/~asemenyuk/8242302/webrev.00

Re: RFR: JDK-8242302 : Refactor jpackage native code

[Alexey Semenyuk]

Erik,

Please review updated patch with the suggested improvements at [1]. The 
webrev includes only incremental changes to 
Lib-jdk.incubator.jpackage.gmk for clarity.


[1] 
http://cr.openjdk.java.net/~asemenyuk/8242302/webrev.01/webrev.01/index.html


- Alexey

On 4/20/2020 1:17 PM, Erik Joelsson wrote:

(adding build-dev)

Hello Alexey,

The SetupJdkExecutable and SetupJdkLibrary macros are designed to find 
the correct source dirs automatically as long as they follow the 
standard naming. In this case you are adding some extra with the 
"common" dir as well as reusing some src dirs between multiple calls 
to the macros. There are constructs prepared for handling these 
situations too. We introduced these specialized macros to avoid 
repeated setting up of almost identical source dirs like in your 
patch, and to help enforce a standard in directory naming.


Upon closer inspection, I see now that what's said above only applies 
to SetupJdkLibrary while SetupJdkExecutable is lacking most of these 
features. Adding them should be pretty straight forward though and 
should be fixed. Not asking you to do it though. So for those cases, I 
recommend using the FindSrcDirsForComponent macro to find the source 
dirs needed for now. It's defined in 
make/common/JdkNativeCompilation.gmk. You can also extract the exact 
resolved SRC dirs from a call to either macro using for example 
BUILD_JPACKAGE_APPLAUNCHEREXE_SRC (after the macro has been evaled). 
This would be recommended for BUILD_JPACKAGE_APPLAUNCHERWEXE.


For the ones where the NAME field matches the source sub dir (which 
should be almost all unless there is a good reason), you don't need to 
specify SRC at all. If you need to add the "common" subdir, then you 
can use EXTRA_SRC and the special designation 
"jdk.incubator.jpackage:common" which will get parsed into all 
existing common directories for your current build target. In the case 
of jpackageapplauncherw, you can similarly use 
"jdk.incubator.jpackage:applauncher" in the SRC field.


For SetupJdkLibrary, there is no need to explicitly add source dirs to 
-I paths. All source dirs are by default added unless HEADERS_FROM_SRC 
is set to false. Again this should be fixed for SetupJdkExecutable.


For macros that we intend to call with parameters, our style 
convention is to declare them with camel case and always on a new 
line. This is to make them stand out clearly from variables which we 
assign with :=, and make them look a little bit more like function 
declarations. See make/common/Utils.gmk for examples of how this 
looks. So something like:


JpackageWithStaticCrt = \
    $(filter-out -MD, $1) -MT

/Erik


On 2020-04-15 13:13, Alexey Semenyuk wrote:

Please review fix [2] for jpackage bug [1].

Refactor jpackage native code.

- Improve code reuse between different platforms.
- Replace custom string classes with the standard std::basic_string.
- Merge functionality of libapplauncher.dll(.so) library with 
jpackageapplauncher(.exe) binary. There is no point to keep two 
different executables.
- Link jpackageapplauncher.exe with MSVC Run-Time library statically 
to avoid copying of MSVC Run-Time dlls to app's bin folder.

- Remove unused code.

- Alexey

[1] https://bugs.openjdk.java.net/browse/JDK-8242302

[2] http://cr.openjdk.java.net/~asemenyuk/8242302/webrev.00

Re: mistriggered "error: warnings found and -Werror specified" for java warnings

[Magnus Ihse Bursie]

On 2020-04-23 17:17, Igor Ignatyev wrote:

Hi Matthias,


jtharness 6.0-b10 and jtreg 5.0-b1.


jtreg 5.0-b1 "officially" depends on jt6.0-b08, and the bundle which 
we use internally has jt6.0-b08 within it, and AFAICT jt6.0-b08 hasn't 
deprecated c.s.j.H.Observer::finishedTesting yet. so as a temp. 
solution to allow configurations like yours, we need to suppress 
deprecation warning in failure handler, that's if we decide that such 
configurations are "supported", which isn't that obvious as you might 
encounter some deviations in jtreg behavior   b/c another version of 
its dependency is used, so I'd encourage you to use the exact same 
version as used by jtreg build script[1].
From my point of view, I think we should have a very narrow range of 
supported jtreg versions. It's not like when building with the system 
zlib; this is a framework solely used for testing the JDK, and I think 
we can (and should) be quite strict as to which versions the current 
code base is designed to work with.


Things get a little bit muddier if the version of jtreg matches, but not 
an upstream dependency for jtreg. I still tend to lean towards Igor's 
suggestion here, that a properly setup jtreg environment is the one 
officially built by jtreg.


/Magnus




a proper solution would include switching jtreg to newer version of 
jt-harness (which implies adjustment of jtreg and subsequently 
testing), promotion/tagging of newer jtreg build, switching to newer 
jtreg in jdk and updating in failurehandler.


Thanks,
-- Igor

[1] 
http://hg.openjdk.java.net/code-tools/jtreg/file/fc37a1d7f0ea/make/build-all.sh#l129


On Apr 23, 2020, at 7:48 AM, Matthias Klose > wrote:


On 4/23/20 4:05 PM, Magnus Ihse Bursie wrote:


23 apr. 2020 kl. 15:50 skrev Igor Ignatyev 
mailto:igor.ignat...@oracle.com>>:




On Apr 23, 2020, at 6:12 AM, Erik Joelsson 
mailto:erik.joels...@oracle.com>> wrote:


Hello Matthias,


On 2020-04-23 05:51, Matthias Klose wrote:
jdk-15+20 fails to build with

* For target 
support_test_failure_handler_classes__the.BUILD_FAILURE_HANDLER_batch:

/packages/openjdk/15/openjdk-15-15~20/test/failure_handler/src/share/classes/jdk/test/failurehandler/jtreg/GatherDiagnosticInfoObserver.java:136:
warning: [deprecation] finishedTesting() in Observer has been 
deprecated

  public void finishedTesting() {
  ^
error: warnings found and -Werror specified
1 error
1 warning
That's strange. I assume this tool is built with the boot JDK, so 
that makes me wonder what boot JDK you are using as we have not 
seen this warning/error?


that's with 14.0.1+7.

I guess version of jtreg/jt-harness is more relevant here as 
deprecated finishedTesting is from com.sun.javatest.Harness.Observer.


Aha, that’s probably the explanation. I recently removed deprecation 
as a disabled warning for the failure handler. If it depends on 
jtreg, and it has changed deprecation status, that might trigger a 
compilation warning.


I’m on mobile now and can’t check how this should be resolved.

If a newer version of jtreg introduced the depreciation, we should 
fix our sources. If this is something only present in older sources 
(?) we might need to raise the minimum jtreg version.


jtharness 6.0-b10 and jtreg 5.0-b1.



/Magnus



--Igor



Apparently --disable-warnings-as-errors only has an effect on 
C/C++ files,
however the build diagnostics trigger on java warnings as well, 
and apparently

-Werror is hard-coded in various places for java options. Should the
documentation for this configure option be clarified, or should 
it trigger for

java warnings as well?


Correct. The reasoning is that OpenJDK is built on a wide variety 
of environments with different compiler versions, so keeping the 
build warning free on all of them isn't feasible. This option 
makes it possible to build with all those different compiler 
versions while still maintaining a warning free source for a core 
set of compiler versions. In contrast, the Java code should only 
be compiled with a very small set of javac versions, which should 
be easily controlled. The majority of the code is even compiled 
with the Javac we are building. We have contemplated a similar 
option for Java code, but concluded that it doesn't serve any 
purpose. The Java source should just always be warning free.


/Erik

Re: [11u] RFR: 8243059: Build fails when --with-vendor-name contains a comma

[Andrew Haley]

On 4/22/20 6:00 PM, Severin Gehwolf wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8243059
> webrev: 
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243059/jdk11/01/webrev/
> original change: https://hg.openjdk.java.net/jdk/jdk/rev/dff3aabdc2f3
> 
> Testing: Build fails before, passes after patch. java.vendor property
> is as expected when it includes commas.

Sure, 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: mistriggered "error: warnings found and -Werror specified" for java warnings

[Igor Ignatyev]

Hi Matthias,

> jtharness 6.0-b10 and jtreg 5.0-b1.

jtreg 5.0-b1 "officially" depends on jt6.0-b08, and the bundle which we use 
internally has jt6.0-b08 within it, and AFAICT jt6.0-b08 hasn't deprecated 
c.s.j.H.Observer::finishedTesting yet. so as a temp. solution to allow 
configurations like yours, we need to suppress deprecation warning in failure 
handler, that's if we decide that such configurations are "supported", which 
isn't that obvious as you might encounter some deviations in jtreg behavior   
b/c another version of its dependency is used, so I'd encourage you to use the 
exact same version as used by jtreg build script[1].

a proper solution would include switching jtreg to newer version of jt-harness 
(which implies adjustment of jtreg and subsequently testing), promotion/tagging 
of newer jtreg build, switching to newer jtreg in jdk and updating in 
failurehandler.

Thanks,
-- Igor 

[1] 
http://hg.openjdk.java.net/code-tools/jtreg/file/fc37a1d7f0ea/make/build-all.sh#l129
 


> On Apr 23, 2020, at 7:48 AM, Matthias Klose  wrote:
> 
> On 4/23/20 4:05 PM, Magnus Ihse Bursie wrote:
>> 
>>> 23 apr. 2020 kl. 15:50 skrev Igor Ignatyev :
>>> 
>>> 
>>> 
 On Apr 23, 2020, at 6:12 AM, Erik Joelsson  
 wrote:
 
 Hello Matthias,
 
> On 2020-04-23 05:51, Matthias Klose wrote:
> jdk-15+20 fails to build with
> 
> * For target 
> support_test_failure_handler_classes__the.BUILD_FAILURE_HANDLER_batch:
> /packages/openjdk/15/openjdk-15-15~20/test/failure_handler/src/share/classes/jdk/test/failurehandler/jtreg/GatherDiagnosticInfoObserver.java:136:
> warning: [deprecation] finishedTesting() in Observer has been deprecated
>   public void finishedTesting() {
>   ^
> error: warnings found and -Werror specified
> 1 error
> 1 warning
 That's strange. I assume this tool is built with the boot JDK, so that 
 makes me wonder what boot JDK you are using as we have not seen this 
 warning/error?
> 
> that's with 14.0.1+7.
> 
>>> I guess version of jtreg/jt-harness is more relevant here as deprecated 
>>> finishedTesting is from com.sun.javatest.Harness.Observer.
>> 
>> Aha, that’s probably the explanation. I recently removed deprecation as a 
>> disabled warning for the failure handler. If it depends on jtreg, and it has 
>> changed deprecation status, that might trigger a compilation warning.
>> 
>> I’m on mobile now and can’t check how this should be resolved. 
>> 
>> If a newer version of jtreg introduced the depreciation, we should fix our 
>> sources. If this is something only present in older sources (?) we might 
>> need to raise the minimum jtreg version. 
> 
> jtharness 6.0-b10 and jtreg 5.0-b1.
> 
>> 
>> /Magnus 
>> 
>>> 
>>> --Igor
>>> 
> 
> Apparently --disable-warnings-as-errors only has an effect on C/C++ files,
> however the build diagnostics trigger on java warnings as well, and 
> apparently
> -Werror is hard-coded in various places for java options. Should the
> documentation for this configure option be clarified, or should it 
> trigger for
> java warnings as well?
 
 Correct. The reasoning is that OpenJDK is built on a wide variety of 
 environments with different compiler versions, so keeping the build 
 warning free on all of them isn't feasible. This option makes it possible 
 to build with all those different compiler versions while still 
 maintaining a warning free source for a core set of compiler versions. In 
 contrast, the Java code should only be compiled with a very small set of 
 javac versions, which should be easily controlled. The majority of the 
 code is even compiled with the Javac we are building. We have contemplated 
 a similar option for Java code, but concluded that it doesn't serve any 
 purpose. The Java source should just always be warning free.
 
 /Erik

Re: mistriggered "error: warnings found and -Werror specified" for java warnings

[Matthias Klose]

On 4/23/20 4:05 PM, Magnus Ihse Bursie wrote:
> 
>> 23 apr. 2020 kl. 15:50 skrev Igor Ignatyev :
>>
>>
>>
>>> On Apr 23, 2020, at 6:12 AM, Erik Joelsson  wrote:
>>>
>>> Hello Matthias,
>>>
 On 2020-04-23 05:51, Matthias Klose wrote:
 jdk-15+20 fails to build with

 * For target 
 support_test_failure_handler_classes__the.BUILD_FAILURE_HANDLER_batch:
 /packages/openjdk/15/openjdk-15-15~20/test/failure_handler/src/share/classes/jdk/test/failurehandler/jtreg/GatherDiagnosticInfoObserver.java:136:
 warning: [deprecation] finishedTesting() in Observer has been deprecated
public void finishedTesting() {
^
 error: warnings found and -Werror specified
 1 error
 1 warning
>>> That's strange. I assume this tool is built with the boot JDK, so that 
>>> makes me wonder what boot JDK you are using as we have not seen this 
>>> warning/error?

that's with 14.0.1+7.

>> I guess version of jtreg/jt-harness is more relevant here as deprecated 
>> finishedTesting is from com.sun.javatest.Harness.Observer.
> 
> Aha, that’s probably the explanation. I recently removed deprecation as a 
> disabled warning for the failure handler. If it depends on jtreg, and it has 
> changed deprecation status, that might trigger a compilation warning.
> 
> I’m on mobile now and can’t check how this should be resolved. 
> 
> If a newer version of jtreg introduced the depreciation, we should fix our 
> sources. If this is something only present in older sources (?) we might need 
> to raise the minimum jtreg version. 

jtharness 6.0-b10 and jtreg 5.0-b1.

> 
> /Magnus 
> 
>>
>> --Igor
>>

 Apparently --disable-warnings-as-errors only has an effect on C/C++ files,
 however the build diagnostics trigger on java warnings as well, and 
 apparently
 -Werror is hard-coded in various places for java options. Should the
 documentation for this configure option be clarified, or should it trigger 
 for
 java warnings as well?
>>>
>>> Correct. The reasoning is that OpenJDK is built on a wide variety of 
>>> environments with different compiler versions, so keeping the build warning 
>>> free on all of them isn't feasible. This option makes it possible to build 
>>> with all those different compiler versions while still maintaining a 
>>> warning free source for a core set of compiler versions. In contrast, the 
>>> Java code should only be compiled with a very small set of javac versions, 
>>> which should be easily controlled. The majority of the code is even 
>>> compiled with the Javac we are building. We have contemplated a similar 
>>> option for Java code, but concluded that it doesn't serve any purpose. The 
>>> Java source should just always be warning free.
>>>
>>> /Erik
>>>
>>
> 

Re: mistriggered "error: warnings found and -Werror specified" for java warnings

[Magnus Ihse Bursie]

> 23 apr. 2020 kl. 15:50 skrev Igor Ignatyev :
> 
> 
> 
>> On Apr 23, 2020, at 6:12 AM, Erik Joelsson  wrote:
>> 
>> Hello Matthias,
>> 
>>> On 2020-04-23 05:51, Matthias Klose wrote:
>>> jdk-15+20 fails to build with
>>> 
>>> * For target 
>>> support_test_failure_handler_classes__the.BUILD_FAILURE_HANDLER_batch:
>>> /packages/openjdk/15/openjdk-15-15~20/test/failure_handler/src/share/classes/jdk/test/failurehandler/jtreg/GatherDiagnosticInfoObserver.java:136:
>>> warning: [deprecation] finishedTesting() in Observer has been deprecated
>>>public void finishedTesting() {
>>>^
>>> error: warnings found and -Werror specified
>>> 1 error
>>> 1 warning
>> That's strange. I assume this tool is built with the boot JDK, so that makes 
>> me wonder what boot JDK you are using as we have not seen this warning/error?
> I guess version of jtreg/jt-harness is more relevant here as deprecated 
> finishedTesting is from com.sun.javatest.Harness.Observer.

Aha, that’s probably the explanation. I recently removed deprecation as a 
disabled warning for the failure handler. If it depends on jtreg, and it has 
changed deprecation status, that might trigger a compilation warning.

I’m on mobile now and can’t check how this should be resolved. 

If a newer version of jtreg introduced the depreciation, we should fix our 
sources. If this is something only present in older sources (?) we might need 
to raise the minimum jtreg version. 

/Magnus 

> 
> --Igor
> 
>>> 
>>> Apparently --disable-warnings-as-errors only has an effect on C/C++ files,
>>> however the build diagnostics trigger on java warnings as well, and 
>>> apparently
>>> -Werror is hard-coded in various places for java options. Should the
>>> documentation for this configure option be clarified, or should it trigger 
>>> for
>>> java warnings as well?
>> 
>> Correct. The reasoning is that OpenJDK is built on a wide variety of 
>> environments with different compiler versions, so keeping the build warning 
>> free on all of them isn't feasible. This option makes it possible to build 
>> with all those different compiler versions while still maintaining a warning 
>> free source for a core set of compiler versions. In contrast, the Java code 
>> should only be compiled with a very small set of javac versions, which 
>> should be easily controlled. The majority of the code is even compiled with 
>> the Javac we are building. We have contemplated a similar option for Java 
>> code, but concluded that it doesn't serve any purpose. The Java source 
>> should just always be warning free.
>> 
>> /Erik
>> 
> 

Re: mistriggered "error: warnings found and -Werror specified" for java warnings

[Igor Ignatyev]

> On Apr 23, 2020, at 6:12 AM, Erik Joelsson  wrote:
> 
> Hello Matthias,
> 
> On 2020-04-23 05:51, Matthias Klose wrote:
>> jdk-15+20 fails to build with
>> 
>> * For target 
>> support_test_failure_handler_classes__the.BUILD_FAILURE_HANDLER_batch:
>> /packages/openjdk/15/openjdk-15-15~20/test/failure_handler/src/share/classes/jdk/test/failurehandler/jtreg/GatherDiagnosticInfoObserver.java:136:
>> warning: [deprecation] finishedTesting() in Observer has been deprecated
>> public void finishedTesting() {
>> ^
>> error: warnings found and -Werror specified
>> 1 error
>> 1 warning
> That's strange. I assume this tool is built with the boot JDK, so that makes 
> me wonder what boot JDK you are using as we have not seen this warning/error?
I guess version of jtreg/jt-harness is more relevant here as deprecated 
finishedTesting is from com.sun.javatest.Harness.Observer.

--Igor

>> 
>> Apparently --disable-warnings-as-errors only has an effect on C/C++ files,
>> however the build diagnostics trigger on java warnings as well, and 
>> apparently
>> -Werror is hard-coded in various places for java options. Should the
>> documentation for this configure option be clarified, or should it trigger 
>> for
>> java warnings as well?
> 
> Correct. The reasoning is that OpenJDK is built on a wide variety of 
> environments with different compiler versions, so keeping the build warning 
> free on all of them isn't feasible. This option makes it possible to build 
> with all those different compiler versions while still maintaining a warning 
> free source for a core set of compiler versions. In contrast, the Java code 
> should only be compiled with a very small set of javac versions, which should 
> be easily controlled. The majority of the code is even compiled with the 
> Javac we are building. We have contemplated a similar option for Java code, 
> but concluded that it doesn't serve any purpose. The Java source should just 
> always be warning free.
> 
> /Erik
> 

Re: mistriggered "error: warnings found and -Werror specified" for java warnings

[Erik Joelsson]

Hello Matthias,

On 2020-04-23 05:51, Matthias Klose wrote:

jdk-15+20 fails to build with

* For target 
support_test_failure_handler_classes__the.BUILD_FAILURE_HANDLER_batch:
/packages/openjdk/15/openjdk-15-15~20/test/failure_handler/src/share/classes/jdk/test/failurehandler/jtreg/GatherDiagnosticInfoObserver.java:136:
warning: [deprecation] finishedTesting() in Observer has been deprecated
 public void finishedTesting() {
 ^
error: warnings found and -Werror specified
1 error
1 warning
That's strange. I assume this tool is built with the boot JDK, so that 
makes me wonder what boot JDK you are using as we have not seen this 
warning/error?


Apparently --disable-warnings-as-errors only has an effect on C/C++ files,
however the build diagnostics trigger on java warnings as well, and apparently
-Werror is hard-coded in various places for java options. Should the
documentation for this configure option be clarified, or should it trigger for
java warnings as well?


Correct. The reasoning is that OpenJDK is built on a wide variety of 
environments with different compiler versions, so keeping the build 
warning free on all of them isn't feasible. This option makes it 
possible to build with all those different compiler versions while still 
maintaining a warning free source for a core set of compiler versions. 
In contrast, the Java code should only be compiled with a very small set 
of javac versions, which should be easily controlled. The majority of 
the code is even compiled with the Javac we are building. We have 
contemplated a similar option for Java code, but concluded that it 
doesn't serve any purpose. The Java source should just always be warning 
free.


/Erik

Re: RFR: JDK-8243477 FreeType library check should prefer 64-bit directory

[Erik Joelsson]

Looks good.

/Erik

On 2020-04-23 04:32, Magnus Ihse Bursie wrote:
The check for the system freetype library should check the 64-bit 
library first on 64-bit systems, before falling back to the generic 
"lib" directory.


When fixing this, I noticed that lib-freetype.m4 needed a lot of love, 
so I shaped it up a bit. Changes made to code quality:

 * Indentation and spacing errors fixed
 * All tests were executed in subshells, which is not necessary. The 
original author presumably did not know what the "if (test ...) " 
shell syntax means.
 * Several lines were far too long. While the file is still not 
perfect, it's much better. (And we've never been zealous about the 80 
char length limit anyway.)


The only "real" change in the file was that I moved the check for 
$OPENJDK_TARGET_CPU_BITS to be the first of the well-known location 
tests.


Bug: https://bugs.openjdk.java.net/browse/JDK-8243477
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8243477-fix-freetype-lib64/webrev.01


/Magnus

mistriggered "error: warnings found and -Werror specified" for java warnings

[Matthias Klose]

jdk-15+20 fails to build with

* For target 
support_test_failure_handler_classes__the.BUILD_FAILURE_HANDLER_batch:
/packages/openjdk/15/openjdk-15-15~20/test/failure_handler/src/share/classes/jdk/test/failurehandler/jtreg/GatherDiagnosticInfoObserver.java:136:
warning: [deprecation] finishedTesting() in Observer has been deprecated
public void finishedTesting() {
^
error: warnings found and -Werror specified
1 error
1 warning


Apparently --disable-warnings-as-errors only has an effect on C/C++ files,
however the build diagnostics trigger on java warnings as well, and apparently
-Werror is hard-coded in various places for java options. Should the
documentation for this configure option be clarified, or should it trigger for
java warnings as well?

Matthias

RFR: JDK-8243477 FreeType library check should prefer 64-bit directory

[Magnus Ihse Bursie]

The check for the system freetype library should check the 64-bit 
library first on 64-bit systems, before falling back to the generic 
"lib" directory.


When fixing this, I noticed that lib-freetype.m4 needed a lot of love, 
so I shaped it up a bit. Changes made to code quality:

 * Indentation and spacing errors fixed
 * All tests were executed in subshells, which is not necessary. The 
original author presumably did not know what the "if (test ...) " shell 
syntax means.
 * Several lines were far too long. While the file is still not 
perfect, it's much better. (And we've never been zealous about the 80 
char length limit anyway.)


The only "real" change in the file was that I moved the check for 
$OPENJDK_TARGET_CPU_BITS to be the first of the well-known location tests.


Bug: https://bugs.openjdk.java.net/browse/JDK-8243477
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8243477-fix-freetype-lib64/webrev.01


/Magnus

Re: RFR: 8239357: Revert gcc implementation of offset_of

[Kim Barrett]

> On Apr 22, 2020, at 9:28 AM, Erik Joelsson  wrote:
> 
> Build change looks good.

Thanks.

> 
> /Erik
> 
> On 2020-04-22 06:23, Kim Barrett wrote:
>> Please review this change to undo the (small) portion of JDK-8238281
>> [1] related to the HotSpot offset_of macro for gcc/clang.  That change
>> should not have been made.  See CR for details.
>> 
>> I did not reinstate the redefinition of offsetof; we shouldn't be
>> using that macro, and having the warning enabled will catch
>> questionable cases.  (I also considered but didn’t poison it.)
>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8239357
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8238281
>> 
>> Webrev:
>> https://cr.openjdk.java.net/~kbarrett/8239357/open.00/
>> 
>> Testing:
>> mach5 tier1
>> 
>> With a temporary change to use offsetof with a non-standard-layout class,
>> verified the expected build failures occur with gcc and clang.

Re: RFR: 8239357: Revert gcc implementation of offset_of

[Kim Barrett]

> On Apr 22, 2020, at 9:57 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2020-04-22 15:23, Kim Barrett wrote:
>> Please review this change to undo the (small) portion of JDK-8238281
>> [1] related to the HotSpot offset_of macro for gcc/clang.  That change
>> should not have been made.  See CR for details.
>> 
>> I did not reinstate the redefinition of offsetof; we shouldn't be
>> using that macro, and having the warning enabled will catch
>> questionable cases.  (I also considered but didn’t poison it.)
>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8239357
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8238281
>> 
>> Webrev:
>> https://cr.openjdk.java.net/~kbarrett/8239357/open.00/
> Looks good to me.
> 
> /Magnus

Thanks.

>> 
>> Testing:
>> mach5 tier1
>> 
>> With a temporary change to use offsetof with a non-standard-layout class,
>> verified the expected build failures occur with gcc and clang.

Re: RFR: 8239357: Revert gcc implementation of offset_of

[Kim Barrett]

> On Apr 22, 2020, at 10:33 PM, David Holmes  wrote:
> 
> On 22/04/2020 11:23 pm, Kim Barrett wrote:
>> Please review this change to undo the (small) portion of JDK-8238281
>> [1] related to the HotSpot offset_of macro for gcc/clang.  That change
>> should not have been made.  See CR for details.
>> I did not reinstate the redefinition of offsetof; we shouldn't be
>> using that macro, and having the warning enabled will catch
>> questionable cases.  (I also considered but didn’t poison it.)
> 
> Okay. This seems reasonable.
> 
> Thanks,
> David

Thanks.

> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8239357
>> [1] https://bugs.openjdk.java.net/browse/JDK-8238281
>> Webrev:
>> https://cr.openjdk.java.net/~kbarrett/8239357/open.00/
>> Testing:
>> mach5 tier1
>> With a temporary change to use offsetof with a non-standard-layout class,
>> verified the expected build failures occur with gcc and clang.