Re: RFR: JDK-8288094: cleanup old _MSC_VER handling

[Phil Race]

On Thu, 9 Jun 2022 11:37:21 GMT, Matthias Baesken  wrote:

> We still handle at a number of places ancient historic _MSC_VER versions of 
> Visual Studio releases e.g. pre VS2013 (VS2013 has _MSC_VER 1800).
> This should be cleaned up, as long as it is not 3rd party code that we don't 
> want to adjust.
> 
> Currently still supported ("valid") VS version are 2017+, see 
> https://github.com/openjdk/jdk/blob/master/make/autoconf/toolchain_microsoft.m4
>  .
> VALID_VS_VERSIONS="2019 2017 2022"
> Even with adjusted toolchain m4 files, something older than VS2013 (also 
> probably older than VS2015) would not be able to build jdk anymore.

src/java.desktop/windows/native/libawt/windows/ThemeReader.cpp line 38:

> 36: #  define ROUND_TO_INT(num)((int) round(num))
> 37: #else
> 38: #  define ROUND_TO_INT(num)((int) floor((num) + 0.5))

If you look at when and why this was introduced (*), the "else" was not to 
support some other compiler - it was to support the older MS compiler. So if 
you don't want that, then the whole thing reduces to
#define ROUND_TO_INT(num) ((int) round(num))

.. you could even go further and eliminate the macro altogether if it makes 
sense - you'd have to look at the usages.

Same logic applies to the other files.


(*) https://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010889.html

-

PR: https://git.openjdk.org/jdk/pull/9105

Re: RFR: 8287137: Problemlist failing x86_32 tests after Loom integration

[Phil Race]

On Mon, 23 May 2022 12:28:30 GMT, Aleksey Shipilev  wrote:

> [JDK-8284161](https://bugs.openjdk.java.net/browse/JDK-8284161) broke a lot 
> of x86_32 code. The x86_32 porting is done under 
> [JDK-8286642](https://bugs.openjdk.java.net/browse/JDK-8286642). Meanwhile, 
> we can problemlist the failing tests to get cleaner runs for other patches. 
> This should also make GHA runs cleaner.
> 
> Additional testing:
>  - [x] Linux x86_32 fastdebug `tier1` (some unrelated failures in hotspot)
>  - [x] Linux x86_32 fastdebug `tier2` (some unrelated failures in hotspot)
>  - [x] Linux x86_32 fastdebug `tier3` (clean now)
>  - [ ] GHA run

I agree this is better than disabling the entire GHA

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8843

Re: RFR: 8284213: Replace usages of 'a the' in xml

[Phil Race]

On Wed, 18 May 2022 13:58:22 GMT, Alexey Ivanov  wrote:

> Replaces usages of articles that follow each other in all combinations: 
> a/the, an?/an?, the/the…
> 
> I tried to avoid changing external libraries, there are quite a few such 
> typos.
> Let me know if I should revert any files.

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8769

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Phil Race]

On Thu, 12 May 2022 00:26:41 GMT, Yasumasa Suenaga  wrote:

>> I don't understand what the actual warning is getting at .. can anyone 
>> explain it ?
>> 
>> FWIW the code is still the same in upstream harfbuzz
>> https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-font.cc
>
> I've pasted a part of warning messages to description of this PR, all of 
> messages are here:
> 
> 
> In function 'void trampoline_reference(hb_trampoline_closure_t*)',
> inlined from 'void hb_font_funcs_set_glyph_func(hb_font_funcs_t*, 
> hb_font_get_glyph_func_t, void*, hb_destroy_func_t)' at 
> /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2368:24:

So upstream say it is not a problem since the analysis overlooks that it would 
not reach that free if it were immutable because of a previous check. I guess 
we disable the false positive warning for now.

-

PR: https://git.openjdk.java.net/jdk/pull/8646

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]

[Phil Race]

On Fri, 13 May 2022 10:02:30 GMT, Yasumasa Suenaga  wrote:

>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should 
>> separate them by area.
>> 
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> * 
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>> 
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool* 
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Revert change for java.c , parse_manifest.c , LinuxPackage.c

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8646

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v4]

[Phil Race]

On Thu, 12 May 2022 01:27:30 GMT, Yasumasa Suenaga  wrote:

>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should 
>> separate them by area.
>> 
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> * 
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>> 
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool* 
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Calculate char offset before realloc()

I will see what upstream thinks about the harfbuzz warning but in the mean time 
we can just disable it.

-

PR: https://git.openjdk.java.net/jdk/pull/8646

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Phil Race]

On Wed, 11 May 2022 13:35:00 GMT, Kim Barrett  wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Avoid pragma error in before GCC 12
>
> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 462:
> 
>> 460:HARFBUZZ_DISABLED_WARNINGS_gcc := type-limits 
>> missing-field-initializers strict-aliasing
>> 461:HARFBUZZ_DISABLED_WARNINGS_CXX_gcc := reorder 
>> delete-non-virtual-dtor strict-overflow \
>> 462: maybe-uninitialized class-memaccess unused-result extra 
>> use-after-free
> 
> Globally disabling use-after-free warnings for this package seems really
> questionable. If these are problems in the code, just suppressing the warning
> and leaving the problems to bite us seems like a bad idea.  And the problems
> ought to be reported upstream to the HarfBuzz folks.

I don't understand what the actual warning is getting at .. can anyone explain 
it ?

FWIW the code is still the same in upstream harfbuzz
https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-font.cc

-

PR: https://git.openjdk.java.net/jdk/pull/8646

Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]

[Phil Race]

On Thu, 31 Mar 2022 08:03:23 GMT, Andrey Turbanov  wrote:

