Code looks fine, I did not run the tests, but +1 assuming this has been run through a mach5 test job already. If not please submit one first. -phil. On 12/18/19, 6:38 AM, Andy Herrick wrote:

Code looks good - ran tests locally and all passed.

/Andyu

On 12/17/2019 8:31 PM, Alexey Semenyuk wrote:

...

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

The fix updates jtreg tests by adding test coverage for scenarios when jpackage is used for packaging applications bundled in .jmod files.

- Alexey

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

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

looks good. /Andy On 12/19/2019 8:52 PM, Alexey Semenyuk wrote:

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

Comments added to the default WiX source file explaining how the default file is expected to be overridden.

- Alexey

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

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

fix looks fine. /Andy On 12/19/2019 8:02 PM, Alexey Semenyuk wrote:

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

The fix changes the default setting for Windows installers to allow downgrades.

- Alexey

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

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

Please review fix [2] for jpackage bug [1]. Add properties to msi installers to properly display installation location and icon of the application in the list of installed applications. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8236132 [2] http://cr.openjdk.java.net/~asemenyuk/8236132/webrev.00

Hi Alexey, Can you add description for JpIcon to overrides.wxi, otherwise looks fine. Thanks, Alexander On 12/20/2019 12:16 PM, Alexey Semenyuk wrote:

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

Add properties to msi installers to properly display installation location and icon of the application in the list of installed applications.

- Alexey

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

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

Hi Alexey, Thanks for explanation, then current patch looks fine. Thanks, Alexander On 1/14/2020 11:29 PM, Alexey Semenyuk wrote:

Alexander,

Icon can be configured on jpackage command line and should not be overridden in custom overrides.wxi file. This is the reason why I didn't put its description in the default overrides.wxi file. JpIcon is just one of WiX variables to pass configuration data from jpackage Java into WiX toolset.

- Alexey

On 1/6/2020 7:36 PM, Alexander Matveev wrote:

...

Hi Alexey,

Can you add description for JpIcon to overrides.wxi, otherwise looks fine.

Thanks, Alexander

On 12/20/2019 12:16 PM, Alexey Semenyuk wrote:

...

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

Add properties to msi installers to properly display installation location and icon of the application in the list of installed applications.

- Alexey

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

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

looks good. /Andy On 4/2/2020 3:51 PM, Alexey Semenyuk wrote:

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

Replace space characters in names of files registered by jpackage with X Desktop in postinstall step of rpm and deb installers.

- Alexey

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

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

Please review fix [2] for jpackage bug [1]. Move function checking values of mime types in file associations from Linux to shared code. Added jtreg tests to cover use cases when no or multiple mime types are specified for file associations. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8232935 [2] http://cr.openjdk.java.net/~asemenyuk/8232935/webrev.00

On 4/15/2020 2:36 PM, Andy Herrick wrote:

The change looks good, but I am confused why Japanese and Chinese MainResources property files show a lot of changes that seem to be no change. Good catch. Seems like my text editor replaced upper case letters in character codes with lower case creating this mess. I'll fix this and update the webrev.

- Alexey

On 4/15/2020 2:13 PM, Alexey Semenyuk wrote:

...

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

Move function checking values of mime types in file associations from Linux to shared code. Added jtreg tests to cover use cases when no or multiple mime types are specified for file associations.

- Alexey

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

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

ok - looks good. /Andy On 4/15/2020 4:10 PM, Alexey Semenyuk wrote:

I've updated the webrev. No more unexpected changes.

- Alexey

On 4/15/2020 3:18 PM, Alexey Semenyuk wrote:

...

On 4/15/2020 2:36 PM, Andy Herrick wrote:

...

The change looks good, but I am confused why Japanese and Chinese MainResources property files show a lot of changes that seem to be no change. Good catch. Seems like my text editor replaced upper case letters in character codes with lower case creating this mess. I'll fix this and update the webrev.

- Alexey

...

On 4/15/2020 2:13 PM, Alexey Semenyuk wrote:

...

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

Move function checking values of mime types in file associations from Linux to shared code. Added jtreg tests to cover use cases when no or multiple mime types are specified for file associations.

- Alexey

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

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

On 4/15/2020 4:21 PM, Alexander Matveev wrote:

Hi Alexey,

http://cr.openjdk.java.net/~asemenyuk/8232935/webrev.00/src/jdk.incubator.jp...

Line 58-59: I think we do not need "." at the end of error messages to make it same as other messages. Right. Thank you for the catch. webrev updated.

Do we need to call FileAssociation.verify(FileAssociation.fetchFrom(params)); for Mac?

