Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 12/2/2019 2:07 PM, Phil Race wrote: This makes it very difficult to do more than a cursory re-review. There is one thing I just spotted. I believe these two scripts http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/test/jdk/tools/jpackage/test_jpackage.sh.html

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

This makes it very difficult to do more than a cursory re-review. There is one thing I just spotted. I believe these two scripts http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/test/jdk/tools/jpackage/test_jpackage.sh.html

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

yes - The attempted incremental patch is not much use.  The source files were moved several times, and despite using "hg addremove -s 60", many of the files show as a remove and a new file. /Andy On 11/26/2019 9:01 PM, Philip Race wrote: > I believe otherwise it is an accurate incremental

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

> I believe otherwise it is an accurate incremental webrev of the jpackage changes since EA-5. It is also not very incremental. * *src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/main/Main.java is definitely not "new" ... -phil. On 11/26/19, 2:17 PM, Andy Herrick wrote: yes,

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

yes,  this incremental webrev contains the JNLPConverter code, which it should not. I believe otherwise it is an accurate incremental webrev of the jpackage changes since EA-5. /Andy On 11/26/2019 4:53 PM, Phil Race wrote: Andy, I am puzzled by what I see here > The webrev at [3] shows

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Andy, I am puzzled by what I see here > The webrev at [3] shows the changes since EA-06 (Build 13-jpackage+1-49 ) up to the current point. > [3] http://cr.openjdk.java.net/~herrick/8212780/webrev.06-10/ This includes the JNLPConverter which isn't what I expected to see and (correctly) isn't

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

This all looks good. +1 -- Kevin On 11/26/2019 12:56 PM, Erik Joelsson wrote: (adding build-dev) Build changes look good. /Erik On 2019-11-20 09:37, Andy Herrick wrote: I posted new composite webrev [7], and git patch [8] after pushing fix for JDK-8234402 [6]. [7]

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

(adding build-dev) Build changes look good. /Erik On 2019-11-20 09:37, Andy Herrick wrote: I posted new composite webrev [7], and git patch [8] after pushing fix for JDK-8234402 [6]. [7] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/ [8]

Fwd: Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Forwarding review thread to build-dev. /Andy Forwarded Message Subject:Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation Date: Wed, 20 Nov 2019 12:37:20 -0500 From: Andy Herrick Organization: Oracle Corporation To: Kevin Rushforth , core-libs-dev

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Andy, I've created the following CRs to track the findings: https://bugs.openjdk.java.net/browse/JDK-8223325 https://bugs.openjdk.java.net/browse/JDK-8223323 - Alexey On 5/2/2019 5:08 PM, Andy Herrick wrote: Alexey: Please file Bugs for these two issues. /Andy On 5/2/2019 1:49 PM, Alexey

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Hi Scott, I agree this a good option. Though we still need to create some custom wix source code for shortcuts, so we can't get rid completely of Java code generating wix sources. - Alexey On 5/2/2019 8:54 PM, Scott Palmer wrote: Ideally the wix code should be generated by running the

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Ideally the wix code should be generated by running the heat.exe tool on the application image. Scott > On May 2, 2019, at 5:08 PM, Andy Herrick wrote: > > Alexey: > > Please file Bugs for these two issues. > > /Andy > > >> On 5/2/2019 1:49 PM, Alexey Semenyuk wrote: >> Some findings: >>

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Alexey: Please file Bugs for these two issues. /Andy On 5/2/2019 1:49 PM, Alexey Semenyuk wrote: Some findings: http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/raw_files/new/make/launcher/Launcher-jdk.jpackage.gmk: I think definitions of BUILD_JPACKAGE_APPLAUNCHEREXE and

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Some findings: http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/raw_files/new/make/launcher/Launcher-jdk.jpackage.gmk: I think definitions of BUILD_JPACKAGE_APPLAUNCHEREXE and BUILD_JPACKAGE_APPLAUNCHERWEXE targets should be moved to

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Hi, Some comments, a initial batch... Having support for the ToolProvider is great. However, there is no indication about whether it is valid to use it from multiple threads. The implementation is structured to be deliberately single thread use only with the invocation being via a static

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

I have filed JDK-8223189 to address these. /Andy On 4/30/2019 7:02 PM, Kevin Rushforth wrote: I have a couple nit-picky comments: 1. The change to src/jdk.jlink/share/classes/module-info.java is unrelated to jpackage and should be

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