>> Method `Class.isAssignableFrom` is often used in form of:
>> 
>> if (clazz.isAssignableFrom(obj.getClass())) {
>> Such condition could be simplified to more shorter and performarnt code
>> 
>> if (clazz.isInstance(obj)) {
>> 
>> Replacement is equivalent if it's known that `obj != null`.
>> In JDK codebase there are many such places.
>> 
>> I tried to measure performance via JMH
>> 
>> Class integerClass = Integer.class;
>> Class numberClass = Number.class;
>> 
>> Object integerObject = 45666;
>> Object doubleObject = 8777d;
>> 
>> @Benchmark
>> public boolean integerInteger_isInstance() {
>> return integerClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean integerInteger_isAssignableFrom() {
>> return integerClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isInstance() {
>> return integerClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isAssignableFrom() {
>> return integerClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isInstance() {
>> return numberClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isAssignableFrom() {
>> return numberClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isInstance() {
>> return numberClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isAssignableFrom() {
>> return numberClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public void numberIntegerDouble_isInstance(Blackhole bh) {
>> bh.consume(numberClass.isInstance(integerObject));
>> bh.consume(numberClass.isInstance(doubleObject));
>> }
>> 
>> @Benchmark
>> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
>> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
>> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
>> }
>> 
>> `isInstance` is a bit faster than `isAssignableFrom`
>> 
>> Benchmark  Mode  Cnt  Score   Error  Units
>> integerDouble_isAssignableFrom avgt5  1,173 ± 0,026  ns/op
>> integerDouble_isInstance   avgt5  0,939 ± 0,038  ns/op
>> integerIntegerDouble_isAssignableFrom  avgt5  2,106 ± 0,068  ns/op
>> numberIntegerDouble_isInstance avgt5  1,516 ± 0,046  ns/op
>> integerInteger_isAssignableFromavgt5  1,175 ± 0,029  ns/op
>> integerInteger_isInstance  avgt5  0,886 ± 0,017  ns/op
>> numberDouble_isAssignableFrom  avgt5  1,172 ± 0,007  ns/op
>> numberDouble_isInstanceavgt5  0,891 ± 0,030  ns/op
>> numberInteger_isAssignableFrom avgt5  1,169 ± 0,014  ns/op
>> numberInteger_isInstance   avgt5  0,887 ± 0,016  ns/op
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8280035: Use Class.isInstance instead of Class.isAssignableFrom where 
> applicable
>   apply suggestion to avoid second Method.invoke call

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7061

Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]

[Phil Race]

On Wed, 4 May 2022 08:00:08 GMT, Matthias Baesken  wrote:

>> Currently we set _WIN32_WINNT at various places in the codebase; this is 
>> used to target a minimum Windows version we want to support. See also for 
>> more detailled information :
>> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt
>> Macros for Conditional Declarations
>> "Certain functions that depend on a particular version of Windows are 
>> declared using conditional code. This enables you to use the compiler to 
>> detect whether your application uses functions that are not supported on its 
>> target version(s) of Windows."
>> 
>> However currently we have quite a lot of differing settings of _WIN32_WINNT 
>> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible 
>> would make sense because we have this setting already at   java_props_md.c  
>> (so targeting older Windows versions at other places is most likely not 
>> useful).
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   adjust API level to Windows 8 for security.cpp and do some cleanup

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8428

Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments

[Phil Race]

On Mon, 9 May 2022 15:56:35 GMT, Adam Sotona  wrote:

> Please review this patch adding new lint option, **lossy-conversions**, to 
> javac to warn about type casts in compound assignments with possible lossy 
> conversions.
> 
> The new lint warning is shown if the type of the right-hand operand of a 
> compound assignment is not assignment compatible with the type of the 
> variable.
> 
> The implementation of the warning is based on similar check performed to emit 
> "possible lossy conversion" compilation error for simple assignments. 
> 
> Proposed patch also include complex matrix-style test with positive and 
> negative test cases of lossy conversions in compound assignments.
> 
> Proposed patch also disables this new lint option in all affected JDK modules 
> and libraries to allow smooth JDK build. Individual cases to address possibly 
> lossy conversions warnings in JDK are already addressed in a separate 
> umbrella issue and its sub-tasks.
> 
> Thanks for your review,
> Adam

Marked as reviewed by prr (Reviewer).

test/langtools/tools/javac/lint/LossyConversions.java line 131:

> 129: @SuppressWarnings("lossy-conversions")
> 130: public void supressedLossyConversions() {
> 131: byte a = 0;

you might want to spell suppressed correctly.

-

PR: https://git.openjdk.java.net/jdk/pull/8599

Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults

[Phil Race]

On Thu, 5 May 2022 16:49:07 GMT, Mark Powers  wrote:

> JDK-6725221 Standardize obtaining boolean properties with defaults

I did wonder why it has security-libs as the sub-category and if the intent was 
not what we see here.

-

PR: https://git.openjdk.java.net/jdk/pull/8559

Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults

[Phil Race]

On Thu, 5 May 2022 16:49:07 GMT, Mark Powers  wrote:

> JDK-6725221 Standardize obtaining boolean properties with defaults

The xtoolkit change is fine. I've not looked at anything else
You'll clearly need multiple reviewers for this one.

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8559

Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]

[Phil Race]

On Thu, 28 Apr 2022 01:31:13 GMT, Joe Darcy  wrote:

>> To enable more complete doclint checking (courtesy @jonathan-gibbons), 
>> please review this PR to add type-level @param tags where they are missing.
>> 
>> To the maintainers of java.util.concurrent, those changes could be separated 
>> out in another bug if that would ease maintenance of that code.
>> 
>> Making these library fixes is a blocker for correcting and expanding the 
>> doclint checks (JDK-8285496).
>> 
>> I'll update copyright years before pushing.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to more review feedback.

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8410

Re: RFR: JDK-8285730: unify _WIN32_WINNT settings

[Phil Race]

On Wed, 27 Apr 2022 14:57:41 GMT, Matthias Baesken  wrote:

> Currently we set _WIN32_WINNT at various places in the codebase; this is used 
> to target a minimum Windows version we want to support. See also for more 
> detailled information :
> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt
> Macros for Conditional Declarations
> "Certain functions that depend on a particular version of Windows are 
> declared using conditional code. This enables you to use the compiler to 
> detect whether your application uses functions that are not supported on its 
> target version(s) of Windows."
> 
> However currently we have quite a lot of differing settings of _WIN32_WINNT 
> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible 
> would make sense because we have this setting already at   java_props_md.c  
> (so targeting older Windows versions at other places is most likely not 
> useful).

This is OK by me, but I wonder if the build folks might want to think about 
this and whether it should be centralised somehow
since it seems odd to mandate different versions of windows in different places.

-

PR: https://git.openjdk.java.net/jdk/pull/8428

Re: RFR: 8257733: Move module-specific data from make to respective module [v9]

[Phil Race]

On Fri, 18 Mar 2022 21:24:43 GMT, Phil Race  wrote:

>> Magnus Ihse Bursie has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix typos
>
> This doesn't seem right to me to put x11wrappergen into share.
> 
> This is X11 specific. It should not be in share.
> 
> Same for all of the fontconfig files. In make/data it did not seem too weird 
> but it is very weird to put them all in share. If you were to go back and 
> look how it used to be before someone moved them to make I am sure you'd find 
> them in platform specific source directories.

> @prrace I have now moved the fontconfig files to `$OS/data`. (I also took the 
> opportunity to clean up the GenData file, which had a quite hairy logic for a 
> trivial task.) I have moved the x11wrappergen files to `unix/data`.
> 
> (Strictly speaking, "unix" and "x11" do not have the same "sense" (in the 
> Fregean meaning), even if it has the same "referent". But we've used that 
> convention before so I'm sticking to it.)

Indeed they do not but it is a better approximation than "shared".

-

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: RFR: 8257733: Move module-specific data from make to respective module [v9]

[Phil Race]

On Thu, 17 Mar 2022 00:12:38 GMT, Magnus Ihse Bursie  wrote:

>> A lot (but not all) of the data in make/data is tied to a specific module. 
>> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
>> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
>> make for the whole build.) 
>> 
>> These data files should move to the module they belong to. The are, after 
>> all, "source code" for that module that is "compiler" into resulting 
>> deliverables, for that module. (But the "source code" language is not Java 
>> or C, but typically a highly domain specific language or data format, and 
>> the "compilation" is, often, a specialized transformation.) 
>> 
>> This misplacement of the data directory is most visible at code review time. 
>> When such data is changed, most of the time build-dev (or the new build 
>> label) is involved, even though this has nothing to do with the build. While 
>> this is annoying, a worse problem is if the actual team that needs to review 
>> the patch (i.e., the team owning the module) is missed in the review.
>> 
>> ### Modules reviewed
>> 
>> - [x] java.base
>> - [x] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix typos

This doesn't seem right to me to put x11wrappergen into share.

This is X11 specific. It should not be in share.

Same for all of the fontconfig files. In make/data it did not seem too weird 
but it is very weird to put them all in share. If you were to go back and look 
how it used to be before someone moved them to make I am sure you'd find them 
in platform specific source directories.

-

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: RFR: 8257733: Move module-specific data from make to respective module [v9]

[Phil Race]

On Thu, 17 Mar 2022 00:12:38 GMT, Magnus Ihse Bursie  wrote:

>> A lot (but not all) of the data in make/data is tied to a specific module. 
>> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
>> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
>> make for the whole build.) 
>> 
>> These data files should move to the module they belong to. The are, after 
>> all, "source code" for that module that is "compiler" into resulting 
>> deliverables, for that module. (But the "source code" language is not Java 
>> or C, but typically a highly domain specific language or data format, and 
>> the "compilation" is, often, a specialized transformation.) 
>> 
>> This misplacement of the data directory is most visible at code review time. 
>> When such data is changed, most of the time build-dev (or the new build 
>> label) is involved, even though this has nothing to do with the build. While 
>> this is annoying, a worse problem is if the actual team that needs to review 
>> the patch (i.e., the team owning the module) is missed in the review.
>> 
>> ### Modules reviewed
>> 
>> - [x] java.base
>> - [x] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix typos

Changes requested by prr (Reviewer).

make/modules/java.desktop/gensrc/GensrcX11Wrappers.gmk line 27:

> 25: 
> 26: # Generate java sources using the X11 offsets that are precalculated in 
> files
> 27: # src/java.desktop/share/data/x11wrappergen/sizes-.txt.

This doesn't seem right to me. This is X11 specific. It should not be in share.
Same for related files.

-

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: RFR: 8257733: Move module-specific data from make to respective module [v6]

[Phil Race]

On Tue, 15 Mar 2022 23:50:20 GMT, Magnus Ihse Bursie  wrote:

>> A lot (but not all) of the data in make/data is tied to a specific module. 
>> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
>> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
>> make for the whole build.) 
>> 
>> These data files should move to the module they belong to. The are, after 
>> all, "source code" for that module that is "compiler" into resulting 
>> deliverables, for that module. (But the "source code" language is not Java 
>> or C, but typically a highly domain specific language or data format, and 
>> the "compilation" is, often, a specialized transformation.) 
>> 
>> This misplacement of the data directory is most visible at code review time. 
>> When such data is changed, most of the time build-dev (or the new build 
>> label) is involved, even though this has nothing to do with the build. While 
>> this is annoying, a worse problem is if the actual team that needs to review 
>> the patch (i.e., the team owning the module) is missed in the review.
>> 
>> ### Modules reviewed
>> 
>> - [x] java.base
>> - [x] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now contains 12 commits:
> 
>  - Merge branch 'master' into shuffle-data-reborn
>  - Fix merge
>  - Merge tag 'jdk-19+13' into shuffle-data-reborn
>
>Added tag jdk-19+13 for changeset 5df2a057
>  - Move characterdata templates to share/classes/java/lang.
>  - Update comment refering to "make" dir
>  - Move new symbols to jdk.compiler
>  - Merge branch 'master' into shuffle-data
>  - Move macosxicons from share to macosx
>  - Move to share/data, and move jdwp.spec to java.se
>  - Update references in test
>  - ... and 2 more: 
> https://git.openjdk.java.net/jdk/compare/83d77186...598f740f

How are folks reviewing this huge PR ?
My browser paints a few files and that's it, and the drop down list to "jump 
to" is too big to display - it says !

-

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()

[Phil Race]

On Fri, 4 Mar 2022 13:25:12 GMT, Zhengyu Gu  wrote:

> Please review this small patch that fixes a potential memory leak that 
> exception return fails to release allocated `cacheDirs`
> 
> Test:
> 
> - [x] jdk_desktop on Linux x86_64

I've increased the number of reviewers because as well as the current reviewer 
it should have
(1) a reviewer from the client team
(2) a reviewer from whoever owns jni_util.h

If there were no change to that file it would have been simpler.

-

PR: https://git.openjdk.java.net/jdk/pull/7691

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Phil Race]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

Marked as reviewed by prr (Reviewer).

Looks like  there's only one client source code file touched
Most of the client changes are only in jtreg tests - and this is very trivial, 
so I'm OK with them being included here.

-

PR: https://git.openjdk.java.net/jdk/pull/7268

Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable

[Phil Race]

On Thu, 13 Jan 2022 08:25:22 GMT, Andrey Turbanov  wrote:

> Method `Class.isAssignableFrom` is often used in form of:
> 
> if (clazz.isAssignableFrom(obj.getClass())) {
> Such condition could be simplified to more shorter and performarnt code
> 
> if (clazz.isInstance(obj)) {
> 
> Replacement is equivalent if it's known that `obj != null`.
> In JDK codebase there are many such places.
> 
> I tried to measure performance via JMH
> 
> Class integerClass = Integer.class;
> Class numberClass = Number.class;
> 
> Object integerObject = 45666;
> Object doubleObject = 8777d;
> 
> @Benchmark
> public boolean integerInteger_isInstance() {
> return integerClass.isInstance(integerObject);
> }
> 
> @Benchmark
> public boolean integerInteger_isAssignableFrom() {
> return integerClass.isAssignableFrom(integerObject.getClass());
> }
> 
> @Benchmark
> public boolean integerDouble_isInstance() {
> return integerClass.isInstance(doubleObject);
> }
> 
> @Benchmark
> public boolean integerDouble_isAssignableFrom() {
> return integerClass.isAssignableFrom(doubleObject.getClass());
> }
> 
> @Benchmark
> public boolean numberDouble_isInstance() {
> return numberClass.isInstance(doubleObject);
> }
> 
> @Benchmark
> public boolean numberDouble_isAssignableFrom() {
> return numberClass.isAssignableFrom(doubleObject.getClass());
> }
> 
> @Benchmark
> public boolean numberInteger_isInstance() {
> return numberClass.isInstance(integerObject);
> }
> 
> @Benchmark
> public boolean numberInteger_isAssignableFrom() {
> return numberClass.isAssignableFrom(integerObject.getClass());
> }
> 
> @Benchmark
> public void numberIntegerDouble_isInstance(Blackhole bh) {
> bh.consume(numberClass.isInstance(integerObject));
> bh.consume(numberClass.isInstance(doubleObject));
> }
> 
> @Benchmark
> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
> }
> 
> `isInstance` is a bit faster than `isAssignableFrom`
> 
> Benchmark  Mode  Cnt  Score   Error  Units
> integerDouble_isAssignableFrom avgt5  1,173 ± 0,026  ns/op
> integerDouble_isInstance   avgt5  0,939 ± 0,038  ns/op
> integerIntegerDouble_isAssignableFrom  avgt5  2,106 ± 0,068  ns/op
> numberIntegerDouble_isInstance avgt5  1,516 ± 0,046  ns/op
> integerInteger_isAssignableFromavgt5  1,175 ± 0,029  ns/op
> integerInteger_isInstance  avgt5  0,886 ± 0,017  ns/op
> numberDouble_isAssignableFrom  avgt5  1,172 ± 0,007  ns/op
> numberDouble_isInstanceavgt5  0,891 ± 0,030  ns/op
> numberInteger_isAssignableFrom avgt5  1,169 ± 0,014  ns/op
> numberInteger_isInstance   avgt5  0,887 ± 0,016  ns/op

I've stared at the javadoc for Class.isAssignableFrom and Class.isInstance and 
if a non-null instance is used to get a non-null class they are PROBABLY the 
same but it is far from clear. The implementations of both are at least native 
and may be instrinsicified. The doc for Class.isAssignableFrom cites JLS 5.1.4 
which in what I found is about primitives so I suspect it is woefully out of 
date 
https://docs.oracle.com/javase/specs/jls/se17/html/jls-5.html#jls-5.1.4

What client tests have you run that touch the code you are changing ?

In short I see insufficient value in the changes here and would prefer you drop 
the client part so I don't have to worry about it.

-

PR: https://git.openjdk.java.net/jdk/pull/7061

Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal [v3]

[Phil Race]

On Wed, 1 Dec 2021 19:23:59 GMT, Brent Christian  wrote:

>> Here are the code changes for the "Deprecate finalizers in the standard Java 
>> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code 
>> review.
>> 
>> This change makes the indicated deprecations, and updates the API spec for 
>> JEP 421. It also updates the relevant @SuppressWarning annotations.
>> 
>> The CSR has been approved.
>> An automated test build+test run passes cleanly (FWIW :D ).
>
> Brent Christian has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 33 commits:
> 
>  - Merge branch 'master' into 8276447
>  - Merge branch 'master' into 8276447
>  - merge
>  - @SuppressWarnings(removal) in Windows CKey.java
>  - Add jls tag to runFinalization methods
>  - Update wording of Object.finalize, new paragraph is now bold
>  - update Object.finalize javadoc
>  - update Object.finalize JavaDoc and @deprecated tag
>  - Update Object.finalize deprecation message
>  - Indicate that runFinalizers does nothing if finalization is disabled or 
> removed
>  - ... and 23 more: 
> https://git.openjdk.java.net/jdk/compare/0dfb3a70...8cde0674

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6465

Re: RFR: 8277868: Use Comparable.compare() instead of surrogate code [v2]

[Phil Race]

On Mon, 29 Nov 2021 08:18:47 GMT, Сергей Цыпанов  wrote:

>> Instead of something like
>> 
>> long x;
>> long y;
>> return (x < y) ? -1 : ((x == y) ? 0 : 1);
>> 
>> we can use `return Long.compare(x, y);`
>> 
>> All replacements are done with IDE.
>
> Сергей Цыпанов has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8277868: Use Integer.signum() in BasicTableUI

src/java.desktop/share/classes/java/awt/geom/Line2D.java line 115:

> 113:  */
> 114: public double getX1() {
> 115: return x1;

What do these changes have to do with the subject of the PR ?

src/java.desktop/share/classes/sun/java2d/Spans.java line 322:

> 320: float otherStart = otherSpan.getStart();
> 321: 
> 322: return Float.compare(mStart, otherStart);

We don't need the variable any more, do we ?

-

PR: https://git.openjdk.java.net/jdk/pull/6575

Re: RFR: 8276348: Use blessed modifier order in java.base

[Phil Race]

On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo  wrote:

> This PR follows up one of the recent PRs, where I used a non-canonical 
> modifier order. Since the problem was noticed [^1], why not to address it at 
> mass?
> 
> As far as I remember, the first mass-canonicalization of modifiers took place 
> in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 
> files. Since then modifiers have become a bit lose, and it makes sense to 
> re-bless (using the JDK-8136583 terminology) them.
> 
> This change was produced by running the below command followed by updating 
> the copyright years on the affected files where necessary:
> 
> $ sh ./bin/blessed-modifier-order.sh src/java.base
> 
> The resulting change is much smaller than that of 2015: 39 lines spanning 21 
> files.
> 
> [^1]: 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html
>  (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
> [^2]: 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html

JFYI a couple of times I've wondered if we regressed on this. I just ran the 
script on the desktop module and we havea  few instances there too, so I've 
filed a clean up bug on it.

-

PR: https://git.openjdk.java.net/jdk/pull/6213

Re: RFR: 8262297: ImageIO.write() method will throw IndexOutOfBoundsException

[Phil Race]

On Thu, 28 Oct 2021 09:29:13 GMT, Masanori Yano  wrote:

> Could you please review the 8262297 bug fixes?
> 
> In this case, ImageIO.write() should throw java.io.IOException rather than 
> java.lang.IndexOutOfBoundsException. IndexOutOfBoundsException is caught and 
> wrapped in IIOException in ImageIO.write() with this fix. In addition, 
> IndexOutOfBoundsException is not expected to throw by 
> RandomAccessFile#write() according to its API specification. So it should be 
> fixed.

This needs looking at closely .. I don't know what's best but the imaging APIs 
are very exception heavy because of the arrays, indices, you name it that they 
consume. Most of the time its probably best for an IIO user to just get an 
IIOException wrapping the original cause. But there could be exceptions.

-

PR: https://git.openjdk.java.net/jdk/pull/6151

Re: RFR: 8275170: Some jtreg sound tests should be marked headful

[Phil Race]

On Fri, 22 Oct 2021 19:01:27 GMT, Phil Race  wrote:

> This fix adds "headful sound" keywords to a number of javax/sound jtreg tests
> 
> The jtreg javax.sound tests are written in such a way that if a needed audio 
> device
> or other platform resource is not available, they just exit with a "pass" for 
> the test.
> It is as if headful tests just swallowed HeadlessException and issued a pass.
> If we allowed that we'd be effectively testing almost nothing of real 
> importance.
> 
> Currently almost  all sound tests have no keyword like "headful" to indicate 
> they
> may need access to a hardware resource to do anything useful so a similar
> situation arises there except when someone runs those tests manually on
> a local system which has audio devices.
> 
> Of course "headful" and "audio device" aren't exactly interchangeable terms
> but if tests are allowed to be run - or worse usually or always run - in a 
> situation
> where they potentially are on a system without an audio device (a headless 
> VM) or are running in
> a session which doesn't have full desktop access - which may in some
> cases be how audio device access is granted, then they are more likely
> to be running in this scenario where the testing isn't valid.
> 
> Another point is that headful tests must be run one at a time - no 
> concurrency because
> you can't have multiple tests moving the mouse at the same time
> Whereas for headless tests, a test harness may choose to run concurrently - 
> perhaps even in the same VM
> When tests that really need access to an audio device are run it is probably 
> safer to consider
> the headful attribute as implying one test at a time is best .. granted on a 
> desktop it isn't
> usual for a single app to be able to monopolize access to a speaker, but for 
> input devices
> that is what you'd generally expect.
> 
> By no means am I sure that the tests being updated here are the full set that 
> need tagging.
> There are a lot of tests and they are all known to pass on systems with NO 
> audio
> devices, so the search has been for tests which call APIs which will 
> definitely
> need access to some device in order to do anything useful.
> So likely there are more to be added - either by a reviewer pointing them out 
> or by later discovery.
> Alternatively we could mark ALL sound tests headful .. but given where we are 
> starting from
> that might be erring unnecessarily far in the opposite direction.
> 
> By adding both headful and sound keywords it gives options to someone who
> wants to identify the tests and perhaps run just tests which need "sound" on 
> some
> config they know supports audio but isn't headful.

This makes the tests eligible to be run on headful systems.
In practice it seems like ONLY headful systems actually have the sound devices.
The sound keyword isn't understood by anything yet, so adding headful is the 
only way
we have right now of ensuring these tests are eligible to be run on systems 
that have the
necessary support.

As I explained in the initial PR description headful is also useful for 
implying exclusive access.

And without this keyword it means that these tests are ALWAYS run on systems 
without
sound device support. So for years we've effectively problem listed these tests 
due to
the tests not having headful and silently passing when there's no sound.
Without headful  no one runs these on systems that have the needed device.

So this solves a big hole that we aren't testing this case.

The 2 keywords give flexibility - anyone who wants to filter when its marked 
headful
and needing sound can do this, but just adding sound right now will solve 
nothing.

And there are a couple of tests NOT in OpenJDK that already do this so we have 
precedent - and in a previous life one was added by you and the other approved 
by you ..

-

PR: https://git.openjdk.java.net/jdk/pull/6086

RFR: 8275170: Some jtreg sound tests should be marked headful

[Phil Race]

This fix adds "headful sound" keywords to a number of javax/sound jtreg tests

The jtreg javax.sound tests are written in such a way that if a needed audio 
device
or other platform resource is not available, they just exit with a "pass" for 
the test.
It is as if headful tests just swallowed HeadlessException and issued a pass.
If we allowed that we'd be effectively testing almost nothing of real 
importance.

Currently almost  all sound tests have no keyword like "headful" to indicate 
they
may need access to a hardware resource to do anything useful so a similar
situation arises there except when someone runs those tests manually on
a local system which has audio devices.

Of course "headful" and "audio device" aren't exactly interchangeable terms
but if tests are allowed to be run - or worse usually or always run - in a 
situation
where they potentially are on a system without an audio device (a headless VM) 
or are running in
a session which doesn't have full desktop access - which may in some
cases be how audio device access is granted, then they are more likely
to be running in this scenario where the testing isn't valid.

Another point is that headful tests must be run one at a time - no concurrency 
because
you can't have multiple tests moving the mouse at the same time
Whereas for headless tests, a test harness may choose to run concurrently - 
perhaps even in the same VM
When tests that really need access to an audio device are run it is probably 
safer to consider
the headful attribute as implying one test at a time is best .. granted on a 
desktop it isn't
usual for a single app to be able to monopolize access to a speaker, but for 
input devices
that is what you'd generally expect.

By no means am I sure that the tests being updated here are the full set that 
need tagging.
There are a lot of tests and they are all known to pass on systems with NO audio
devices, so the search has been for tests which call APIs which will definitely
need access to some device in order to do anything useful.
So likely there are more to be added - either by a reviewer pointing them out 
or by later discovery.
Alternatively we could mark ALL sound tests headful .. but given where we are 
starting from
that might be erring unnecessarily far in the opposite direction.

By adding both headful and sound keywords it gives options to someone who
wants to identify the tests and perhaps run just tests which need "sound" on 
some
config they know supports audio but isn't headful.

-

Commit messages:
 - 8275170: Some jtreg sound tests should be marked headful

Changes: https://git.openjdk.java.net/jdk/pull/6086/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6086=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8275170
  Stats: 63 lines in 58 files changed: 59 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6086.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6086/head:pull/6086

PR: https://git.openjdk.java.net/jdk/pull/6086

Integrated: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

[Phil Race]

On Mon, 27 Sep 2021 20:56:28 GMT, Phil Race  wrote:

> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set 
> the name of the application in the system menu bar.
> 
> Because this set shortly after the VM is running, it causes a thread safety 
> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549
> 
> Since the AWT already looks for the system property 
> "apple.awt.application.name" for this same purpose,
> we can set that instead of the env. var and avoid the threading issue.
> 
> This trades the JAVA_MAIN_CLASS_ pollution of the environment for 
> setting a system property which is visible to apps as well but it seems a 
> reasonable trade-off since we already (implicitly) support this variable 
> anyway in preference to the env. var.

This pull request has now been integrated.

Changeset: 37890650
Author:Phil Race 
URL:   
https://git.openjdk.java.net/jdk/commit/37890650a7c97d484b6b520d909f677dac4e46e1
Stats: 205 lines in 5 files changed: 159 ins; 26 del; 20 mod

8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

Reviewed-by: rriggs, serb

-

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v4]

[Phil Race]

On Fri, 1 Oct 2021 21:10:27 GMT, Phil Race  wrote:

>> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set 
>> the name of the application in the system menu bar.
>> 
>> Because this set shortly after the VM is running, it causes a thread safety 
>> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549
>> 
>> Since the AWT already looks for the system property 
>> "apple.awt.application.name" for this same purpose,
>> we can set that instead of the env. var and avoid the threading issue.
>> 
>> This trades the JAVA_MAIN_CLASS_ pollution of the environment for 
>> setting a system property which is visible to apps as well but it seems a 
>> reasonable trade-off since we already (implicitly) support this variable 
>> anyway in preference to the env. var.
>
> Phil Race has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

I just pushed a small update to the new test.
The compiled class file of the child process was not found when I ran it in our 
test framework even though jtreg run locally found it. I just had to explicitly 
add the location of the compiled classes to the classpath. 
So it is just about getting the test to run in that env. rather than any 
problem with the fix, or the test logic.
This update HAS passed in that framework - as well as locally

-

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v5]

[Phil Race]

> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set 
> the name of the application in the system menu bar.
> 
> Because this set shortly after the VM is running, it causes a thread safety 
> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549
> 
> Since the AWT already looks for the system property 
> "apple.awt.application.name" for this same purpose,
> we can set that instead of the env. var and avoid the threading issue.
> 
> This trades the JAVA_MAIN_CLASS_ pollution of the environment for 
> setting a system property which is visible to apps as well but it seems a 
> reasonable trade-off since we already (implicitly) support this variable 
> anyway in preference to the env. var.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5724/files
  - new: https://git.openjdk.java.net/jdk/pull/5724/files/4c8fb9af..ffd77dd9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5724=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5724=03-04

  Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5724.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v3]

[Phil Race]

On Wed, 29 Sep 2021 03:39:09 GMT, Phil Race  wrote:

>> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set 
>> the name of the application in the system menu bar.
>> 
>> Because this set shortly after the VM is running, it causes a thread safety 
>> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549
>> 
>> Since the AWT already looks for the system property 
>> "apple.awt.application.name" for this same purpose,
>> we can set that instead of the env. var and avoid the threading issue.
>> 
>> This trades the JAVA_MAIN_CLASS_ pollution of the environment for 
>> setting a system property which is visible to apps as well but it seems a 
>> reasonable trade-off since we already (implicitly) support this variable 
>> anyway in preference to the env. var.
>
> Phil Race has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

I've now pushed the new test to verify the system property.
I've verified the test passes on my local machine but can't (until Monday) be 
sure it passes in the CI test framework because of power maintenance that has 
just started.

-

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v4]

[Phil Race]

> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set 
> the name of the application in the system menu bar.
> 
> Because this set shortly after the VM is running, it causes a thread safety 
> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549
> 
> Since the AWT already looks for the system property 
> "apple.awt.application.name" for this same purpose,
> we can set that instead of the env. var and avoid the threading issue.
> 
> This trades the JAVA_MAIN_CLASS_ pollution of the environment for 
> setting a system property which is visible to apps as well but it seems a 
> reasonable trade-off since we already (implicitly) support this variable 
> anyway in preference to the env. var.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5724/files
  - new: https://git.openjdk.java.net/jdk/pull/5724/files/96a5590c..4c8fb9af

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5724=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5724=02-03

  Stats: 112 lines in 2 files changed: 112 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5724.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v3]

[Phil Race]

On Wed, 29 Sep 2021 03:39:09 GMT, Phil Race  wrote:

>> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set 
>> the name of the application in the system menu bar.
>> 
>> Because this set shortly after the VM is running, it causes a thread safety 
>> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549
>> 
>> Since the AWT already looks for the system property 
>> "apple.awt.application.name" for this same purpose,
>> we can set that instead of the env. var and avoid the threading issue.
>> 
>> This trades the JAVA_MAIN_CLASS_ pollution of the environment for 
>> setting a system property which is visible to apps as well but it seems a 
>> reasonable trade-off since we already (implicitly) support this variable 
>> anyway in preference to the env. var.
>
> Phil Race has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

And, oh, the env. var test code I needed to delete was checking not for a 
useful value but just that the env var was there.
I wrote a simple jtreg test or the new code that set the system property and 
tested the expected value (default or set)
but it seems jtreg makes MainWrapper the main class that is found regardless of 
main/othervm so I am currently grumbling quietly to myself about whether to add 
a test which is equivalent to the previous one which just tests the property 
has a value, 
or to (I suppose) write a more sophisticated test that has to exec another VM 
where it *should* be able to properly verify it.

-

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]

[Phil Race]

On Mon, 27 Sep 2021 23:50:38 GMT, Phil Race  wrote:

>> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set 
>> the name of the application in the system menu bar.
>> 
>> Because this set shortly after the VM is running, it causes a thread safety 
>> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549
>> 
>> Since the AWT already looks for the system property 
>> "apple.awt.application.name" for this same purpose,
>> we can set that instead of the env. var and avoid the threading issue.
>> 
>> This trades the JAVA_MAIN_CLASS_ pollution of the environment for 
>> setting a system property which is visible to apps as well but it seems a 
>> reasonable trade-off since we already (implicitly) support this variable 
>> anyway in preference to the env. var.
>
> Phil Race has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher 
> code

I've added code in the launcher part that strips the package name from what is 
seen.
This was previously done in the AWT code for MAIN_CLASS_ since it was 
presumably the only code setting that but I didn't do it because before because 
I didn't want to interfere with what an app might have set as the system 
property .. but .. if the app didn't set it and we derived it, then I realised 
we probably should be consistent with what happened before and only the 
launcher code knows whether it was set by itself or the app

-

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v3]

[Phil Race]

> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set 
> the name of the application in the system menu bar.
> 
> Because this set shortly after the VM is running, it causes a thread safety 
> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549
> 
> Since the AWT already looks for the system property 
> "apple.awt.application.name" for this same purpose,
> we can set that instead of the env. var and avoid the threading issue.
> 
> This trades the JAVA_MAIN_CLASS_ pollution of the environment for 
> setting a system property which is visible to apps as well but it seems a 
> reasonable trade-off since we already (implicitly) support this variable 
> anyway in preference to the env. var.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5724/files
  - new: https://git.openjdk.java.net/jdk/pull/5724/files/93108c59..96a5590c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5724=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5724=01-02

  Stats: 24 lines in 1 file changed: 23 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5724.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]