I tried running tests with no mime and multiple mimes in file associations property file on Mac. In both cases jpackage successfully created packages. So I disabled tests added in this fix on Mac as these seem to be valid scenarios on this platform. - Alexey

Otherwise looks fine.

Thanks, Alexander

On 4/15/20 1:10 PM, Alexey Semenyuk wrote:

...

I've updated the webrev. No more unexpected changes.

- Alexey

On 4/15/2020 3:18 PM, Alexey Semenyuk wrote:

...

On 4/15/2020 2:36 PM, Andy Herrick wrote:

...

The change looks good, but I am confused why Japanese and Chinese MainResources property files show a lot of changes that seem to be no change. Good catch. Seems like my text editor replaced upper case letters in character codes with lower case creating this mess. I'll fix this and update the webrev.

- Alexey

...

On 4/15/2020 2:13 PM, Alexey Semenyuk wrote:

...

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

Move function checking values of mime types in file associations from Linux to shared code. Added jtreg tests to cover use cases when no or multiple mime types are specified for file associations.

- Alexey

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

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

Please review fix [2] for jpackage bug [1]. Refactor jpackage native code. - Improve code reuse between different platforms. - Replace custom string classes with the standard std::basic_string. - Merge functionality of libapplauncher.dll(.so) library with jpackageapplauncher(.exe) binary. There is no point to keep two different executables. - Link jpackageapplauncher.exe with MSVC Run-Time library statically to avoid copying of MSVC Run-Time dlls to app's bin folder. - Remove unused code. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8242302 [2] http://cr.openjdk.java.net/~asemenyuk/8242302/webrev.00

Hi Alexey, Looks good. Thanks, Alexander On 4/16/20 10:37 AM, Andy Herrick wrote:

OK - I approve. - I have tested on 2 platforms and it looks good.

/Andy

On 4/15/2020 4:13 PM, Alexey Semenyuk wrote:

...

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

Refactor jpackage native code.

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

- Alexey

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

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

Erik, Please review updated patch with the suggested improvements at [1]. The webrev includes only incremental changes to Lib-jdk.incubator.jpackage.gmk for clarity. [1] http://cr.openjdk.java.net/~asemenyuk/8242302/webrev.01/webrev.01/index.html - Alexey On 4/20/2020 1:17 PM, Erik Joelsson wrote:

(adding build-dev)

Hello Alexey,

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

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

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

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

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

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

/Erik

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

...

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

Refactor jpackage native code.

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

- Alexey

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

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

Thank you for the review! Whitespace issues will be fixed in the final patch. - Alexey On 4/23/2020 2:51 PM, Erik Joelsson wrote:

Thanks, that looks good!

Only two minor whitespace nits.

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

Line 101: extra space

No need for new webrev.

/Erik

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

...

Erik,

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

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

- Alexey

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

...

(adding build-dev)

Hello Alexey,

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

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

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

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

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

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

/Erik

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

...

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

Refactor jpackage native code.

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

- Alexey

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

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

Hi Alexey, Looks fine. Thanks, Alexander On 4/23/20 2:34 PM, Alexey Semenyuk wrote:

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

Implement rebranding of exe installers produced by jpackage.

- Alexey

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

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

Please review fix [2] for jpackage bug [1]. Fix vs2019 compliation error. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8244220 [2] http://cr.openjdk.java.net/~asemenyuk/8244220/webrev.00

It may be self-evident but I really like to see a RFR include an evaluation and explanation of the fix and the same in the bug report. Please do both. -phil. On 5/1/20, 1:00 PM, Alexey Semenyuk wrote:

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

Fix vs2019 compliation error.

- Alexey

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

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

OK. Approved. -phil. On 5/1/20, 4:18 PM, Alexey Semenyuk wrote:

Evaluation:

Code snippet at memory(1871): --- ~unique_ptr() noexcept { if (_Mypair._Myval2) { _Mypair._Get_first()(_Mypair._Myval2); } } ---

Where '_Myval2' is of type jni::JniObjWithEnv defined in JniUtils.h.

JniObjWithEnv is a struct wrapping two pointers: JNIEnv* and jobject. It requires user defined bool operator to make code in ~unique_ptr() compile. By some reason vs2017 didn't complain on missing bool operator.

I've added the evaluation in the comments section of the bug report as well.

- Alexey

On 5/1/2020 6:21 PM, Philip Race wrote:

...

It may be self-evident but I really like to see a RFR include an evaluation and explanation of the fix and the same in the bug report.

Please do both.

-phil.

On 5/1/20, 1:00 PM, Alexey Semenyuk wrote:

...

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

Fix vs2019 compliation error.

- Alexey

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

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