I filed task JDK-8223187 to look into (1) and CR JDK-8223188 to address (2). /Andy On 4/30/2019 6:53 PM, Phil Race wrote: A couple of questions / observations :- 1) setlocale

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

I have a couple nit-picky comments: 1. The change to src/jdk.jlink/share/classes/module-info.java is unrelated to jpackage and should be reverted (there is only a white-space change and a copyright date change to that file) 2. The following files have whitespace errors that will cause

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

A couple of questions / observations :- 1) setlocale http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/linux/native/jpackageapplauncher/launcher.cpp.html 52 int main(int argc, char *argv[]) { 53 int result = 1; 54 setlocale(LC_ALL, "en_US.utf8"); Why is this

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 4/29/2019 12:21 PM, Erik Joelsson wrote: There is a new set of macros that should be used to check things like target OS. The new macro is called like this: ifeq ($(call isTargetOs, windows macosx linux), false) Lib-jdk.jpackage.gmk and Launcher-jdk.jpackage.gmk: Same thing with

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Hello, We did review build changes as they were done in the sandbox, but IMO this change needs a fresh review now since some things have changed in the build since those reviews took place. CompileJavaModules.gmk: The -parameters flag to javac is currently only used for jdk.aot and

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Adding build-dev for the build changes. I don't know if these were previously reviewed there, but I am not sure what the changes in NativeCompilation.gmk have to do with jpackage. -phil. On 4/24/19, 5:47 PM, Andy Herrick wrote: On 4/24/2019 8:44 PM, Andy Herrick wrote: Please review

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation (update 2)

On 2019-01-17 16:06, Andy Herrick wrote: If I remove the line -nologo from lib-jdk.jpackage.gmk: 69 LDFLAGS_windows := -nologo, \ I get the logo during build (same with console andnon-console version): Microsoft (R) Incremental Linker Version 14.12.25835.0 Copyright (C) Microsoft

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation (update 2)

Hi Andy, This is looking really sweet from a build perspective! Just a few minor nits: * In Launcher-jdk.jpackage.gmk, please indent the else clause ("$(eval $(call SetupBuildLauncher, jpackage ") two spaces. * In Lib-jdk.jpackage.gmk, I think there's still room to prune some more

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 11/13/18, 12:52 PM, Roger Riggs wrote: Hi, There are enough files unique to each platform to put them in separate packages otherwise you get too many (IMHO) files in a single package/directory and its harder to tell which go with which. There isn't much of a problem with classes

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Hi, There are enough files unique to each platform to put them in separate packages otherwise you get too many (IMHO) files in a single package/directory and its harder to tell which go with which.  There isn't much of a problem with classes being public because they are all in a module and

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

I agree with this and would take it further. 1 file is in ./share/classes/jdk/jpackager/internal/builders - why not just ./share/classes/jdk/jpackager/internal 2 files are in ./share/classes/jdk/jpackager/internal/bundlers - why not just in ./share/classes/jdk/jpackager/internal 1 file is

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Question .. why is "mac", "linux" and "windows" necessary in the package name here ?  src/jdk.jpackager/macosx/classes/jdk/jpackager/internal/mac/MacAppBundler.java   src/jdk.jpackager/windows/classes/jdk/jpackager/internal/builders/windows/WindowsAppImageBuilder.java

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 11/13/2018 3:39 AM, Alan Bateman wrote: On 12/11/2018 21:40, Philip Race wrote:   74   75 static String getTmpDir() {   76 String os = System.getProperty("os.name").toLowerCase();   77 if (os.contains("win")) {   78 return System.getProperty("user.home")   79 

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Yes - The intent of getTmpDir() here is to match the GetTempDirectory() in launcher, which this does for the three supported platforms, but there is no need to check for the unsupported platforms. I will clean this up as you suggest as part ofJDK-8213756

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 12/11/2018 21:40, Philip Race wrote:   74   75 static String getTmpDir() {   76 String os = System.getProperty("os.name").toLowerCase();   77 if (os.contains("win")) {   78 return System.getProperty("user.home")   79 +

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

BTW there were also a few minor grammar issues in javadoc eg 41 * the option named "-singleton" must be specified on jpackager command line. "the jpackager" 84 * Registers {@code SingleInstanceListener} for current process. and 96 * Registers {@code SingleInstanceListener}

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 11/12/2018 4:54 PM, Philip Race wrote: On 11/12/18, 1:45 PM, Andy Herrick wrote: On 11/12/2018 3:22 PM, Philip Race wrote: Adding build-dev back .. I noticed that module jdk.jpackager.runtime requires java.desktop on all platforms So far as I can tell this is for a Mac-only support

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 11/12/18, 1:45 PM, Andy Herrick wrote: On 11/12/2018 3:22 PM, Philip Race wrote: Adding build-dev back .. I noticed that module jdk.jpackager.runtime requires java.desktop on all platforms So far as I can tell this is for a Mac-only support for receiving and handling file open events.

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 11/12/2018 3:22 PM, Philip Race wrote: Adding build-dev back .. I noticed that module jdk.jpackager.runtime requires java.desktop on all platforms So far as I can tell this is for a Mac-only support for receiving and handling file open events. Probably it only makes sense or gets used

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

74 75 static String getTmpDir() { 76 String os = System.getProperty("os.name").toLowerCase(); 77 if (os.contains("win")) { 78 return System.getProperty("user.home") 79 + "\\AppData\\LocalLow\\Sun\\Java\\JPackager\\tmp"; 80

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

SingleInstanceService. registerSingleInstance() says If the {@code SingleInstanceListener} object is already registered, or 98 * {@code slistener} is {@code null}, then the registration is skipped. I don't see how that can be working as every call to registerSingleInstanceForId creates

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Adding build-dev back .. I noticed that module jdk.jpackager.runtime requires java.desktop on all platforms So far as I can tell this is for a Mac-only support for receiving and handling file open events. Probably it only makes sense or gets used when the API is used from a running desktop

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

yes of the nine build system issues brought up by Magnus and Erik (listed in JDK-8212936 ) , only one has been addressed so far. This is at the top of the list of remaining issues we will be working ont: The following additional issues are targeted to be address in the next few weeks:

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 10/11/2018 21:59, Magnus Ihse Bursie wrote: I can't see that you've addressed any of the build system changes Erik and I requested? Or is this just a preliminary review, and you intend to go at least one more round before attempting to push? If so, this was not very clear to me. I see

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

I can't see that you've addressed any of the build system changes Erik and I requested? Or is this just a preliminary review, and you intend to go at least one more round before attempting to push? If so, this was not very clear to me. /Magnus > 9 nov. 2018 kl. 23:30 skrev Andy Herrick : > >

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 11/9/2018 5:25 PM, Andy Herrick wrote: This is an update to the Request For Review of the implementation of the Java Packager Tool (jpackager) as described in JEP 343: Packaging Tool This refresh renames the packages used to

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 2018-10-24 19:18, Erik Joelsson wrote: Hello, Nice to see this finally happening! Are we actually adding a new demo? I thought we were working towards getting rid of the demos completely. CompileJavaModules.gmk: The jdk.packager_CLEAN_FILES could be replaced with a simple

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 10/30/2018 10:11 AM, Andy Herrick wrote: On 10/24/2018 10:22 AM, Alan Bateman wrote: On 23/10/2018 16:49, Andy Herrick wrote: This patch implements the Java Packager Tool (jpackager) as described in JEP 343: Packaging Tool jpackager

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

inline On 10/30/2018 9:09 AM, Alan Bateman wrote: On 30/10/2018 14:11, Andy Herrick wrote: : If I read the webrev correctly then it adds two modules, one with the jpackager tool and the other with an API. It would be useful to get a bit more information on the split. Also I think the name

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 30/10/2018 14:11, Andy Herrick wrote: : What is the status of the JNLPConverter tool? I see it is included as a "demo" but maybe it would be better to host somewhere else as this is for developers migrating Java Web Start applications. Our current plan is to deliver it only as a demo.

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 10/24/2018 10:22 AM, Alan Bateman wrote: On 23/10/2018 16:49, Andy Herrick wrote: This patch implements the Java Packager Tool (jpackager) as described in JEP 343: Packaging Tool jpackager is a simple packaging tool, based on the

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Andy added the a comment [1] to the JEP with the command line options. I'll format it and add it to the JEP itself soon, but until then you can see it in the comments to help you review it. The tests will come shortly (Andy can comment on the state of this). They will be a mix of automated

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

Hello, Nice to see this finally happening! Are we actually adding a new demo? I thought we were working towards getting rid of the demos completely. CompileJavaModules.gmk: The jdk.packager_CLEAN_FILES could be replaced with a simple "jdk.packager_CLEAN := .properties" unless you actually

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

On 23/10/2018 16:49, Andy Herrick wrote: This patch implements the Java Packager Tool (jpackager) as described in JEP 343: Packaging Tool jpackager is a simple packaging tool, based on the JavaFX |javapackager| tool, that:  * Supports