[Phil Race]

On Wed, 29 Sep 2021 01:46:32 GMT, Sergey Bylokhov  wrote:

>> Phil Race has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher 
>> code
>
> src/java.base/macosx/native/libjli/java_md_macosx.m line 879:
> 
>> 877: }
>> 878: 
>> 879: (*env)->DeleteLocalRef(env, jKey);
> 
> I am not sure about error handling, the jkey is not removed when NULL_CHECK 
> is used. Also an exceptions are not cleared in case of NULL_CHECK like in 
> other cases, is it intentional?

Well .. they aren't removed by the existing code either. And this is the 
launcher.
So far as I can tell if we error out of here (as I found when I made a typo in 
a method signature) the
VM exits very rapidly. So if I do anything here, it would be to remove 
DeleteLocalRef since it really doesn't matter to clean up local refs when you 
are either bailing from native .. or the entire process .. cleaning up when you 
are continuing on (as the code does) is perhaps more important since you might 
need more local refs before you are done.
Or file a separate bug against the launcher and JNI clean up after error 
handling.
java.base/share/native/libjli/java.c is a good example of where the same 
pattern exists.

-

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]

[Phil Race]

> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set 
> the name of the application in the system menu bar.
> 
> Because this set shortly after the VM is running, it causes a thread safety 
> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549
> 
> Since the AWT already looks for the system property 
> "apple.awt.application.name" for this same purpose,
> we can set that instead of the env. var and avoid the threading issue.
> 
> This trades the JAVA_MAIN_CLASS_ pollution of the environment for 
> setting a system property which is visible to apps as well but it seems a 
> reasonable trade-off since we already (implicitly) support this variable 
> anyway in preference to the env. var.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5724/files
  - new: https://git.openjdk.java.net/jdk/pull/5724/files/232bfae4..93108c59

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5724=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5724=00-01

  Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5724.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724