Please review fix [2] for jpackage bug [1]. Fix failure to load jli.dll from app launcher. The fix is to append path to directory with jli.dll to PATH env variable and load jli.dll with altered value of PATH if the first attempt to load jli.dll without altering PATH fails. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8244634 [2] http://cr.openjdk.java.net/~asemenyuk/8244634/webrev.00

Is the problem that by removing the copy of the microsoft dlls from the applications bin directory, then on machines that do not have all of these dll's already in the PATH (usually in C:\windows\system32) they can no longer be found ? I don't like manually manipulating the PATH env variable either, but if so I don't see what else can be done short of putting the app exe in the bin dir of the runtime. /Andy On 5/11/2020 9:37 PM, Alexander Matveev wrote:

Hi Alexey,

Updating PATH does not look like good solution to me. Did you try to load jli.dll by specifying full path to jli.dll when calling LoadLibary? If it does not work, then for AddDllDirectory() did you used LoadLibrary() or LoadLibraryEx() with LOAD_LIBRARY_SEARCH_USER_DIRS? According to doc you need to use LoadLibraryEx() with flag when using AddDllDirectory().

Thanks, Alexander

On 5/11/2020 4:36 PM, Alexey Semenyuk wrote:

...

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

Fix failure to load jli.dll from app launcher. The fix is to append path to directory with jli.dll to PATH env variable and load jli.dll with altered value of PATH if the first attempt to load jli.dll without altering PATH fails.

- Alexey

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

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

I think that sorry if things is usually handled in an “Application Manifest” on Windows https://docs.microsoft.com/en-us/windows/win32/sbscs/application-manifests Scott

On May 12, 2020, at 8:33 AM, Kevin Rushforth <kevin.rushforth@oracle.com> wrote:

Is there a way you can link the launcher (e.g., something similar to RPATH on Unix systems) to look in the right place relative to the launcher? Otherwise, the solution with adding to the PATH seems OK to me.

-- Kevin

...

On 5/12/2020 5:22 AM, Andy Herrick wrote: Is the problem that by removing the copy of the microsoft dlls from the applications bin directory, then on machines that do not have all of these dll's already in the PATH (usually in C:\windows\system32) they can no longer be found ?

I don't like manually manipulating the PATH env variable either, but if so I don't see what else can be done short of putting the app exe in the bin dir of the runtime.

/Andy

...

On 5/11/2020 9:37 PM, Alexander Matveev wrote: Hi Alexey,

Updating PATH does not look like good solution to me. Did you try to load jli.dll by specifying full path to jli.dll when calling LoadLibary? If it does not work, then for AddDllDirectory() did you used LoadLibrary() or LoadLibraryEx() with LOAD_LIBRARY_SEARCH_USER_DIRS? According to doc you need to use LoadLibraryEx() with flag when using AddDllDirectory().

Thanks, Alexander

On 5/11/2020 4:36 PM, Alexey Semenyuk wrote:

...

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

Fix failure to load jli.dll from app launcher. The fix is to append path to directory with jli.dll to PATH env variable and load jli.dll with altered value of PATH if the first attempt to load jli.dll without altering PATH fails.

- Alexey

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

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

Changed the patch to try AddDllDirectory() first and alter PATH as the last resort. Webrev at [1] Log output: --- $ env JPACKAGE_DEBUG=true ./JTwork/scratch/output/test/test.exe [TRACE] applauncher.cpp:217: Entering AppLauncher::launch [TRACE] applauncher.cpp:100: Launcher config file path: "C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\app\test.cfg" [TRACE] winlauncher.cpp:70: Entering `anonymous-namespace'::loadDllWithAddDllDirectory [TRACE] winlauncher.cpp:86: AddDllDirectory(C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin): OK [TRACE] winlauncher.cpp:94: LoadLibraryEx(C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin\jli.dll, LOAD_LIBRARY_SEARCH_DEFAULT_DIRS): 00007FFBED200000 [TRACE] winlauncher.cpp:0: Exiting `anonymous-namespace'::loadDllWithAddDllDirectory (entered at winlauncher.cpp:70) [TRACE] jvmlauncher.cpp:177: JVM library: "C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin\jli.dll" hello: Environment supports a display jpackage test application args.length: 0 -Dparam1=Some Param 1 -Dparam2=Some "Param" 2 -Dparam3=Some "Param" with " 3 hello: Output file: [appOutput.txt] [TRACE] applauncher.cpp:0: Exiting AppLauncher::launch (entered at applauncher.cpp:217) --- AddDllDirectory/LoadLibraryEx works. However not with LOAD_LIBRARY_SEARCH_USER_DIRS flag, but with wider LOAD_LIBRARY_SEARCH_DEFAULT_DIRS. Should we keep the code that would alter PATH in case AddDllDirectory() doesn't work? [1] http://cr.openjdk.java.net/~asemenyuk/8244634/webrev.01 - Alexey On 5/12/2020 10:33 AM, Alexey Semenyuk wrote:

...

Is the problem that by removing the copy of the microsoft dlls from the applications bin directory, then on machines that do not have all of these dll's already in the PATH (usually in C:\windows\system32) they can no longer be found ? Yes.

...

Is there a way you can link the launcher (e.g., something similar to RPATH on Unix systems) to look in the right place relative to the launcher? The problem with failure to load jli.dll from the app launcher is that the system fails to load dependent dlls for jli.dll (MSVC run-time libs) from the directory with jli.dll even though the full path to jli.dll is specified in LoadLibrary() call. Launcher itself is statically linked with MSVC run-time libs. It it not an issue.

...

Did you try to load jli.dll by specifying full path to jli.dll when calling LoadLibary? Yes. Launcher loads jli.dll with the full path specified. The problem why LoadLibrary() fails to load it from the full path is that depending on OS/DLL loading settings LoadLibrary() would or would NOT look for dependent dlls in the directory where jli.dll is located. The tricky part about AddDllDirectory() is that it would work for LoadLibraryEx() with LOAD_LIBRARY_SEARCH_USER_DIRS only. Is this how Windows implicitly loads MSVC run-time libs before it completes explicit request to load jli.dll from the app launcher? I don't know.

PATH env variable will be altered only in case the first attempt to load jli.dll fails. Value of PATH can be restored after jli.dll is loaded. This piece of code can be added to the patch. I'll also give another try to AddDllDirectory() with LoadLibraryEx() call.

- Alexey

On 5/12/2020 8:31 AM, Kevin Rushforth wrote:

...

Is there a way you can link the launcher (e.g., something similar to RPATH on Unix systems) to look in the right place relative to the launcher? Otherwise, the solution with adding to the PATH seems OK to me.

-- Kevin

On 5/12/2020 5:22 AM, Andy Herrick wrote:

...

Is the problem that by removing the copy of the microsoft dlls from the applications bin directory, then on machines that do not have all of these dll's already in the PATH (usually in C:\windows\system32) they can no longer be found ?

I don't like manually manipulating the PATH env variable either, but if so I don't see what else can be done short of putting the app exe in the bin dir of the runtime.

/Andy

On 5/11/2020 9:37 PM, Alexander Matveev wrote:

...

Hi Alexey,

Updating PATH does not look like good solution to me. Did you try to load jli.dll by specifying full path to jli.dll when calling LoadLibary? If it does not work, then for AddDllDirectory() did you used LoadLibrary() or LoadLibraryEx() with LOAD_LIBRARY_SEARCH_USER_DIRS? According to doc you need to use LoadLibraryEx() with flag when using AddDllDirectory().

Thanks, Alexander

On 5/11/2020 4:36 PM, Alexey Semenyuk wrote:

...

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

Fix failure to load jli.dll from app launcher. The fix is to append path to directory with jli.dll to PATH env variable and load jli.dll with altered value of PATH if the first attempt to load jli.dll without altering PATH fails.

- Alexey

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

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

OK - looks good. /Andy On 5/12/2020 11:42 AM, Alexey Semenyuk wrote:

Changed the patch to try AddDllDirectory() first and alter PATH as the last resort. Webrev at [1]

Log output: --- $ env JPACKAGE_DEBUG=true ./JTwork/scratch/output/test/test.exe [TRACE] applauncher.cpp:217: Entering AppLauncher::launch [TRACE] applauncher.cpp:100: Launcher config file path: "C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\app\test.cfg" [TRACE] winlauncher.cpp:70: Entering `anonymous-namespace'::loadDllWithAddDllDirectory [TRACE] winlauncher.cpp:86: AddDllDirectory(C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin): OK [TRACE] winlauncher.cpp:94: LoadLibraryEx(C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin\jli.dll, LOAD_LIBRARY_SEARCH_DEFAULT_DIRS): 00007FFBED200000 [TRACE] winlauncher.cpp:0: Exiting `anonymous-namespace'::loadDllWithAddDllDirectory (entered at winlauncher.cpp:70) [TRACE] jvmlauncher.cpp:177: JVM library: "C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin\jli.dll" hello: Environment supports a display jpackage test application args.length: 0 -Dparam1=Some Param 1 -Dparam2=Some "Param" 2 -Dparam3=Some "Param" with " 3 hello: Output file: [appOutput.txt] [TRACE] applauncher.cpp:0: Exiting AppLauncher::launch (entered at applauncher.cpp:217) ---

AddDllDirectory/LoadLibraryEx works. However not with LOAD_LIBRARY_SEARCH_USER_DIRS flag, but with wider LOAD_LIBRARY_SEARCH_DEFAULT_DIRS. Should we keep the code that would alter PATH in case AddDllDirectory() doesn't work?

[1] http://cr.openjdk.java.net/~asemenyuk/8244634/webrev.01

- Alexey

On 5/12/2020 10:33 AM, Alexey Semenyuk wrote:

...

...

Is the problem that by removing the copy of the microsoft dlls from the applications bin directory, then on machines that do not have all of these dll's already in the PATH (usually in C:\windows\system32) they can no longer be found ? Yes.

...

Is there a way you can link the launcher (e.g., something similar to RPATH on Unix systems) to look in the right place relative to the launcher? The problem with failure to load jli.dll from the app launcher is that the system fails to load dependent dlls for jli.dll (MSVC run-time libs) from the directory with jli.dll even though the full path to jli.dll is specified in LoadLibrary() call. Launcher itself is statically linked with MSVC run-time libs. It it not an issue.

...

Did you try to load jli.dll by specifying full path to jli.dll when calling LoadLibary? Yes. Launcher loads jli.dll with the full path specified. The problem why LoadLibrary() fails to load it from the full path is that depending on OS/DLL loading settings LoadLibrary() would or would NOT look for dependent dlls in the directory where jli.dll is located. The tricky part about AddDllDirectory() is that it would work for LoadLibraryEx() with LOAD_LIBRARY_SEARCH_USER_DIRS only. Is this how Windows implicitly loads MSVC run-time libs before it completes explicit request to load jli.dll from the app launcher? I don't know.

PATH env variable will be altered only in case the first attempt to load jli.dll fails. Value of PATH can be restored after jli.dll is loaded. This piece of code can be added to the patch. I'll also give another try to AddDllDirectory() with LoadLibraryEx() call.

- Alexey

On 5/12/2020 8:31 AM, Kevin Rushforth wrote:

...

Is there a way you can link the launcher (e.g., something similar to RPATH on Unix systems) to look in the right place relative to the launcher? Otherwise, the solution with adding to the PATH seems OK to me.

-- Kevin

On 5/12/2020 5:22 AM, Andy Herrick wrote:

...

Is the problem that by removing the copy of the microsoft dlls from the applications bin directory, then on machines that do not have all of these dll's already in the PATH (usually in C:\windows\system32) they can no longer be found ?

I don't like manually manipulating the PATH env variable either, but if so I don't see what else can be done short of putting the app exe in the bin dir of the runtime.

/Andy

On 5/11/2020 9:37 PM, Alexander Matveev wrote:

...

Hi Alexey,

Updating PATH does not look like good solution to me. Did you try to load jli.dll by specifying full path to jli.dll when calling LoadLibary? If it does not work, then for AddDllDirectory() did you used LoadLibrary() or LoadLibraryEx() with LOAD_LIBRARY_SEARCH_USER_DIRS? According to doc you need to use LoadLibraryEx() with flag when using AddDllDirectory().

Thanks, Alexander

On 5/11/2020 4:36 PM, Alexey Semenyuk wrote:

...

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

Fix failure to load jli.dll from app launcher. The fix is to append path to directory with jli.dll to PATH env variable and load jli.dll with altered value of PATH if the first attempt to load jli.dll without altering PATH fails.

- Alexey

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

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

Hi Alexey, Looks good. Thanks, Alexander On 5/26/20 12:26 PM, Alexey Semenyuk wrote:

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

Get rid of duplicated code parsing version strings. Move the code parsing version strings to dedicated classes with unit test coverage. Also remove Mac specific identifier setting in app's config file from the shared code.

- Alexey

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

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

On 5/27/2020 10:05 AM, Andy Herrick wrote:

looks good

curious why app.identifier was added to cfg file.  I don't see it used anywhere. Right. That is why I removed it. Thank you for the review!

- Alexey

/Andy

On 5/26/2020 3:26 PM, Alexey Semenyuk wrote:

...

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

Get rid of duplicated code parsing version strings. Move the code parsing version strings to dedicated classes with unit test coverage. Also remove Mac specific identifier setting in app's config file from the shared code.

- Alexey

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

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

looks good. /Andy On 6/4/2020 12:21 PM, Alexey Semenyuk wrote:

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

Replace xargs call with --no-run-if-empty parameter with bash expressions in run_tests.sh Call run_tests.sh from test_jpackager.sh in a way to avoid shebang interpretation that doesn't work properly in the environment without bash in the PATH.