PR: https://git.openjdk.java.net/jdk/pull/5724

RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

[Phil Race]

macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set the 
name of the application in the system menu bar.

Because this set shortly after the VM is running, it causes a thread safety 
issue described in https://bugs.openjdk.java.net/browse/JDK-8270549

Since the AWT already looks for the system property 
"apple.awt.application.name" for this same purpose,
we can set that instead of the env. var and avoid the threading issue.

This trades the JAVA_MAIN_CLASS_ pollution of the environment for setting 
a system property which is visible to apps as well but it seems a reasonable 
trade-off since we already (implicitly) support this variable anyway in 
preference to the env. var.

-

Commit messages:
 - 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code
 - 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

Changes: https://git.openjdk.java.net/jdk/pull/5724/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5724=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8274397
  Stats: 75 lines in 2 files changed: 33 ins; 33 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5724.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724

PR: https://git.openjdk.java.net/jdk/pull/5724

Re: RFR: 8236505: Mark jdk/editpad/EditPadTest.java as @headful

[Phil Race]

On Thu, 16 Sep 2021 09:13:15 GMT, Aleksey Shipilev  wrote:

> This is a GUI test, and it should be `@headful`.
> 
> Additional testing:
>  - [x] Test still runs in default, and does not run with `!headful`

Well .. since our test framework doesn't run whatever test group this is in on 
headful systems, what this change has effectively done is say "don't run this 
test anymore. ever".
You'd need to add it to the desktop test group.

-

PR: https://git.openjdk.java.net/jdk/pull/5544

Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging

[Phil Race]

On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov  wrote:

> The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" 
> via an AppContext to support "applet logging isolation". The AppContext class 
> became useless since the plugin and webstart are no longer supported and 
> removed in jdk11.
> 
> This is the request to delete the usage of AppContext in the LogManager and 
> related tests.
> 
> Tested by tier1/tier2/tier3 and jdk_desktop tests.

@aivanov-jdk will help make sure the closed changes are pushed at exactly the 
same time as this gets pushed.

-

PR: https://git.openjdk.java.net/jdk/pull/5326

Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging

[Phil Race]

On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov  wrote:

> The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" 
> via an AppContext to support "applet logging isolation". The AppContext class 
> became useless since the plugin and webstart are no longer supported and 
> removed in jdk11.
> 
> This is the request to delete the usage of AppContext in the LogManager and 
> related tests.
> 
> Tested by tier1/tier2/tier3 and jdk_desktop tests.

Hmm I was under the impression this was removing AppContext itself but it is 
just removing the backdoor needed by logging
Perhaps this isn't the change that requires the CSR but it then leaves an 
inconsistent state where desktop supports AppContext still but other modules 
don't ...

-

PR: https://git.openjdk.java.net/jdk/pull/5326

Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging

[Phil Race]

On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov  wrote:

> The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" 
> via an AppContext to support "applet logging isolation". The AppContext class 
> became useless since the plugin and webstart are no longer supported and 
> removed in jdk11.
> 
> This is the request to delete the usage of AppContext in the LogManager and 
> related tests.
> 
> Tested by tier1/tier2/tier3 and jdk_desktop tests.

I believe we should have a CSR for this. It removes a long standing capability 
in the platform that was used by
components such as plugin and webstart.

-

PR: https://git.openjdk.java.net/jdk/pull/5326

Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging

[Phil Race]

On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov  wrote:

> The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" 
> via an AppContext to support "applet logging isolation". The AppContext class 
> became useless since the plugin and webstart are no longer supported and 
> removed in jdk11.
> 
> This is the request to delete the usage of AppContext in the LogManager and 
> related tests.
> 
> Tested by tier1/tier2/tier3 and jdk_desktop tests.

This fix requires coordinated closed changes so needs an Oracle sponsor.
And actually is probably better handled entirely by an Oracle engineer because 
pushes need to be very co-ordinated.

-

PR: https://git.openjdk.java.net/jdk/pull/5326

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Phil Race]

On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang  wrote:

>> The code change refactors classes that have a `SuppressWarnings("removal")` 
>> annotation that covers more than 50KB of code. The big annotation is often 
>> quite faraway from the actual deprecated API usage it is suppressing, and 
>> with the annotation covering too big a portion it's easy to call other 
>> deprecated methods without being noticed.
>> 
>> The code change shows some common solutions to avoid such too wide 
>> annotations:
>> 
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local 
>> variable and add annotation on the declaration, and then assign that value 
>> to the original l-value if not void. The local variable will be called `tmp` 
>> if later reassigned or `dummy` if it will be discarded.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>> 
>> I'll add a copyright year update commit before integration.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update FtpClient.java

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4138

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Phil Race]

On Fri, 28 May 2021 02:50:55 GMT, Weijun Wang  wrote:

>> src/java.desktop/share/classes/java/awt/Component.java line 630:
>> 
>>> 628: }
>>> 629: 
>>> 630: @SuppressWarnings("removal")
>> 
>> I'm confused. I thought the reason this wasn't done in the JEP 
>> implementation PR is because of refactoring
>> that was needed because of the usage in this static block and you could not 
>> apply the annotation here.
>> Yet it seems you are doing exactly what was supposed to be impossible with 
>> no refactoring.
>> Can you explain ?
>
> There *is* a tiny refactoring here: a new variable `s2` is introduced so the 
> 2nd `doPrivileged` call on line 636 is now also in a declaration statement 
> (for `s2`) and therefore annotatable. Without this I cannot add the 
> annotation on line 635.

Ok. But I will quote you
"This happens when a deprecated method is called inside a static block. The 
annotation can only be added to a declaration and here it must be the whole 
class"

So the way you explained this before made it sound like any time there was any 
SM API usage in a static block, the entire class needed to be annotated.

Why has the explanation changed ?

-

PR: https://git.openjdk.java.net/jdk/pull/4138

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Phil Race]

On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang  wrote:

>> The code change refactors classes that have a `SuppressWarnings("removal")` 
>> annotation that covers more than 50KB of code. The big annotation is often 
>> quite faraway from the actual deprecated API usage it is suppressing, and 
>> with the annotation covering too big a portion it's easy to call other 
>> deprecated methods without being noticed.
>> 
>> The code change shows some common solutions to avoid such too wide 
>> annotations:
>> 
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local 
>> variable and add annotation on the declaration, and then assign that value 
>> to the original l-value if not void. The local variable will be called `tmp` 
>> if later reassigned or `dummy` if it will be discarded.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>> 
>> I'll add a copyright year update commit before integration.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update FtpClient.java

src/java.desktop/share/classes/java/awt/Component.java line 630:

> 628: }
> 629: 
> 630: @SuppressWarnings("removal")

I'm confused. I thought the reason this wasn't done in the JEP implementation 
PR is because of refactoring
that was needed because of the usage in this static block and you could not 
apply the annotation here.
Yet it seems you are doing exactly what was supposed to be impossible with no 
refactoring.
Can you explain ?

-

PR: https://git.openjdk.java.net/jdk/pull/4138

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]

[Phil Race]

On Mon, 24 May 2021 13:53:34 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   keep only one systemProperty tag

Marked as reviewed by prr (Reviewer).

I'm OK with this now given that the refactoring is already underway at 
https://github.com/openjdk/jdk/pull/4138

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

I haven't seen any response to this comment I made a couple of days ago and I 
suspect it got missed
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
>
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:

> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new 
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {

Who or what prompted this change ?

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]

[Phil Race]

On Tue, 18 May 2021 21:44:43 GMT, Weijun Wang  wrote:

>> Please review the test changes for [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> With JEP 411 and the default value of `-Djava.security.manager` becoming 
>> `disallow`, tests calling `System.setSecurityManager()`  need 
>> `-Djava.security.manager=allow` when launched. This PR covers such changes 
>> for tier1 to tier3 (except for the JCK tests).
>> 
>> To make it easier to focus your review on the tests in your area, this PR is 
>> divided into multiple commits for different areas (~~serviceability~~, 
>> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d, 
>> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~, 
>> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is 
>> the same as how Skara adds labels, but there are several small tweaks:
>> 
>> 1. When a file is covered by multiple labels, only one is chosen. I make my 
>> best to avoid putting too many tests into `core-libs`. If a file is covered 
>> by `core-libs` and another label, I categorized it into the other label.
>> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the 
>> `xml` commit.
>> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the 
>> `rmi` commit.
>> 4. One file not covered by any label -- 
>> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in 
>> the `swing` commit.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> all files to minimize unnecessary merge conflict.
>> 
>> Please note that this PR can be integrated before the source changes for JEP 
>> 411, as the possible values of this system property was already defined long 
>> time ago in JDK 9.
>> 
>> Most of the change in this PR is a simple adding of 
>> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes 
>> it was not `othervm` and we add one. Sometimes there's no `@run` at all and 
>> we add the line.
>> 
>> There are several tests that launch another Java process that needs to call 
>> the `System.setSecurityManager()` method, and the system property is added 
>> to `ProcessBuilder`, `ProcessTools`, or the java command line (if the test 
>> is a shell test).
>> 
>> 3 langtools tests are added into problem list due to 
>> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>> 
>> 2 SQL tests are moved because they need different options on the `@run` line 
>> but they are inside a directory that has a `TEST.properties`:
>> 
>> rename test/jdk/java/sql/{testng/test/sql/othervm => 
>> permissionTests}/DriverManagerPermissionsTests.java (93%)
>> rename test/jdk/javax/sql/{testng/test/rowset/spi => 
>> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>>  ```
>> 
>> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   adjust order of VM options

The client changes are fine except for the one misplaced (c)

test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3:

> 1: /*
> 2:  * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights 
> reserved.
> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

Probably the (c) update was meant to be on the .sh file that is actually 
modified.

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4071

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Thu, 20 May 2021 07:06:00 GMT, Alan Bateman  wrote:

>> The JEP isn't PTT for 17 so there's plenty of time isn't there ?
>
> There are 827 files in this patch. Phil is right that adding SW at the class 
> level is introducing technical debt but if addressing that requires 
> refactoring and re-testing of AWT or font code then it would be better to 
> have someone from that area working on. Is there any reason why this can't be 
> going on now on awt-dev/2d-dev and integrated immediately after JEP 411? I 
> don't think we should put Max through the wringer here as there are too many 
> areas to cover.

Are you suggesting that the patch doesn't need testing as it is ? It should be 
the same either way.
It is very straight forward to run the automated tests across all platforms 
these days.
I get the impression that no one is guaranteeing to do this straight after 
integration.
It sounds like it is up for deferral if time runs out.

The amount of follow-on work that I am hearing about here, and may be for 
tests, does not  make it sound
like this JEP is nearly as done as first presented.

If there was some expectation that groups responsible for an area would get 
involved with this
issue which I am assured was already known about, then why was it not raised 
before and made
part of the plan ?

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Thu, 20 May 2021 04:05:23 GMT, Phil Race  wrote:

>> By converting JDK-8267432 to a bug, there is an extra benefit that we can 
>> fix it after RDP. So I'll convert it now.
>
> That is unfortunate, but nonetheless I think required to be done.
> We have acknowledeged this can't reasonably be called an RFE, so the JEP is 
> introducing bugs/technical debt/call it what you will. This should generally 
> be part of a sandbox for the JEP and fixed before integration of the JEP.
> From my point of view it is a blocker.

The JEP isn't PTT for 17 so there's plenty of time isn't there ?

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Thu, 20 May 2021 02:09:57 GMT, Weijun Wang  wrote:

>> I can make it a bug.
>> 
>> I don't want to do it here is because it involves indefinite amount of 
>> manual work and we will see everyone having their preferences. The more time 
>> we spend on this PR the more likely there will be merge conflict. There are 
>> just too many files here.
>> 
>> And no matter if we include that change in this PR or after it, it will be 
>> after the automatic conversion. In fact, the data about which cases are more 
>> worth fixing come from the automatic conversion itself. Also, as the manual 
>> work will be done part by part, if the automatic conversion is after it, 
>> there will be rounds and rounds of history rewriting and force push. This is 
>> quite unfriendly to the reviewers.
>> 
>> Altogether, there are 117 class-level annotations. Unfortunately, 
>> `java.awt.Component` is the one with the biggest class -- estimated number 
>> of bytes that the annotation covers is over 380K. In the client area, the 
>> 2nd place is `java.awt.Container`, and then we have 
>> `sun.font.SunFontManager`, `java.awt.Window`, and `java.awt.Toolkit` which 
>> are over 100KB, and other 25 classes over 10KB, and other 11 classes smaller 
>> than 10KB.
>
> By converting JDK-8267432 to a bug, there is an extra benefit that we can fix 
> it after RDP. So I'll convert it now.

That is unfortunate, but nonetheless I think required to be done.
We have acknowledeged this can't reasonably be called an RFE, so the JEP is 
introducing bugs/technical debt/call it what you will. This should generally be 
part of a sandbox for the JEP and fixed before integration of the JEP.
>From my point of view it is a blocker.

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 22:14:20 GMT, Weijun Wang  wrote:

>> I don't think it is a separate P4 enhancement (?) that someone will (maybe) 
>> do next release.
>> I think it should all be taken care of as part of this proposed change.
>
> I just made it P3 (P4 was the default value), and I will target it to 17 once 
> JEP 411 is targeted 17. But I think it's probably not a good idea to include 
> it inside *this* PR. There are some middle ground where it's debatable if a 
> change is worth doing (Ex: which is uglier between an 
> a-liitle-faraway-annotation and a temporary variable?) so it's not obvious 
> what the scope of the refactoring can be. Thus it will be divided into 
> smaller sub-tasks. It's not totally impossible that part of the work will be 
> delayed to next release.

Well .. as an enhancement (P3 or otherwise) I can see it being dropped and 
definitely if it misses the fork,
and I don't get why you can't do it here. And if it isn't done the JEP isn't 
really ready.
I already pasted the patch for Component.java and it took about 60 seconds to 
do that.
Then yes, you would have to deal with the fact that now you need to reapply 
your automated tool to the file but this is just work you'd have to have done 
anyway if it was already refactored.

I only *noticed* Component and Container. And stopped there to raise the 
question. How many more cases are there ?

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 21:53:35 GMT, Weijun Wang  wrote:

>> That's a sad limitation of the annotation stuff then, but I don't think that 
>> it is insurmountable.
>> You can define a static private method to contain this and call it from the 
>> static initializer block.
>> Much better than applying the annotation to an entire class.
>> 
>> --- a/src/java.desktop/share/classes/java/awt/Component.java
>> +++ b/src/java.desktop/share/classes/java/awt/Component.java
>> @@ -618,6 +618,17 @@ public abstract class Component implements 
>> ImageObserver, MenuContainer,
>>   */
>>  static boolean isInc;
>>  static int incRate;
>> +
>> +private static void initIncRate() {
>> +String s = java.security.AccessController.doPrivileged(
>> + new 
>> GetPropertyAction("awt.image.incrementaldraw"));
>> +isInc = (s == null || s.equals("true"));
>> +
>> +s = java.security.AccessController.doPrivileged(
>> +  new GetPropertyAction("awt.image.redrawrate"));
>> +incRate = (s != null) ? Integer.parseInt(s) : 100;
>> +}
>> +
>>  static {
>>  /* ensure that the necessary native libraries are loaded */
>>  Toolkit.loadLibraries();
>> @@ -625,14 +636,7 @@ public abstract class Component implements 
>> ImageObserver, MenuContainer,
>>  if (!GraphicsEnvironment.isHeadless()) {
>>  initIDs();
>>  }
>> -
>> -String s = java.security.AccessController.doPrivileged(
>> -   new 
>> GetPropertyAction("awt.image.incrementaldraw"));
>> -isInc = (s == null || s.equals("true"));
>> -
>> -s = java.security.AccessController.doPrivileged(
>> -new 
>> GetPropertyAction("awt.image.redrawrate"));
>> -incRate = (s != null) ? Integer.parseInt(s) : 100;
>> +initIncRate();
>>  }
>
> Correct, there are ways to modify the code to make it more 
> annotation-friendly. We thought about whether it's good to do it before 
> adding the annotations or after it. Our decision now is to do it after 
> because it will be more easy to see why it's necessary and we can take time 
> to do them little by little. A new enhancement at 
> https://bugs.openjdk.java.net/browse/JDK-8267432 is filed.

I don't think it is a separate P4 enhancement (?) that someone will (maybe) do 
next release.
I think it should all be taken care of as part of this proposed change.

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 18:38:39 GMT, Weijun Wang  wrote:

>> src/java.desktop/share/classes/java/awt/Component.java line 217:
>> 
>>> 215:  * @author  Sami Shaio
>>> 216:  */
>>> 217: @SuppressWarnings("removal")
>> 
>> Why is this placed on the *entire class* ??
>> This class is 10535 lines long and mentions AccessControl something like 8 
>> places it uses AccessController or AcessControlContext.
>
> This happens when a deprecated method is called inside a static block. The 
> annotation can only be added to a declaration and here it must be the whole 
> class. The call in this file is
> 
> s = java.security.AccessController.doPrivileged(
> new 
> GetPropertyAction("awt.image.redrawrate"));

That's a sad limitation of the annotation stuff then, but I don't think that it 
is insurmountable.
You can define a static private method to contain this and call it from the 
static initializer block.
Much better than applying the annotation to an entire class.

--- a/src/java.desktop/share/classes/java/awt/Component.java
+++ b/src/java.desktop/share/classes/java/awt/Component.java
@@ -618,6 +618,17 @@ public abstract class Component implements ImageObserver, 
MenuContainer,
  */
 static boolean isInc;
 static int incRate;
+
+private static void initIncRate() {
+String s = java.security.AccessController.doPrivileged(
+ new 
GetPropertyAction("awt.image.incrementaldraw"));
+isInc = (s == null || s.equals("true"));
+
+s = java.security.AccessController.doPrivileged(
+  new GetPropertyAction("awt.image.redrawrate"));
+incRate = (s != null) ? Integer.parseInt(s) : 100;
+}
+
 static {
 /* ensure that the necessary native libraries are loaded */
 Toolkit.loadLibraries();
@@ -625,14 +636,7 @@ public abstract class Component implements ImageObserver, 
MenuContainer,
 if (!GraphicsEnvironment.isHeadless()) {
 initIDs();
 }
-
-String s = java.security.AccessController.doPrivileged(
-   new 
GetPropertyAction("awt.image.incrementaldraw"));
-isInc = (s == null || s.equals("true"));
-
-s = java.security.AccessController.doPrivileged(
-new 
GetPropertyAction("awt.image.redrawrate"));
-incRate = (s != null) ? Integer.parseInt(s) : 100;
+initIncRate();
 }

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

src/java.desktop/share/classes/java/awt/Component.java line 217:

> 215:  * @author  Sami Shaio
> 216:  */
> 217: @SuppressWarnings("removal")

Why is this placed on the *entire class* ??
This class is 10535 lines long and mentions AccessControl something like 8 
places it uses AccessController or AcessControlContext.

src/java.desktop/share/classes/java/awt/Container.java line 97:

> 95:  * @since 1.0
> 96:  */
> 97: @SuppressWarnings("removal")

Same issue as with Component. a > 5,000 line file that uses AccessController in 
just 4 places.

Where else are you adding this to entire classes instead of the specific site ?

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

This change is so large that github won't even display the list of files so I 
can't jump to where I want to go !
And when I try to scroll I get just a blank page.

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:

> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new 
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {

Who or what prompted this change ?

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Integrated: 8265793: Remove duplicate jtreg TEST.groups references for some client tests

[Phil Race]

On Thu, 22 Apr 2021 20:59:22 GMT, Phil Race  wrote:

> Delete some duplicates

This pull request has now been integrated.

Changeset: b84f6901
Author:    Phil Race 
URL:   https://git.openjdk.java.net/jdk/commit/b84f6901
Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod

8265793: Remove duplicate jtreg TEST.groups references for some client tests

Reviewed-by: erikj

-

PR: https://git.openjdk.java.net/jdk/pull/3642

RFR: 8265793: Remove duplicate jtreg TEST.groups references for some client tests

[Phil Race]

Delete some duplicates

-

Commit messages:
 - 8265793: Remove duplicate jtreg TEST.groups references for some client tests

Changes: https://git.openjdk.java.net/jdk/pull/3642/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3642=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8265793
  Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3642.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3642/head:pull/3642

PR: https://git.openjdk.java.net/jdk/pull/3642

Re: RFR: 8189198: Add "forRemoval = true" to Applet API deprecations [v2]

[Phil Race]

On Thu, 25 Mar 2021 22:58:53 GMT, Andy Herrick  wrote:

>> implementation of
>> JDK-8256145: JEP 398: Deprecate the Applet API for Removal
>
> Andy Herrick has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 301 additional 
> commits since the last revision:
> 
>  - 8189198: Add "forRemoval = true" to Applet API deprecations
>  - Merge branch 'master' into 8189198
>  - 8260862: JFR: New configure command for the jfr tool
>
>Reviewed-by: mgronlun
>  - 8264161: BigDecimal#stripTrailingZeros can throw undocumented 
> ArithmeticException
>
>Reviewed-by: bpb
>  - 8262081: 
> vmTestbase/nsk/jdi/ThreadDeathRequest/addThreadFilter/addthreadfilter001/TestDescription.java
>  failed with "ERROR: eventSet1.size() != 3 :: 2"
>
>Reviewed-by: cjplummer, lmesnik, sspitsyn
>  - 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised 
> exception handling
>
>Reviewed-by: jnimeh
>  - 8253795: Implementation of JEP 391: macOS/AArch64 Port
>8253816: Support macOS W^X
>8253817: Support macOS Aarch64 ABI in Interpreter
>8253818: Support macOS Aarch64 ABI for compiled wrappers
>8253819: Implement os/cpu for macOS/AArch64
>8253839: Update tests and JDK code for macOS/Aarch64
>8254941: Implement Serviceability Agent for macOS/AArch64
>8255776: Change build system for macOS/AArch64
>8262903: [macos_aarch64] Thread::current() called on detached thread
>
>Co-authored-by: Vladimir Kempik 
>Co-authored-by: Bernhard Urban-Forster 
>Co-authored-by: Ludovic Henry 
>Co-authored-by: Monica Beckwith 
>Reviewed-by: erikj, ihse, prr, cjplummer, stefank, gziemski, aph, 
> mbeckwit, luhenry
>  - 4833719: (bf) Views of MappedByteBuffers are not MappedByteBuffers, and 
> cannot be forced
>
>Reviewed-by: adinn
>  - 8264165: jpackage BasicTest fails after JDK-8220266: Check help text 
> contains plaform specific parameters
>
>Reviewed-by: herrick, dcubed
>  - 8263454: com.apple.laf.AquaFileChooserUI ignores the result of 
> String.trim()
>
>Reviewed-by: serb, pbansal, kizune, trebari, psadhukhan
>  - ... and 291 more: 
> https://git.openjdk.java.net/jdk/compare/00b7b36f...026f09c8

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/2920

Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v3]

[Phil Race]

On Sun, 14 Mar 2021 19:35:25 GMT, Сергей Цыпанов 
 wrote:

>> In some cases wrapping of `InputStream` with `BufferedInputStream` is 
>> redundant, e.g. in case the wrapped one is `ByteArrayOutputStream` which 
>> does not require any buffer having one within.
>> 
>> Other cases are related to reading either a byte or short `byte[]`: in both 
>> cases `BufferedInputStream.fill()` will be called resulting in load of much 
>> bigger amount of data (8192 by default) than required.
>
> Сергей Цыпанов has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use InputStream.readNBytes() and fix JLinkNegativeTest

2d change is fine.

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2992

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v18]

[Phil Race]

On Wed, 17 Feb 2021 12:36:10 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 88 commits:
> 
>  - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos
>  - Re-do safefetch.hpp
>  - Merge remote-tracking branch 'origin/jdk/8261075-stubroutines-inline' into 
> jdk-macos
>  - stubRoutines.inline.hpp -> safefetch.hpp
>  - Update copyrights
>  - Merge remote-tracking branch 'upstream/jdk/master' into 
> 8261075-stubroutines-inline
>  - Merge remote-tracking branch 'upstream/jdk/master' into 
> 8261075-stubroutines-inline
>  - Extract SafeFetch32/N to stubRoutines.inline.hpp
>  - Revert "Extract SafeFetch32/N to stubRoutines.inline.hpp"
>
>This reverts commit b873c25f31dd21349d140b790713cc9ccb5f2dc0.
>  - Merge pull request #9 from VladimirKempik/pull/2200
>
>Removed unused variables
>  - ... and 78 more: 
> https://git.openjdk.java.net/jdk/compare/b955f85e...ab72613c

Looks like the compiler warning changess are now the only desktop changes.
That is fine by me.

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2200

Integrated: 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4

[Phil Race]

On Thu, 4 Feb 2021 02:32:31 GMT, Phil Race  wrote:

> remove un-needed disabling now JNF has gone ..

This pull request has now been integrated.

Changeset: 3bb6a3d2
Author:    Phil Race 
URL:   https://git.openjdk.java.net/jdk/commit/3bb6a3d2
Stats: 8 lines in 2 files changed: 0 ins; 8 del; 0 mod

8261109: [macOS] Remove disabled warning for JNF in 
make/autoconf/flags-cflags.m4

Reviewed-by: serb, ihse, erikj

-

PR: https://git.openjdk.java.net/jdk/pull/2396

Re: RFR: 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4 [v2]

[Phil Race]

On Thu, 4 Feb 2021 11:42:48 GMT, Magnus Ihse Bursie  wrote:

>> Phil Race has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Remove condition that should have been fixed as part of 8257858
>
> Marked as reviewed by ihse (Reviewer).

Magnus pointed out a condition that I think should have been removed in the fix 
for
8257858 : Remove JNF dependency from libosxsecurity/KeystoreImpl.m

since its a build related change too, and I've verified that after removing it 
we still build, I am rolling it in here, if that's OK with folks

-

PR: https://git.openjdk.java.net/jdk/pull/2396

Re: RFR: 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4 [v2]

[Phil Race]

> remove un-needed disabling now JNF has gone ..

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  Remove condition that should have been fixed as part of 8257858

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2396/files
  - new: https://git.openjdk.java.net/jdk/pull/2396/files/34dcbfb1..93fd193f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2396=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2396=00-01

  Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2396.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2396/head:pull/2396

PR: https://git.openjdk.java.net/jdk/pull/2396

Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v3]

[Phil Race]

On Fri, 29 Jan 2021 22:47:52 GMT, Weijun Wang  wrote:

>> This fix covers both
>> 
>> - [[macOS]: Remove JNF dependency from 
>> libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858)
>> - [[macOS]: Remove JNF dependency from 
>> libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860)
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   phil comment

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1845

Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v2]

[Phil Race]

On Fri, 29 Jan 2021 21:47:32 GMT, Weijun Wang  wrote:

>> src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 619:
>> 
>>> 617: (*env)->ReleaseCharArrayElements(env, passwordObj, 
>>> passwordChars,
>>> 618: JNI_ABORT);
>>> 619: }
>> 
>> Although you have it in the later code, here you are missing
>>  @catch (NSException *e) { 
>>  NSLog(@"%@", [e callStackSymbols]); 
>>  }
>
> Will add.
> 
> BTW, will these be shown on stdout or stderr? If so, is this a good idea?

should be stderr. Whether to log is up to you. You could wrap it in something 
that checks for a debug build.
The idea is that if this ever happens there is a serious problem. NSException 
is only thrown (by the OS frameworks) in supposedly fatal scenarios.

>> src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m line 
>> 57:
>> 
>>> 55: }
>>> 56: }
>>> 57: (*localVM)->DetachCurrentThread(localVM);
>> 
>> I think you only want to detach if you actually attached ! you don't want to 
>> be detaching VM threads.
>
> So I should remember how env was retrieved and only detach when it's from 
> `AttachCurrentThreadAsDaemon`? In fact, in my test the plain `GetEnv` has 
> never succeeded and it was always attached.

Yes that is the idea. BTW which thread was it attached to in what you saw ?

-

PR: https://git.openjdk.java.net/jdk/pull/1845

Re: RFR: 8254702: jpackage app launcher crashes on CentOS

[Phil Race]

On Fri, 29 Jan 2021 21:33:40 GMT, Alexey Semenyuk  wrote:

> Fix for https://bugs.openjdk.java.net/browse/JDK-8254702
> 
> The fix splits Linux app launcher in app launcher and launcher shared lib. 
> App launcher is pure C and doesn't have C++ code. App launcher lib 
> incorporates bulk of C++ code from app launcher. 
> At startup app launcher loads launcher shared lib and calls functions it 
> provides to get data for launching JVM (path to jli lib and arguments for 
> JLI_Launch function call).
> App launcher unloads launcher shared lib before launching JVM to remove C++ 
> runtime from the process memory.
> 
> Getting rid of C++ code from app launcher required to rewrite app 
> installation location lookup code from C++ to C. LinuxPackage.c source is C 
> alternative for 
> https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Package.cpp
>  and 
> https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Executor.cpp.
> 
> Layout of jpackage's native code changed:
> - `common`: code shared between all jpackage binaries.
> - `applauncher`: launcher only code.
> - `applauncherlib`: launcher lib code on Linux and launcher code on other 
> platforms.
> - `applaunchercommon`: code shared between launcher and launcher lib on Linux 
> and launcher code on other platforms.

So after this change if you bundle and run an app on Linux and then do "ps" .. 
what is shown to be running ? Java or the app-name you expected ?

-

PR: https://git.openjdk.java.net/jdk/pull/2320

Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v2]

[Phil Race]

On Fri, 29 Jan 2021 14:57:56 GMT, Weijun Wang  wrote:

>> This fix covers both
>> 
>> - [[macOS]: Remove JNF dependency from 
>> libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858)
>> - [[macOS]: Remove JNF dependency from 
>> libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860)
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   same behavior as before -- empty realm map

make/modules/java.security.jgss/Lib.gmk line 84:

> 82: $(call SET_SHARED_LIBRARY_ORIGIN), \
> 83: LIBS := -framework Cocoa -framework SystemConfiguration \
> 84: -framework Kerberos -ljava, \

The need to add -ljava is interesting. It implies we were getting something 
from the platform that usually we'd expect to come from the JDK itself ??

src/java.base/macosx/classes/apple/security/KeychainStore.java line 820:

> 818: private void createKeyEntry(String alias, long creationDate, long 
> secKeyRef,
> 819: long[] secCertificateRefs, byte[][] 
> rawCertData) {
> 820: KeyEntry ke = new KeyEntry();

removing these exceptions is presumably just clean up - not directly related ??

src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 28:

> 26: #import "apple_security_KeychainStore.h"
> 27: #include "jni.h"
> 28: #include "jni_util.h"

jni_util.h includes jni.h so I don't understand the need for this change.
Also why did you change import to include ? import is the Obj-C norm ...

src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 619:

> 617: (*env)->ReleaseCharArrayElements(env, passwordObj, 
> passwordChars,
> 618: JNI_ABORT);
> 619: }

Although you have it in the later code, here you are missing
 @catch (NSException *e) { 
 NSLog(@"%@", [e callStackSymbols]); 
 }

src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m line 41:

> 39: if ([keys count] == 0) return;
> 40: if (![keys containsObject:KERBEROS_DEFAULT_REALMS] && ![keys 
> containsObject:KERBEROS_DEFAULT_REALM_MAPPINGS]) return;
> 41: //JNFPerformEnvBlock(JNFThreadDetachOnThreadDeath | 
> JNFThreadSetSystemClassLoaderOnAttach | JNFThreadAttachAsDaemon, ^(JNIEnv 
> *env) {

remove commented out code

src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m line 57:

> 55: }
> 56: }
> 57: (*localVM)->DetachCurrentThread(localVM);

I think you only want to detach if you actually attached ! you don't want to be 
detaching VM threads.

-

PR: https://git.openjdk.java.net/jdk/pull/1845

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Phil Race]

On Mon, 25 Jan 2021 23:34:04 GMT, Phil Race  wrote:

>> that sounds good to me, I am just afraid to break intel mac on older macos 
>> versions with this change.
>
> That may actually be a valid concern. Both say macOS 10.12+ ...  which might 
> conflict with the 10.9 target.

Maybe you should just file a bug after all for this to be dealt with separately.
Certainly if it is NOT fixed now such a bug needs to be filed.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Phil Race]

On Mon, 25 Jan 2021 22:47:33 GMT, Vladimir Kempik  wrote:

>> 1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask
>> 2) So maybe rather than the deprecation suppression  you could change both 
>> constants to the new ones.
>> Ordinarily I'd say let someone else do that but this looks like a simple 
>> obvious change and is dwarfed by all the other changes going on for Mac ARM 
>> ...
>
> that sounds good to me, I am just afraid to break intel mac on older macos 
> versions with this change.

That may actually be a valid concern. Both say macOS 10.12+ ...  which might 
conflict with the 10.9 target.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Phil Race]

On Mon, 25 Jan 2021 22:25:48 GMT, Vladimir Kempik  wrote:

>> Are you doing something somewhere to change the target version of macOS or 
>> SDK ? I had no such problem.
>> I think we currently target a macOS 10.9 and if you are changing that it 
>> would need discussion.
>> If you are changing it only for Mac ARM that may make more sense .. 
>> 
>> And these appear to be just API churn by Apple
>> NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst
>> 
>> https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc
>> 
>> NSBorderlessWindowMask is replaced by NSWindowStyleMask
>> 
>> https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ
>
> Min_macos version is changed to 11.0 for macos_aarch64
> 
> https://github.com/openjdk/jdk/pull/2200/files/0c2cb0a372bf1a8607810d773b53d6959616a816#diff-7cd97cdbeb3053597e5d6659016cdf0f60b2c412bd39934a43817ee0b717b7a7R136

1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask
2) So maybe rather than the deprecation suppression  you could change both 
constants to the new ones.
Ordinarily I'd say let someone else do that but this looks like a simple 
obvious change and is dwarfed by all the other changes going on for Mac ARM ...

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Phil Race]

On Mon, 25 Jan 2021 21:18:59 GMT, Vladimir Kempik  wrote:

>> Hello
>> I believe it was a workaround for issues with xcode 12.2 in early beta days.
>> Those issues were fixed later in upstream jdk, so most likely we need to 
>> remove these workarounds.
>
> It seems these workarounds are still needed:
> 
> jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:300:39: 
> error: 'NSAlphaFirstBitmapFormat' is deprecated: first deprecated in macOS 
> 10.14 [-Werror,-Wdeprecated-declarations]
> bitmapFormat: NSAlphaFirstBitmapFormat | 
> NSAlphaNonpremultipliedBitmapFormat
>   ^~~~
>   NSBitmapFormatAlphaFirst
> 
> jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:438:34: 
> error: 'NSBorderlessWindowMask' is deprecated: first deprecated in macOS 
> 10.12 [-Werror,-Wdeprecated-declarations]
>   styleMask: NSBorderlessWindowMask
>  ^~
>  NSWindowStyleMaskBorderless

Are you doing something somewhere to change the target version of macOS or SDK 
? I had no such problem.
I think we currently target a macOS 10.9 and if you are changing that it would 
need discussion.
If you are changing it only for Mac ARM that may make more sense .. 

And these appear to be just API churn by Apple
NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst

https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc

NSBorderlessWindowMask is replaced by NSWindowStyleMask

https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Phil Race]

On Mon, 25 Jan 2021 19:38:16 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Refactor CDS disabling
>  - Redo builsys support for aarch64-darwin

make/modules/java.desktop/lib/Awt2dLibraries.gmk line 573:

> 571: EXTRA_HEADER_DIRS := $(LIBFONTMANAGER_EXTRA_HEADER_DIRS), \
> 572: WARNINGS_AS_ERRORS_xlc := false, \
> 573: DISABLED_WARNINGS_clang := deprecated-declarations, \

I see this added here and in another location. What is deprecated ? You really 
need to explain these sorts of things.
I've built JDK using Xcode 12.3 and not had any need for this.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]

[Phil Race]

On Sun, 24 Jan 2021 15:32:59 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Address feedback for signature generators
>  - Enable -Wformat-nonliteral back

Changes requested by prr (Reviewer).

src/java.desktop/share/native/libharfbuzz/hb-common.h line 113:

> 111: 
> 112: #define HB_TAG(c1,c2,c3,c4) 
> ((hb_tag_t)uint32_t)(c1)&0xFF)<<24)|(((uint32_t)(c2)&0xFF)<<16)|(((uint32_t)(c3)&0xFF)<<8)|((uint32_t)(c4)&0xFF)))
> 113: #define HB_UNTAG(tag)   (char)(((tag)>>24)&0xFF), 
> (char)(((tag)>>16)&0xFF), (char)(((tag)>>8)&0xFF), (char)((tag)&0xFF)

I need a robust explanation of these changes.
They are not mentioned, nor are they in upstream harfbuzz.
Likely these should be pushed to upstream first if there is a reason for them.

src/java.desktop/share/native/libharfbuzz/hb-coretext.cc line 193:

> 191:* crbug.com/549610 */
> 192:   // 0x0007 stands for "kCTVersionNumber10_10", see CoreText.h
> 193: #if TARGET_OS_OSX && MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_10

I need a robust explanation of these changes.
They are not mentioned, nor are they in upstream harfbuzz.
Likely these should be pushed to upstream first if there is a reason for them.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8257733: Move module-specific data from make to respective module [v4]

[Phil Race]

On Tue, 15 Dec 2020 22:56:15 GMT, Magnus Ihse Bursie  wrote:

>> A lot (but not all) of the data in make/data is tied to a specific module. 
>> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
>> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
>> make for the whole build.) 
>> 
>> These data files should move to the module they belong to. The are, after 
>> all, "source code" for that module that is "compiler" into resulting 
>> deliverables, for that module. (But the "source code" language is not Java 
>> or C, but typically a highly domain specific language or data format, and 
>> the "compilation" is, often, a specialized transformation.) 
>> 
>> This misplacement of the data directory is most visible at code review time. 
>> When such data is changed, most of the time build-dev (or the new build 
>> label) is involved, even though this has nothing to do with the build. While 
>> this is annoying, a worse problem is if the actual team that needs to review 
>> the patch (i.e., the team owning the module) is missed in the review.
>> 
>> ### Modules reviewed
>> 
>> - [x] java.base
>> - [ ] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains eight additional 
> commits since the last revision:
> 
>  - Update comment refering to "make" dir
>  - Move new symbols to jdk.compiler
>  - Merge branch 'master' into shuffle-data
>  - Move macosxicons from share to macosx
>  - Move to share/data, and move jdwp.spec to java.se
>  - Update references in test
>  - Step 2: Update references
>  - First stage, move actual data files

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: [jdk16] RFR: 8258827: ProblemList Naming/DefaultRegistryPort.java and Naming/legalRegistryNames/LegalRegistryNames.java on Windows

[Phil Race]

On Tue, 22 Dec 2020 16:56:33 GMT, Daniel D. Daugherty  
wrote:

> ProblemList two java/rmi/Naming tests on Windows in order to reduce the
> noise in the JDK16 CI. This is a trivial fix.

overdue

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk16/pull/58

Re: RFR: 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo

[Phil Race]

On Sun, 20 Dec 2020 20:22:48 GMT, Andrey Turbanov 
 wrote:

>> jrtfs is compiled twice, the second is to --release 8 so it can be packaged 
>> into jrt-fs.jar for use by IDEs/tools running on older JDK releases. So need 
>> to be careful with the changes here as it will likely causing build breakage.
>> 
>> We try to keep the changes to ASM to a minimum, might be better to leave 
>> this change out of the patch.
>> 
>> One or two of the sources changes should probably uses Files.copy, e.g. 
>> ZipPath, sjavac/CopyFile.java.
>
>> One or two of the sources changes should probably uses Files.copy, e.g. 
>> ZipPath, sjavac/CopyFile.java.
> 
> Good idea! Replaced in few places. But not in ZipPath: it's actually 
> implementation of underlying call from Files.copy: 
> `jdk.nio.zipfs.ZipFileSystemProvider#copy`. So, `Files.copy` call will be 
> recursive.

So these changes are all over the place which means no one person knows how to 
test all of it.
Have you run the sound, swing tests, and the  printing tests on unix and 
windows ?
For printing for sure you'll need to do some manual testing.

-

PR: https://git.openjdk.java.net/jdk/pull/1853

Re: RFR: JDK-8257539: tools/jpackage/windows/WinL10nTest.java unpack.bat failed with Exit code: 1618

[Phil Race]

On Wed, 9 Dec 2020 18:58:54 GMT, Andy Herrick  wrote:

> Same code change as https://github.com/openjdk/jdk/pull/1676 that got messed 
> up with merge

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1720

Re: RFR: 8257620: Do not use objc_msgSend_stret to get macOS version

[Phil Race]

On Wed, 2 Dec 2020 20:04:12 GMT, Anton Kozlov  wrote:

>> Surely these days you can just call [NSProcessInfo operatingSystemVersion] 
>> directly ?
>> If I read the doc below it is in the 10.10 SDK and later.
>> https://developer.apple.com/documentation/foundation/nsprocessinfo/1410906-operatingsystemversion?language=occ
>
> Unfortunately, no. AFAIK, the minimum target version is 10.9 
> https://github.com/openjdk/jdk/blob/master/make/autoconf/flags.m4#L133, so I 
> had to keep indirection.

I wonder if we should be "upping" that to something later.
10.9 is over 7 years old and has been out of support for what - 4 years ?

-

PR: https://git.openjdk.java.net/jdk/pull/1569

Re: RFR: 8257620: Do not use objc_msgSend_stret to get macOS version

[Phil Race]

On Wed, 2 Dec 2020 17:34:00 GMT, Anton Kozlov  wrote:

> Please review a small change that replaces use of objc_msgSend_stret in macOS 
> platform code with pure ObjC code. It's also a prerequisite for macOS/AArch64 
> support, where objc_msgSend_stret is not available.

Surely these days you can just call [NSProcessInfo operatingSystemVersion] 
directly ?
If I read the doc below it is in the 10.10 SDK and later.
https://developer.apple.com/documentation/foundation/nsprocessinfo/1410906-operatingsystemversion?language=occ

-

PR: https://git.openjdk.java.net/jdk/pull/1569

Re: RFR: 8066622 8066637: remove deprecated using java.io.File.toURL() in internal classes

[Phil Race]

On Sat, 7 Nov 2020 07:55:03 GMT, Sebastian Ritter 
 wrote:

> In result of Javadoc to do not use java.io.File.toURL()
> Change use  java.io.File.toURL().toURI() instead.

You reference a desktop bug that discusses many, many deprecations
and skara has identified https://bugs.openjdk.java.net/browse/JDK-8066622
as the issue being fixed.
Yet you propose to fix precisely one of these.
But when this is integrated that bug will be closed out as resolved.
I think you need a new bug about JUST the changes you are making.
So I don't think you should use that bug id any where in this PR.
And I'd like to hear whether you actually *tested* splashscreen with your 
change ? I see no sign that you did.

-

Changes requested by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1108

Re: RFR: 8248122: java.base should not handle JavaFX application in a specific way

[Phil Race]

On Sat, 31 Oct 2020 16:10:23 GMT, Kevin Rushforth  wrote:

>> This will cause a regression in behavior. It will break existing JavaFX 
>> applications that do not have a main program. It could also break 
>> applications that create or use certain JavaFX objects in the class 
>> initializer of their JavaFX application.
>> 
>> There was [some initial 
>> discussion](https://bugs.openjdk.java.net/browse/JDK-8202553?focusedCommentId=14176584=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14176584)
>>  around doing this as a follow-on to the removal of JavaFX from JDK 11, but 
>> if it is to be done, it needs to be discussed first. It would need to be 
>> done using a process similar to deprecation-for-removal. We would need to 
>> make changes in the JDK and/or JavaFX to warn about this in one release, and 
>> then remove it in the following. A CSR would be needed for both steps.
>> 
>> I note that while I disagree with the rationale described in 
>> [JDK-8248122](https://bugs.openjdk.java.net/browse/JDK-8248122) for making 
>> this change, I am not necessarily opposed to the change itself.
>
> This also needs to be discussed on the openjfx-dev mailing list, since it 
> will have behavioral compatibility implications for JavaFX.

Indeed this is very much of out the blue and the pre-existence of a bug report 
discussing this does not mean it
has been accepted to be done .. 

And launcher changes need to be done carefully. I would expect them to come 
from someone who has a long history of contributions in this area and has had 
extensive discussions with stakeholders before moving on to code ..

-

PR: https://git.openjdk.java.net/jdk/pull/978

Re: RFR: 8255299: Drop explicit zeroing at instantiation of Atomic* objects

[Phil Race]

On Thu, 22 Oct 2020 20:46:15 GMT, Сергей Цыпанов 
 wrote:

> As discussed in https://github.com/openjdk/jdk/pull/510 there is never a 
> reason to explicitly instantiate any instance of `Atomic*` class with its 
> default value, i.e. `new AtomicInteger(0)` could be replaced with `new 
> AtomicInteger()` which is faster:
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @BenchmarkMode(value = Mode.AverageTime)
> public class AtomicBenchmark {
>   @Benchmark
>   public Object defaultValue() {
> return new AtomicInteger();
>   }
>   @Benchmark
>   public Object explicitValue() {
> return new AtomicInteger(0);
>   }
> }
> THis benchmark demonstrates that `explicitValue()` is much slower:
> Benchmark  Mode  Cnt   Score   Error  Units
> AtomicBenchmark.defaultValue   avgt   30   4.778 ± 0.403  ns/op
> AtomicBenchmark.explicitValue  avgt   30  11.846 ± 0.273  ns/op
> So meanwhile https://bugs.openjdk.java.net/browse/JDK-8145948 is still in 
> progress we could trivially replace explicit zeroing with default 
> constructors gaining some performance benefit with no risk.
> 
> I've tested the changes locally, both tier1 and tier 2 are ok. 
> 
> Could one create an issue for tracking this?

client changes are fine

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/818

Re: RFR: JDK-8252870: Finalize (remove "incubator" from) jpackage [v2]

[Phil Race]

On Tue, 13 Oct 2020 14:48:40 GMT, Andy Herrick  wrote:

>> JDK-8252870: Finalize (remove "incubator" from) jpackage
>
> Andy Herrick has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8252870: Finalize (remove "incubator" from) jpackage
> - reverting two auto-generated files, and changing module-info to "@since 
> 16"

Amazing how many places the word incubator appeared.
I am approving on the understanding that all the automated jpackage tests pass 
on all platforms.

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/633

Re: RFR: 8250859: Address reliance on default constructors in the Accessibility APIs

[Phil Race]

On Tue, 15 Sep 2020 10:04:49 GMT, Conor Cleary  wrote:

> This issue relates to JDK-8250639 '☂ Address reliance on default constructors 
> in the java.desktop module'. The
> following classes have had an explicit no-arg constructor added, with a 
> protected access modifier and accompanying API
> description:
>  - Default ctor on com.sun.java.accessibility.util.SwingEventMonitor
>  - Default ctor on javax.accessibility.AccessibleContext
>  - Default ctor on javax.accessibility.AccessibleHyperlink
> 
> The following classes have had an explicit no-arg constructor added, with a 
> public access modifier and accompanying API
> description:
>  - Default ctor on javax.accessibility.AccessibleResourceBundle
>  - Default ctor on com.sun.java.accessibility.util.AWTEventMonitor
>  - Default ctor on com.sun.java.accessibility.util.AccessibilityEventMonitor
>  - Default ctor on com.sun.java.accessibility.util.AccessibilityListenerList
>  - Default ctor on com.sun.java.accessibility.util.EventID
> 
> specdiff:
> http://cr.openjdk.java.net/~ccleary/issues/webrevs-store/8250859/webrevs/webrev.00/specdiff/overview-summary.html
>  bug:
> https://bugs.openjdk.java.net/browse/JDK-8250859

This looks OK but the CSR needs updates first.

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/174

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

[Phil Race]

On Sun, 6 Sep 2020 13:57:19 GMT, Dmitriy Dumanskiy 
 wrote:

> **isEmpty** is faster + has less byte code + easier to read. Benchmarks could 
> be found
>   
> [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416).

1) This is un-necessary churn.
2) I can't even be sure I am finding the ones in my area because there's so 
much here
3) The ones I can find have no need of whatever performance improvement this 
might bring.
I think this whole PR should be withdrawn, and there may an attempt at 
measuring the benefits of this for the various
cases before submitting in appropriate smaller chunks. But I'll tell you now I 
don't see a need for the test updates
you are making.

-

Changes requested by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/29

Re: RFR: JDK-8250611: Cannot display splash screen on Windows

[Phil Race]

Only back to 11 and client support ended before that.

-Phil.

> On Aug 13, 2020, at 7:58 PM, Alexey Semenyuk  
> wrote:
> 
> Phil,
> 
> jpackage can be used to bundle apps with older JREs. Do you think it doesn't 
> make sense for this check given this?
> 
> - Alexey
> 
>> On 8/13/2020 6:03 PM, Philip Race wrote:
>> Do I understand correctly that you are checking if the client VM is being 
>> specified ?
>> I didn't think we had client VMs any more ..
>> 
>> -phil.
>> 
>>> On 8/13/20, 12:35 PM, Andy Herrick wrote:
>>> Please review this jpackage fix for issue [1] at [2].
>>> 
>>> In order to show splash screen from statically linked applauncher, we need 
>>> to load the dependent libraries of splashscreen.dll first (java.dll and 
>>> jvm.dll).
>>> 
>>> /Andy
>>> 
>>> [1] - https://bugs.openjdk.java.net/browse/JDK-8250611
>>> 
>>> [2] - http://cr.openjdk.java.net/~herrick/8250611/webrev.01/
>>> 
> 

Re: [14] Review Request: 8233827 Enable screenshots in the enhanced failure handler on Linux/macOS

[Phil Race]

I am not sure what the right mailing list(s) are for this change.
It definitely isn't a core-libs change. I think build-dev may be better.

I am also unclear when this failure handler is invoked and how all this 
machinery works.


It is only useful for headful tests and so I'd only want it enabled in 
such a case.
And we disable the failure handlers in the headful test jobs anyway 
because they seem

focused on taking pointless core dumps ...

So we need something that can be used with headful tests only and 
doesn't involve

re-enabling the other handlers.

Also why exclude Windows ? No easy way to get the screenshot ?

-phil.

On 12/11/19 1:00 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for JDK 14.

Bug: https://bugs.openjdk.java.net/browse/JDK-8233827
Fix: http://cr.openjdk.java.net/~serb/8233827/webrev.01

This change adds the "screen capture on the test failure" feature on 
macOS and Linux.
 - On Linux, it is implemented by the "gnome-screenshot" command(in 
case of
   multiscreen+xinerama    the whole big screen will be saved to the 
"screen.png" file).
 - On macOS it is implemented by the "screencapture" command, note 
that I have
   used 1 file per screen, if the number of screens less than 5, other 
files will be ignored.

Re: RFR: JDK-8235738: tools/jpackage/macosx/NameWithSpaceTest.java failed due to exit code 134

[Phil Race]

testForPresenseOnly  It should be spelt testForPresenceOnly -phil.




On 12/13/19 6:16 AM, Andy Herrick wrote:

I approve these changes.

My first thought was that, if reading output only after Process is 
complete is valid and safe, then why not do it that way all the time 
?  But comment in Process javadoc: "Because some native platforms only 
provide limited buffer size for standard input and output streams, 
failure to promptly write the input stream or read the output stream 
of the process may cause the process to block, or even deadlock." 
indicates is is prudent to do this only when necessary.


My second thought is that this indicates an underlying unidentified 
bug in the mac native code used by Process or ProcessBuilder , and 
although it would be better to identify and resolve the underlying 
problem, the fix is needed now, so this change seems appropriate.


/Andy

On 12/13/2019 1:01 AM, Alexander Matveev wrote:

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

Not sure why it happens, but reading output from "hdiutil attach" was 
not exiting immediately after process terminated with delays upto 20 
seconds always and in some case upto 10 minutes and thus test was 
timeout. Only possible workaround found is to wait for "hdiutil 
attach" process to exit before reading output. In this case call to 
"hdiutil attach" and reading output happens very fast and does not 
hang for long time. Generating simple DMG image went from ~40 seconds 
to ~20 seconds after fix. Test was done with verbose output enabled.


Thanks,
Alexander

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

[2] http://cr.openjdk.java.net/~almatvee/8235738/webrev.00/

Re: RFR: JDK-8235728: JDK-8212780 breaks builds with a custom X11 include path

[Phil Race]

ok. all good.

-phil

On 12/11/19 12:14 PM, Alexey Semenyuk wrote:

Yes, I did a test build.

- Alexey

On 12/11/2019 1:48 PM, Phil Race wrote:

Looks OK. I presume you did a test build in our build system ?

-phil

On 12/11/19 10:46 AM, Alexey Semenyuk wrote:

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

- adds $(X_CFLAGS) to compiler command line.

Patch contributed by Arthur Eubanks (aeuba...@google.com).

- Alexey

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

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

Re: RFR: 8235788: Changeset for JDK-8235252 pushed with wrong bug ID

[Phil Race]

+1

-phil

On 12/11/19 11:43 AM, Andy Herrick wrote:

sorry - fix is at:

[3] http://cr.openjdk.java.net/~herrick/8235788/webrev.01/

/Andy

On 12/11/2019 2:42 PM, Andy Herrick wrote:

Please review fix [1] for issue [2]

This is backing out the fix to JDK-8235252 so I can re-push it with 
the right comment


[1] 8235788  
Changeset for JDK-8235252 pushed with wrong bug ID


[2] https://bugs.openjdk.java.net/browse/JDK-8235788

/Andy

Re: RFR: JDK-8235728: JDK-8212780 breaks builds with a custom X11 include path

[Phil Race]

Looks OK. I presume you did a test build in our build system ?

-phil

On 12/11/19 10:46 AM, Alexey Semenyuk wrote:

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

- adds $(X_CFLAGS) to compiler command line.

Patch contributed by Arthur Eubanks (aeuba...@google.com).

- Alexey

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

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

Re: RFR: JDK-8234867: Issue warning for mutually exclusive options on jpackage command line

[Phil Race]

+1

-Phil.

> On Dec 9, 2019, at 2:36 PM, Alexander Matveev  
> wrote:
> 
> Looks good.
> 
> Thanks,
> Alexander
> 
>> On 12/9/2019 1:42 PM, Andy Herrick wrote:
>> Please review simple fix [2] for jpackage bug [1]
>> 
>> fixing error message for mutually exclusive options.
>> 
>> /Andy
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8234867
>> 
>> [2] http://cr.openjdk.java.net/~herrick/8234867/webrev.01/
>> 
> 

Re: RFR: JDK-8235601: redundant code in IOUtils.java

[Phil Race]

> [2] http://cr.openjdk.java.net/~herrick/8235601/webrev.01/

This does not bring up the expected webrev

-phil.

On 12/9/19 3:17 PM, Andy Herrick wrote:

Please review simple jpackage fir for issue [1] at [2]

/Andy

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

[2] http://cr.openjdk.java.net/~herrick/8235601/webrev.01/

/Andy

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

[Phil Race]

Ok. That makes sense. I was under the impression other tests were failing.
So only the one update is needed for this and it is arguably just an 
omission.


jpackage has a couple of types of requirement
1) run only on platforms that support jpackage - the @modiules will do this.
2) run only on a subset of (1) - ie one or more specific platforms that 
support jpackage