Both scripts are used to run jpackage tests in local builds.

- Alexey

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

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

Please review fix [2] for jpackage bug [1]. Move functionality to collect data about app (main class name, main jar, module name, etc) from JLinkBundlerHelper and StandardBundlerParam classes in dedicated LauncherData class. Remove ArgAction, ModFile and RelativeFileSet classes as they are not needed any more after refactoring of JLinkBundlerHelper and StandardBundlerParam classes. Clean unused code. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8246624 [2] http://cr.openjdk.java.net/~asemenyuk/8246624/webrev.00

Hi Alexey, Looks good. Thanks, Alexander On 6/4/20 1:46 PM, Alexey Semenyuk wrote:

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

Move duplicated functionality from LinuxAppBundler, MacAppBundler and WinAppBundler classes in the base class.

[2] webrev is on top of [3] webrev.

- Alexey

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

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

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

Hi Alexey, Looks good. Thanks, Alexander On 6/4/20 1:22 PM, Alexey Semenyuk wrote:

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

Move functionality to collect data about app (main class name, main jar, module name, etc) from JLinkBundlerHelper and StandardBundlerParam classes in dedicated LauncherData class. Remove ArgAction, ModFile and RelativeFileSet classes as they are not needed any more after refactoring of JLinkBundlerHelper and StandardBundlerParam classes. Clean unused code.

- Alexey

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

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

Please review fix [2] for jpackage bug [1]. Add support to jpackage to create Linux packages installing app images in '/usr' tree with splicing of the app image. For --install-dir=/usr jpackag option the resulting Debian package will have the following layout: - Launchers will be installed in `/usr/bin` directory - Java runtime will be installed in `/usr/lib/$PACKAGE_NAME/runtime` directory - `app` directory of the app image will be installed in `/usr/lib/$PACKAGE_NAME/app` directory - copyright file will be installed in `/usr/share/doc/$PACKAGE_NAME/copyright` file RPM package will have the following layout: - Launchers will be installed in `/usr/bin` directory - Java runtime will be installed in `/usr/lib/$PACKAGE_NAME/runtime` directory - `app` directory of the app image will be installed in `/usr/lib/$PACKAGE_NAME/app` directory For --install-dir=/usr/local jpackag option the resulting Debian package will have the following layout: - Launchers will be installed in `/usr/local/bin` directory - Java runtime will be installed in `/usr/local/lib/$PACKAGE_NAME/runtime` directory - `app` directory of the app image will be installed in `/usr/local/lib/$PACKAGE_NAME/app` directory - copyright file will be installed in `/usr/share/doc/$PACKAGE_NAME/copyright` file RPM package will have the following layout: - Launchers will be installed in `/usr/local/bin` directory - Java runtime will be installed in `/usr/local/lib/$PACKAGE_NAME/runtime` directory - `app` directory of the app image will be installed in `/usr/local/lib/$PACKAGE_NAME/app` directory For any other prefixes starting with `/usr` no app image splicing will happen and package layout will be the same as for installing at the default `/opt` prefix. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8231283 [2] http://cr.openjdk.java.net/~asemenyuk/8231283/webrev.00

Hi Alexey, Looks good. Thanks, Alexander On 6/8/20 7:26 AM, Alexey Semenyuk wrote:

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

Add support to jpackage to create Linux packages installing app images in '/usr' tree with splicing of the app image.

For --install-dir=/usr jpackag option the resulting Debian package will have the following layout: - Launchers will be installed in `/usr/bin` directory - Java runtime will be installed in `/usr/lib/$PACKAGE_NAME/runtime` directory - `app` directory of the app image will be installed in `/usr/lib/$PACKAGE_NAME/app` directory - copyright file will be installed in `/usr/share/doc/$PACKAGE_NAME/copyright` file

RPM package will have the following layout: - Launchers will be installed in `/usr/bin` directory - Java runtime will be installed in `/usr/lib/$PACKAGE_NAME/runtime` directory - `app` directory of the app image will be installed in `/usr/lib/$PACKAGE_NAME/app` directory

For --install-dir=/usr/local jpackag option the resulting Debian package will have the following layout: - Launchers will be installed in `/usr/local/bin` directory - Java runtime will be installed in `/usr/local/lib/$PACKAGE_NAME/runtime` directory - `app` directory of the app image will be installed in `/usr/local/lib/$PACKAGE_NAME/app` directory - copyright file will be installed in `/usr/share/doc/$PACKAGE_NAME/copyright` file

RPM package will have the following layout: - Launchers will be installed in `/usr/local/bin` directory - Java runtime will be installed in `/usr/local/lib/$PACKAGE_NAME/runtime` directory - `app` directory of the app image will be installed in `/usr/local/lib/$PACKAGE_NAME/app` directory

For any other prefixes starting with `/usr` no app image splicing will happen and package layout will be the same as for installing at the default `/opt` prefix.

- Alexey

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

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

looks good. /Andy On 6/10/2020 12:21 PM, Alexey Semenyuk wrote:

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

Minor jpackage jtreg tests clean up of issues uncovered during the recent local test runs.

- Alexey

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

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

Please review fix [2] for jpackage bug [1]. Restore handling not only Java runtime home but also Java runtime root directory in `--runtime-image` option on Mac. The patch also fixes [3] issue as a side effect of moving functionality of BasicTest.testJLinkRuntime() function into the new CookedRuntimeTest.test(). - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8247422 [2] http://cr.openjdk.java.net/~asemenyuk/8247422/webrev.00 [3] https://bugs.openjdk.java.net/browse/JDK-8247424

Looks good. Thanks, Alexander On 6/11/20 1:19 PM, Andy Herrick wrote:

looks good.

/Andy

On 6/11/2020 1:23 PM, Alexey Semenyuk wrote:

...

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

Restore handling not only Java runtime home but also Java runtime root directory in `--runtime-image` option on Mac. The patch also fixes [3] issue as a side effect of moving functionality of BasicTest.testJLinkRuntime() function into the new CookedRuntimeTest.test().

- Alexey

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

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

[3] https://bugs.openjdk.java.net/browse/JDK-8247424

Hi Alexey, Looks good. I think you got links and bug ID incorrect. It should be JDK-8246244 and you have 8264244. Links also does not work. Working links are: https://bugs.openjdk.java.net/browse/JDK-8246244 http://cr.openjdk.java.net/~asemenyuk/8246244/webrev.00/ Thanks, Alexander On 6/16/2020 2:56 PM, Alexey Semenyuk wrote:

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

The fix is to put value of `Exec` property of .desktop files generated by jpackage in double quotes if the value of the propery contains whitespace characters.

- Alexey

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

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

Yeh, you are right. It was copy/paste error. Thank you for the correction! - Alexey On 6/16/2020 6:13 PM, Alexander Matveev wrote:

Hi Alexey,

Looks good. I think you got links and bug ID incorrect. It should be JDK-8246244 and you have 8264244. Links also does not work. Working links are: https://bugs.openjdk.java.net/browse/JDK-8246244 http://cr.openjdk.java.net/~asemenyuk/8246244/webrev.00/

Thanks, Alexander

On 6/16/2020 2:56 PM, Alexey Semenyuk wrote:

...

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

The fix is to put value of `Exec` property of .desktop files generated by jpackage in double quotes if the value of the propery contains whitespace characters.

- Alexey

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

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

Hi Alexey, Looks good. Thanks, Alexander On 6/23/20 10:53 AM, Alexey Semenyuk wrote:

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

Fix how icon is configured for installers on Windows. The value of ARPPRODUCTICON property should point to an entry in Icon table of msi rather to a path of icon file in install directory. The issue shows up only in downgrade scenarios.

- Alexey

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

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

Please review fix [2] for jpackage bug [1]. Put `RemoveExistingProducts` action before `CostInitialize` action in `InstallExecuteSequence` sequence to uninstall existing product(s) before installer makes changes to the file system. Existing Wix `MajorUpgrade` elements in main.wxs implicitely add `RemoveExistingProducts` action to `InstallExecuteSequence` sequence but not in the required order. Thus all `MajorUpgrade` elements in main.wxs were replaced with explicitly defined upgrade table. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8248264 [2] http://cr.openjdk.java.net/~asemenyuk/8248264/webrev.00

Please review fix [2] for jpackage bug [1]. Makes value of `JpIcon` wix variable absolute path. This fixes a regression introduced by the fix of [3] issue. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8248427 [2] http://cr.openjdk.java.net/~asemenyuk/8248427/webrev.00 [3] https://bugs.openjdk.java.net/browse/JDK-8246212

Please review fix [2] for jpackage bug [1]. Fix jpackage code to be able to locate app module if it is linked in external runtime. The suggested fix only verifies if app module exists in external runtime. It doesn't extract version and main class from modules in exetrnal runtime. To address this shortcoming [3] CR was filed. Corresponding jtreg created and added to problem list. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8248254 [2] http://cr.openjdk.java.net/~asemenyuk/8248254/webrev.00 [3] https://bugs.openjdk.java.net/browse/JDK-8248418