If the tests being updated are all of type (2) then the @requires 
platform may still
be merited but probably that is not the case - it sounds like (1), so I 
think Andy
should add what you suggest but I expect the @requires is harmless to 
leave in too ...


-phil.

On 12/7/19 1:47 PM, Igor Ignatev wrote:

As far as I can see only junit.java test is executed on Solaris and is the only 
one failing. jtreg uses @modules during test selection/filtering phase, so 
tests which have @modules A won’t be run on jdk which doesn’t have module A, 
hence it should be sufficient. If it’s not, we have a bug in jtreg.

— Igor


On Dec 7, 2019, at 1:43 PM, Phil Race  wrote:

All these tests specify this already so it doesn’t seem sufficient.

-Phil.


On Dec 7, 2019, at 12:07 PM, Igor Ignatyev  wrote:

can we just add '@modules jdk.incubator.jpackage' to 
test/jdk/tools/jpackage/junit/junit.java to solve that? or some of the tests 
run by test/jdk/tools/jpackage/junit/junit.java don't need 
jdk.incubator.jpackage module?

-- Igor


On Dec 7, 2019, at 11:57 AM, Philip Race  wrote:

Yes, since only a (relatively) small number of tests needed to be updated
this is fine with me at least for now. So +1

-phil.


On 12/7/19, 5:48 AM, Andy Herrick wrote:
Phil - are you approving this change ? - I think you are the only registered 
Reviewer.