Looks good. /Andy On 6/26/2020 7:30 PM, Alexey Semenyuk wrote:

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

Fix jpackage code to be able to locate app module if it is linked in external runtime. The suggested fix only verifies if app module exists in external runtime. It doesn't extract version and main class from modules in exetrnal runtime. To address this shortcoming [3] CR was filed. Corresponding jtreg created and added to problem list.

- Alexey

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

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

[3] https://bugs.openjdk.java.net/browse/JDK-8248418

Hi Alexey, Looks good. Thanks, Alexander On 6/26/20 9:48 AM, Alexey Semenyuk wrote:

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

Put `RemoveExistingProducts` action before `CostInitialize` action in `InstallExecuteSequence` sequence to uninstall existing product(s) before installer makes changes to the file system. Existing Wix `MajorUpgrade` elements in main.wxs implicitely add `RemoveExistingProducts` action to `InstallExecuteSequence` sequence but not in the required order. Thus all `MajorUpgrade` elements in main.wxs were replaced with explicitly defined upgrade table.

- Alexey

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

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

Hi Alexey, Looks good. Only suggestion is to move CmdlineExecutor to Executor class and make it configurable for error message and probably attempts and timeout in case if we need such functionality in other places. Thanks, Alexander On 7/8/20 6:37 PM, Alexey Semenyuk wrote:

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

Sometimes jpackage fails building .deb package due to failure of `fakeroot` command with the same error message printed in stderr: `semop(1): encountered an error: Invalid argument`. The suggested fix is to rerun `fakeroot` command if it fails and the known error message is pritnted to stederr.

- Alexey

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

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

looks good. /Andy On 7/9/2020 3:49 PM, Alexey Semenyuk wrote:

Hi Alexander,

Agreed with your suggestion. Updated review available at [1].

- Alexey

[1] http://cr.openjdk.java.net/~asemenyuk/8247229/webrev.01/webrev.01/

On 7/9/2020 12:21 AM, alexander.matveev@oracle.com wrote:

...

Hi Alexey,

Looks good. Only suggestion is to move CmdlineExecutor to Executor class and make it configurable for error message and probably attempts and timeout in case if we need such functionality in other places.

Thanks, Alexander

On 7/8/20 6:37 PM, Alexey Semenyuk wrote:

...

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

Sometimes jpackage fails building .deb package due to failure of `fakeroot` command with the same error message printed in stderr: `semop(1): encountered an error: Invalid argument`. The suggested fix is to rerun `fakeroot` command if it fails and the known error message is pritnted to stederr.

- Alexey

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

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

Please review fix [2] for jpackage bug [1]. The fix adds ability to jpackage to hook up custom WiX localization files from resource directory. This allows to create .msi installers with custom locales, not just with three locales supported by OpenJDK. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8232621 [2] http://cr.openjdk.java.net/~asemenyuk/8232621/webrev.00

Hi Alexey, Looks good. Thanks, Alexander On 8/10/20 9:09 AM, Alexey Semenyuk wrote:

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

The fix adds ability to jpackage to hook up custom WiX localization files from resource directory. This allows to create .msi installers with custom locales, not just with three locales supported by OpenJDK.

- Alexey

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

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

looks good. /Andy On 6/10/2020 12:41 PM, Alexey Semenyuk wrote:

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

On Windows encode arguments for JNI_Launch() call with an encoding of the current process ANSI code page instead of utf8.

- Alexey

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

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

Please review fix [2] for jpackage bug [1]. Move `.jpackage.xml` file in `app` directory of app image and keep it in all OSX packages (dmg and pkg). Resotore copying of `libjli.dylib` into `$RUNTIME/Contents/MacOS` directory in MacAppImageBuilder.java. Update jtreg test helpers accordingly. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8246792 [2] http://cr.openjdk.java.net/~asemenyuk/8246792/webrev.00

Hi Alexey, Looks good. Thanks, Alexander On 6/10/20 2:40 PM, Andy Herrick wrote:

looks good -

This works for me finally.

/Andy

On 6/10/2020 5:29 PM, Alexey Semenyuk wrote:

...

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

Move `.jpackage.xml` file in `app` directory of app image and keep it in all OSX packages (dmg and pkg). Resotore copying of `libjli.dylib` into `$RUNTIME/Contents/MacOS` directory in MacAppImageBuilder.java. Update jtreg test helpers accordingly.

- Alexey

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

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

looks good. /Andy On 6/10/2020 1:04 PM, Alexey Semenyuk wrote:

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

The fix adds verification there are `xdg-icon-resource` commands in Linux packages for file associations with icons.

- Alexey

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

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