/Andy


On 12/6/2019 8:11 PM, Phil Race wrote:
Well we could deprecate and remove the solaris port :-)
But until that is done this is the only way I know of.
we could require all jpackage tests to include some helper code which decides 
if it is applicable but that will be more work upfront and many jpackage tests 
are already platform specific so @requires is not going away.


-Phil.


On Dec 6, 2019, at 2:33 PM, Alexander Matveev  
wrote:

Looks good, but is there better way to exclude tests on Solaris? I do not like 
idea adding @requires for all tests.

Thanks,
Alexander


On 12/6/2019 10:35 AM, Alexey Semenyuk wrote:
Looks good.

- Alexey


On 12/6/2019 1:33 PM, Andy Herrick wrote:
Please review this jpackager test fix for bug [1] at [2].

the fix adds "@requires (os.family == "linux") | (os.family == "mac") | (os.family == "windows")" 
to all jpackage tests that were not already filtered with "@requires (os.family == "XXX")"

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

[2] http://cr.openjdk.java.net/~herrick/8235453/

/Andy

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

[Phil Race]

All these tests specify this already so it doesn’t seem sufficient.

-Phil.

> On Dec 7, 2019, at 12:07 PM, Igor Ignatyev  wrote:
> 
> can we just add '@modules jdk.incubator.jpackage' to 
> test/jdk/tools/jpackage/junit/junit.java to solve that? or some of the tests 
> run by test/jdk/tools/jpackage/junit/junit.java don't need 
> jdk.incubator.jpackage module?
> 
> -- Igor
> 
>> On Dec 7, 2019, at 11:57 AM, Philip Race  wrote:
>> 
>> Yes, since only a (relatively) small number of tests needed to be updated
>> this is fine with me at least for now. So +1
>> 
>> -phil.
>> 
>>> On 12/7/19, 5:48 AM, Andy Herrick wrote:
>>> Phil - are you approving this change ? - I think you are the only 
>>> registered Reviewer.
>>> 
>>> /Andy
>>> 
>>>> On 12/6/2019 8:11 PM, Phil Race wrote:
>>>> Well we could deprecate and remove the solaris port :-)
>>>> But until that is done this is the only way I know of.
>>>> we could require all jpackage tests to include some helper code which 
>>>> decides if it is applicable but that will be more work upfront and many 
>>>> jpackage tests are already platform specific so @requires is not going 
>>>> away.
>>>> 
>>>> 
>>>> -Phil.
>>>> 
>>>>> On Dec 6, 2019, at 2:33 PM, Alexander Matveev 
>>>>>  wrote:
>>>>> 
>>>>> Looks good, but is there better way to exclude tests on Solaris? I do not 
>>>>> like idea adding @requires for all tests.
>>>>> 
>>>>> Thanks,
>>>>> Alexander
>>>>> 
>>>>>> On 12/6/2019 10:35 AM, Alexey Semenyuk wrote:
>>>>>> Looks good.
>>>>>> 
>>>>>> - Alexey
>>>>>> 
>>>>>>> On 12/6/2019 1:33 PM, Andy Herrick wrote:
>>>>>>> Please review this jpackager test fix for bug [1] at [2].
>>>>>>> 
>>>>>>> the fix adds "@requires (os.family == "linux") | (os.family == "mac") | 
>>>>>>> (os.family == "windows")" to all jpackage tests that were not already 
>>>>>>> filtered with "@requires (os.family == "XXX")"
>>>>>>> 
>>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8235453
>>>>>>> 
>>>>>>> [2] http://cr.openjdk.java.net/~herrick/8235453/
>>>>>>> 
>>>>>>> /Andy
>>>>>>> 
> 

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

[Phil Race]

Well we could deprecate and remove the solaris port :-) 
But until that is done this is the only way I know of.
we could require all jpackage tests to include some helper code which decides 
if it is applicable but that will be more work upfront and many jpackage tests 
are already platform specific so @requires is not going away.


-Phil.

> On Dec 6, 2019, at 2:33 PM, Alexander Matveev  
> wrote:
> 
> Looks good, but is there better way to exclude tests on Solaris? I do not 
> like idea adding @requires for all tests.
> 
> Thanks,
> Alexander
> 
>> On 12/6/2019 10:35 AM, Alexey Semenyuk wrote:
>> Looks good.
>> 
>> - Alexey
>> 
>>> On 12/6/2019 1:33 PM, Andy Herrick wrote:
>>> Please review this jpackager test fix for bug [1] at [2].
>>> 
>>> the fix adds "@requires (os.family == "linux") | (os.family == "mac") | 
>>> (os.family == "windows")" to all jpackage tests that were not already 
>>> filtered with "@requires (os.family == "XXX")"
>>> 
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8235453
>>> 
>>> [2] http://cr.openjdk.java.net/~herrick/8235453/
>>> 
>>> /Andy
>>> 
>> 
> 

Re: RFR: JDK-8233270: Add support to jtreg helpers to unpack packages

[Phil Race]

On 12/6/19 12:51 PM, Alexey Semenyuk wrote:



On 12/5/2019 8:44 PM, Alexander Matveev wrote:

Hi Alexey,

1) Remove this file:
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JavaTool.java.rej

Done. webrev updated in place.


2) Agree with Phil, they probably should be pushed as two separate bugs.
Decoupling is not straightforward unfortunately: tests that cover 
additional launchers functionality contain fixes for both [1] and [2] 
CRs.


So the synopsis reads like this is all about updating tests, and your 
email says

"go read the bugs" and nothing more.
I think most people like to see a description of the problem and the 
proposed solution

written in the review request.

I think the source code updates are what you need to highlight.
And I don't think you have explained why it is so hard.
Saying is is hard is not the same thing as explaining why.

I think you should decouple unless you can provide that explanation.
And definitely you need to provide explanation of the fix etc.

-phil.

If there would be no strong arguments against pushing the fixes in a 
single patch, I'd rather avoid decoupling.


3) Do you know how to run installer tests with new changes? It is not 
clear from code.
Fix for [1] moved code to install packages produced by jpackage from 
manage_packages.sh script into jtreg helper classes.
If you run tests locally with test_jpackage.sh script, nothing will 
change for you.


- Alexey


Changes itself looks fine.

Thanks,
Alexander

On 12/5/2019 5:33 PM, Philip Race wrote:

I don't understand the relationship between these two bugs.

-phil

On 12/5/19, 2:47 PM, Alexey Semenyuk wrote:

Please review  fixes for [1] and [2]. Both target jpackage tool.

The webrev is at [3].

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

[2] https://bugs.openjdk.java.net/browse/JDK-8230933

[3] http://cr.openjdk.java.net/~asemenyuk/8233270/webrev.00/