subject:"Re\: RFR\: JDK\-8212780\: JEP 343\: Packaging Tool Implementation"


OK, thanks.

-- Kevin

On 5/3/2019 11:31 AM, Roger Riggs wrote:

Hi Kevin,

I'm fine with the disclaimer; undefined behavior is fine.

Thanks, Roger

p.s. synchronizing the run method would prevent filing of issues when
someone does it anyway, keeping the noise to a minimum.


On 05/03/2019 02:08 PM, Kevin Rushforth wrote:

Here is the webrev to document the threading limitation:

https://bugs.openjdk.java.net/browse/JDK-8223321
http://cr.openjdk.java.net/~kcr/8223321/webrev.00/

Once reviewed, Andy can include this in the next version of the 
webrev (and we'll update the CSR).


-- Kevin


On 5/3/2019 10:23 AM, Kevin Rushforth wrote:
Thanks for your feedback. I filed two issues [1][2] for the thread 
concurrency issue. The first one needs to be solved for JDK 13, 
which is to either document the existing limitation (which is 
probably what we'll do) or serialize access by synchronizing on the 
JPackageToolProvider class (or, equivalently, making the Main::run 
methods synchronized). The second one is the improvement to allow 
concurrent execution of two instances of the tool, which can be done 
for a future version.


The rest of the comments can be added as cleanup items (either a new 
issue or to one of the existing issues).


-- Kevin

[1] https://bugs.openjdk.java.net/browse/JDK-8223321
[2] https://bugs.openjdk.java.net/browse/JDK-8223322


On 5/2/2019 8:16 AM, Roger Riggs wrote:

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 method and the logging being 
via static methods.
There will need to be a disclaimer and perhaps an exception should 
be thrown.


A future improvement:
The implementation should be methods on the instance created by the 
ToolProvider

and the logging relative to that instance/delegated where needed.
Main can then be a simple lookup of the tool provider and invoke.

jpackage.main.Main:

 - Main.run returns -1, which then is used as exit status, -1 is 
not the usual exit status for a C/Unix main.


65,  the run() method is usually not static and should be re-named 
to avoid confusion


92: Implies something should be logged on a failure.

65: Run (Pw, Pw, args...) doesn't use try ... finally to ensure log 
is flushed.


65: 'throws Exception' implies it can throw an exception but is 
ambiguous as to whether

it returns an error number or throws on what kinds of errors?

91: Ambiguous as to whether processArguments() throws or return false!

CommandLine:
59: Would be more flexible and powerful to use List 
consistently...


67: use arg.startsWith() for cleaner code.

102: Seems wasteful to parse all the arguments twice.

jdk.jpackage.internal classes:

AbstractAppImageBuilder:
 57: The constructor does not need to throw IOException

60:  are .diz files common enough to preemptively exclude (w/o 
documentation)


89: Can the mix of old File API and new Path/Files APIs be avoided?

Adding javadoc to the abstract builder methods will help future 
maintainers.


203: is a bit more generous than most CLASSPATH parsing and might 
lead to non-obvious bugs.

   For example, a path component with a space in it!

229:  There is no mention of debugging support in JEP 343.
  Where is this functionality defined or is it to be identified as 
undocumented internal implementation


Regards, Roger

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

2019-05-03 Thread Phil Race


Looks good to me. We can live with this limitation for a little while, since
there aren't exactly thousands of users who will need to run this 
concurrently.
And concurrent use of the command line tool - ie using separate VMs 
works fine - per Kevin.


But we do need to get to it.

-phil.

On 5/3/19 11:08 AM, Kevin Rushforth wrote:

Here is the webrev to document the threading limitation:

https://bugs.openjdk.java.net/browse/JDK-8223321
http://cr.openjdk.java.net/~kcr/8223321/webrev.00/

Once reviewed, Andy can include this in the next version of the webrev 
(and we'll update the CSR).


-- Kevin


On 5/3/2019 10:23 AM, Kevin Rushforth wrote:
Thanks for your feedback. I filed two issues [1][2] for the thread 
concurrency issue. The first one needs to be solved for JDK 13, which 
is to either document the existing limitation (which is probably what 
we'll do) or serialize access by synchronizing on the 
JPackageToolProvider class (or, equivalently, making the Main::run 
methods synchronized). The second one is the improvement to allow 
concurrent execution of two instances of the tool, which can be done 
for a future version.


The rest of the comments can be added as cleanup items (either a new 
issue or to one of the existing issues).


-- Kevin

[1] https://bugs.openjdk.java.net/browse/JDK-8223321
[2] https://bugs.openjdk.java.net/browse/JDK-8223322


On 5/2/2019 8:16 AM, Roger Riggs wrote:

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 method and the logging being 
via static methods.
There will need to be a disclaimer and perhaps an exception should 
be thrown.


A future improvement:
The implementation should be methods on the instance created by the 
ToolProvider

and the logging relative to that instance/delegated where needed.
Main can then be a simple lookup of the tool provider and invoke.

jpackage.main.Main:

 - Main.run returns -1, which then is used as exit status, -1 is not 
the usual exit status for a C/Unix main.


65,  the run() method is usually not static and should be re-named 
to avoid confusion


92: Implies something should be logged on a failure.

65: Run (Pw, Pw, args...) doesn't use try ... finally to ensure log 
is flushed.


65: 'throws Exception' implies it can throw an exception but is 
ambiguous as to whether

it returns an error number or throws on what kinds of errors?

91: Ambiguous as to whether processArguments() throws or return false!

CommandLine:
59: Would be more flexible and powerful to use List 
consistently...


67: use arg.startsWith() for cleaner code.

102: Seems wasteful to parse all the arguments twice.

jdk.jpackage.internal classes:

AbstractAppImageBuilder:
 57: The constructor does not need to throw IOException

60:  are .diz files common enough to preemptively exclude (w/o 
documentation)


89: Can the mix of old File API and new Path/Files APIs be avoided?

Adding javadoc to the abstract builder methods will help future 
maintainers.


203: is a bit more generous than most CLASSPATH parsing and might 
lead to non-obvious bugs.

   For example, a path component with a space in it!

229:  There is no mention of debugging support in JEP 343.
  Where is this functionality defined or is it to be identified as 
undocumented internal implementation


Regards, Roger

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

2019-05-03 Thread Roger Riggs


Hi Kevin,

I'm fine with the disclaimer; undefined behavior is fine.

Thanks, Roger

p.s. synchronizing the run method would prevent filing of issues when
someone does it anyway, keeping the noise to a minimum.


On 05/03/2019 02:08 PM, Kevin Rushforth wrote:

Here is the webrev to document the threading limitation:

https://bugs.openjdk.java.net/browse/JDK-8223321
http://cr.openjdk.java.net/~kcr/8223321/webrev.00/

Once reviewed, Andy can include this in the next version of the webrev 
(and we'll update the CSR).


-- Kevin


On 5/3/2019 10:23 AM, Kevin Rushforth wrote:
Thanks for your feedback. I filed two issues [1][2] for the thread 
concurrency issue. The first one needs to be solved for JDK 13, which 
is to either document the existing limitation (which is probably what 
we'll do) or serialize access by synchronizing on the 
JPackageToolProvider class (or, equivalently, making the Main::run 
methods synchronized). The second one is the improvement to allow 
concurrent execution of two instances of the tool, which can be done 
for a future version.


The rest of the comments can be added as cleanup items (either a new 
issue or to one of the existing issues).


-- Kevin

[1] https://bugs.openjdk.java.net/browse/JDK-8223321
[2] https://bugs.openjdk.java.net/browse/JDK-8223322


On 5/2/2019 8:16 AM, Roger Riggs wrote:

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 method and the logging being 
via static methods.
There will need to be a disclaimer and perhaps an exception should 
be thrown.


A future improvement:
The implementation should be methods on the instance created by the 
ToolProvider

and the logging relative to that instance/delegated where needed.
Main can then be a simple lookup of the tool provider and invoke.

jpackage.main.Main:

 - Main.run returns -1, which then is used as exit status, -1 is not 
the usual exit status for a C/Unix main.


65,  the run() method is usually not static and should be re-named 
to avoid confusion


92: Implies something should be logged on a failure.

65: Run (Pw, Pw, args...) doesn't use try ... finally to ensure log 
is flushed.


65: 'throws Exception' implies it can throw an exception but is 
ambiguous as to whether

it returns an error number or throws on what kinds of errors?

91: Ambiguous as to whether processArguments() throws or return false!

CommandLine:
59: Would be more flexible and powerful to use List 
consistently...


67: use arg.startsWith() for cleaner code.

102: Seems wasteful to parse all the arguments twice.

jdk.jpackage.internal classes:

AbstractAppImageBuilder:
 57: The constructor does not need to throw IOException

60:  are .diz files common enough to preemptively exclude (w/o 
documentation)


89: Can the mix of old File API and new Path/Files APIs be avoided?

Adding javadoc to the abstract builder methods will help future 
maintainers.


203: is a bit more generous than most CLASSPATH parsing and might 
lead to non-obvious bugs.

   For example, a path component with a space in it!

229:  There is no mention of debugging support in JEP 343.
  Where is this functionality defined or is it to be identified as 
undocumented internal implementation


Regards, Roger

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


Here is the webrev to document the threading limitation:

https://bugs.openjdk.java.net/browse/JDK-8223321
http://cr.openjdk.java.net/~kcr/8223321/webrev.00/

Once reviewed, Andy can include this in the next version of the webrev 
(and we'll update the CSR).


-- Kevin


On 5/3/2019 10:23 AM, Kevin Rushforth wrote:
Thanks for your feedback. I filed two issues [1][2] for the thread 
concurrency issue. The first one needs to be solved for JDK 13, which 
is to either document the existing limitation (which is probably what 
we'll do) or serialize access by synchronizing on the 
JPackageToolProvider class (or, equivalently, making the Main::run 
methods synchronized). The second one is the improvement to allow 
concurrent execution of two instances of the tool, which can be done 
for a future version.


The rest of the comments can be added as cleanup items (either a new 
issue or to one of the existing issues).


-- Kevin

[1] https://bugs.openjdk.java.net/browse/JDK-8223321
[2] https://bugs.openjdk.java.net/browse/JDK-8223322


On 5/2/2019 8:16 AM, Roger Riggs wrote:

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 method and the logging being 
via static methods.
There will need to be a disclaimer and perhaps an exception should be 
thrown.


A future improvement:
The implementation should be methods on the instance created by the 
ToolProvider

and the logging relative to that instance/delegated where needed.
Main can then be a simple lookup of the tool provider and invoke.

jpackage.main.Main:

 - Main.run returns -1, which then is used as exit status, -1 is not 
the usual exit status for a C/Unix main.


65,  the run() method is usually not static and should be re-named to 
avoid confusion


92: Implies something should be logged on a failure.

65: Run (Pw, Pw, args...) doesn't use try ... finally to ensure log 
is flushed.


65: 'throws Exception' implies it can throw an exception but is 
ambiguous as to whether

it returns an error number or throws on what kinds of errors?

91: Ambiguous as to whether processArguments() throws or return false!

CommandLine:
59: Would be more flexible and powerful to use List 
consistently...


67: use arg.startsWith() for cleaner code.

102: Seems wasteful to parse all the arguments twice.

jdk.jpackage.internal classes:

AbstractAppImageBuilder:
 57: The constructor does not need to throw IOException

60:  are .diz files common enough to preemptively exclude (w/o 
documentation)


89: Can the mix of old File API and new Path/Files APIs be avoided?

Adding javadoc to the abstract builder methods will help future 
maintainers.


203: is a bit more generous than most CLASSPATH parsing and might 
lead to non-obvious bugs.

   For example, a path component with a space in it!

229:  There is no mention of debugging support in JEP 343.
  Where is this functionality defined or is it to be identified as 
undocumented internal implementation


Regards, Roger

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

2019-05-03 Thread Alexey Semenyuk

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 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
BUILD_JPACKAGE_APPLAUNCHERWEXE targets should be moved to
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/make/lib/Lib-jdk.jpackage.gmk.html.
Reason: these targets don't output executables into images/jdk/bin
directory. They produce artifacts that stored as resources in
jpackage just like other targets defined in Lib-jdk.jpackage.gmk.

---
$ less config/bundle.wxi | grep 'http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinMsiBundler.java.html:745

+ " Guid=\"" + UUID.randomUUID().toString() + "\""
Use of random GUIDs for components is not recommended and potentially
can result in issues with application updates. The recommended
approach is to generate stable GUIDs -
http://wixtoolset.org/documentation/manual/v3/howtos/general/generate_guids.html,
http://windows-installer-xml-wix-toolset.687559.n2.nabble.com/How-does-heat-maintain-consistent-GUIDs-td7599757.html.

Algorithm to create stable GUIDs is explained at
https://tools.ietf.org/html/rfc4122#page-13. However we can avoid the
hassle of generating stable GUIDs if we would put only one file in
every component. In this case WiX is able to generate stable GUIDs
for us.

- Alexey

On 4/27/2019 8:46 PM, Philip Race wrote:
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 changes for [1] which is the implementation bug for
JEP-343.

The webrev at [2] is the total cumulative webrev of changes for
the jpackage tool, currently in the JDK-8200758-branch branch of
the open sandbox repository.

The webrev at [3] shows the changes from EA-05 to EA-06.
sorry - the links are reversed from what is stated above. [2] is
the incremental webrev since EA 05, [3] is the cumulativewebrev

/Andy

The latest EA-6 (build 49) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net

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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/

[4] http://jdk.java.net/jpackage/

/Andy

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

Thanks for your feedback. I filed two issues [1][2] for the thread 
concurrency issue. The first one needs to be solved for JDK 13, which is 
to either document the existing limitation (which is probably what we'll 
do) or serialize access by synchronizing on the JPackageToolProvider 
class (or, equivalently, making the Main::run methods synchronized). The 
second one is the improvement to allow concurrent execution of two 
instances of the tool, which can be done for a future version.


The rest of the comments can be added as cleanup items (either a new 
issue or to one of the existing issues).


-- Kevin

[1] https://bugs.openjdk.java.net/browse/JDK-8223321
[2] https://bugs.openjdk.java.net/browse/JDK-8223322


On 5/2/2019 8:16 AM, Roger Riggs wrote:

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 method and the logging being 
via static methods.
There will need to be a disclaimer and perhaps an exception should be 
thrown.


A future improvement:
The implementation should be methods on the instance created by the 
ToolProvider

and the logging relative to that instance/delegated where needed.
Main can then be a simple lookup of the tool provider and invoke.

jpackage.main.Main:

 - Main.run returns -1, which then is used as exit status, -1 is not 
the usual exit status for a C/Unix main.


65,  the run() method is usually not static and should be re-named to 
avoid confusion


92: Implies something should be logged on a failure.

65: Run (Pw, Pw, args...) doesn't use try ... finally to ensure log is 
flushed.


65: 'throws Exception' implies it can throw an exception but is 
ambiguous as to whether

it returns an error number or throws on what kinds of errors?

91: Ambiguous as to whether processArguments() throws or return false!

CommandLine:
59: Would be more flexible and powerful to use List 
consistently...


67: use arg.startsWith() for cleaner code.

102: Seems wasteful to parse all the arguments twice.

jdk.jpackage.internal classes:

AbstractAppImageBuilder:
 57: The constructor does not need to throw IOException

60:  are .diz files common enough to preemptively exclude (w/o 
documentation)


89: Can the mix of old File API and new Path/Files APIs be avoided?

Adding javadoc to the abstract builder methods will help future 
maintainers.


203: is a bit more generous than most CLASSPATH parsing and might lead 
to non-obvious bugs.

   For example, a path component with a space in it!

229:  There is no mention of debugging support in JEP 343.
  Where is this functionality defined or is it to be identified as 
undocumented internal implementation


Regards, Roger

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

Thanks for your feedback. I filed two issues [1][2] for the thread 
concurrency issue. The first one needs to be solved for JDK 13, which is 
to either document the existing limitation (which is probably what we'll 
do) or serialize access by synchronizing on the JPackageToolProvider 
class (or, equivalently, making the Main::run methods synchronized). The 
second one is the improvement to allow concurrent execution of two 
instances of the tool, which can be done for a future version.


The rest of the comments can be added as cleanup items (either a new 
issue or to one of the existing issues).


-- Kevin


On 5/2/2019 8:16 AM, Roger Riggs wrote:

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 method and the logging being 
via static methods.
There will need to be a disclaimer and perhaps an exception should be 
thrown.


A future improvement:
The implementation should be methods on the instance created by the 
ToolProvider

and the logging relative to that instance/delegated where needed.
Main can then be a simple lookup of the tool provider and invoke.

jpackage.main.Main:

 - Main.run returns -1, which then is used as exit status, -1 is not 
the usual exit status for a C/Unix main.


65,  the run() method is usually not static and should be re-named to 
avoid confusion


92: Implies something should be logged on a failure.

65: Run (Pw, Pw, args...) doesn't use try ... finally to ensure log is 
flushed.


65: 'throws Exception' implies it can throw an exception but is 
ambiguous as to whether

it returns an error number or throws on what kinds of errors?

91: Ambiguous as to whether processArguments() throws or return false!

CommandLine:
59: Would be more flexible and powerful to use List 
consistently...


67: use arg.startsWith() for cleaner code.

102: Seems wasteful to parse all the arguments twice.

jdk.jpackage.internal classes:

AbstractAppImageBuilder:
 57: The constructor does not need to throw IOException

60:  are .diz files common enough to preemptively exclude (w/o 
documentation)


89: Can the mix of old File API and new Path/Files APIs be avoided?

Adding javadoc to the abstract builder methods will help future 
maintainers.


203: is a bit more generous than most CLASSPATH parsing and might lead 
to non-obvious bugs.

   For example, a path component with a space in it!

229:  There is no mention of debugging support in JEP 343.
  Where is this functionality defined or is it to be identified as 
undocumented internal implementation


Regards, Roger

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

2019-05-03 Thread Alexey Semenyuk

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 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:

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
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/make/lib/Lib-jdk.jpackage.gmk.html.
Reason: these targets don't output executables into images/jdk/bin directory.
They produce artifacts that stored as resources in jpackage just like other
targets defined in Lib-jdk.jpackage.gmk.

Wix source code produced by
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinMsiBundler.java.html
doesn't comply to recommendations of how files should be packed in component.
The recommendation is to use one file per a component -
http://wixtoolset.org/documentation/manual/v3/howtos/files_and_registry/add_a_file.html.
However jpackage produces way less components than files:
---
$ less config/bundle.wxi | grep 'http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinMsiBundler.java.html:745
+ " Guid=\"" + UUID.randomUUID().toString() + "\""
Use of random GUIDs for components is not recommended and potentially can
result in issues with application updates. The recommended approach is to
generate stable GUIDs -
http://wixtoolset.org/documentation/manual/v3/howtos/general/generate_guids.html,

http://windows-installer-xml-wix-toolset.687559.n2.nabble.com/How-does-heat-maintain-consistent-GUIDs-td7599757.html.
Algorithm to create stable GUIDs is explained at
https://tools.ietf.org/html/rfc4122#page-13. However we can avoid the hassle of
generating stable GUIDs if we would put only one file in every component. In
this case WiX is able to generate stable GUIDs for us.

- Alexey

On 4/27/2019 8:46 PM, Philip Race wrote:
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 changes for [1] which is the implementation bug for JEP-343.

The webrev at [2] is the total cumulative webrev of changes for the jpackage
tool, currently in the JDK-8200758-branch branch of the open sandbox repository.

The webrev at [3] shows the changes from EA-05 to EA-06.

sorry - the links are reversed from what is stated above. [2] is the
incremental webrev since EA 05, [3] is the cumulativewebrev
/Andy

The latest EA-6 (build 49) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net

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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/

[4] http://jdk.java.net/jpackage/

/Andy

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


Hi Alexander,

I'll file cleanup issues (or add to existing bugs) for the rest.

Thanks.

-- Kevin



On 5/2/2019 6:33 PM, Alexander Matveev wrote:

Hi Kevin,

See below.

On 5/2/2019 5:38 PM, Kevin Rushforth wrote:
Here are a few follow-on comments. As with my earlier comments, none 
of these need to be addressed prior to integration.



1. I found a few more classes that do I/O and could benefit from 
using try-with-resources:
   IOUtils, LinuxAppImageBuilder, LinuxDebBundler, LinuxRpmBundler, 
MacAppImageBuilder, etc.



JLinkBundlerHelper.java:

2. JRE_MODULES_FILENAME and SERVER_JRE_MODULES_FILENAME are unused 
(obsolete) and should be removed.



VersionExtractor.java:

3. The isLessThan method only looks at MAJOR.MINOR so might not be 
flexible enough for some applications
Currently it is being used for InnoSetup and Wixs and we only need to 
compare major.minor, so should be fine for now.



LinuxAppBundler.java:

3. Several places where non-public (package-scope) API is exported 
publicly; these should all be package-scope itself or else 
BundlerParamInfo should be public




LinuxAppImageBuilder.java:

3. createUtf8File is unused (I went looking because I was curious how 
and why we would use such a method). I see similarly-unused methods 
of the same name in the Mac and Windows AppImageBuilder classes.



MacAppImageBuilder.java:

4. Line 818: is the following still needed?

    || p.toString().contains(
"/Contents/MacOS/JavaAppletPlugin")


WindowsDefender.java + WindowsRegistry.java:

5. Is the check and warning for Windows Defender really needed? Have 
we seen problems as a result of it running while jpackage is building 
an app installer?
Yes, it is still needed. Resource update fails sometimes and I was 
able to reproduce this issue. However, not very often.


Thanks,
Alexander

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

2019-05-02 Thread Alexander Matveev


Hi Kevin,

See below.

On 5/2/2019 5:38 PM, Kevin Rushforth wrote:
Here are a few follow-on comments. As with my earlier comments, none 
of these need to be addressed prior to integration.



1. I found a few more classes that do I/O and could benefit from using 
try-with-resources:
   IOUtils, LinuxAppImageBuilder, LinuxDebBundler, LinuxRpmBundler, 
MacAppImageBuilder, etc.



JLinkBundlerHelper.java:

2. JRE_MODULES_FILENAME and SERVER_JRE_MODULES_FILENAME are unused 
(obsolete) and should be removed.



VersionExtractor.java:

3. The isLessThan method only looks at MAJOR.MINOR so might not be 
flexible enough for some applications
Currently it is being used for InnoSetup and Wixs and we only need to 
compare major.minor, so should be fine for now.



LinuxAppBundler.java:

3. Several places where non-public (package-scope) API is exported 
publicly; these should all be package-scope itself or else 
BundlerParamInfo should be public




LinuxAppImageBuilder.java:

3. createUtf8File is unused (I went looking because I was curious how 
and why we would use such a method). I see similarly-unused methods of 
the same name in the Mac and Windows AppImageBuilder classes.



MacAppImageBuilder.java:

4. Line 818: is the following still needed?

    || p.toString().contains(
"/Contents/MacOS/JavaAppletPlugin")


WindowsDefender.java + WindowsRegistry.java:

5. Is the check and warning for Windows Defender really needed? Have 
we seen problems as a result of it running while jpackage is building 
an app installer?
Yes, it is still needed. Resource update fails sometimes and I was able 
to reproduce this issue. However, not very often.


Thanks,
Alexander

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

2019-05-02 Thread Scott Palmer

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:
>> 
>> 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 
>> http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/make/lib/Lib-jdk.jpackage.gmk.html.
>>  Reason: these targets don't output executables into images/jdk/bin 
>> directory. They produce artifacts that stored as resources in jpackage just 
>> like other targets defined in Lib-jdk.jpackage.gmk.
>> 
>> Wix source code produced by 
>> http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinMsiBundler.java.html
>>  doesn't comply to recommendations of how files should be packed in 
>> component. The recommendation is to use one file per a component - 
>> http://wixtoolset.org/documentation/manual/v3/howtos/files_and_registry/add_a_file.html.
>>  However jpackage produces way less components than files:
>> ---
>> $ less config/bundle.wxi | grep '> 634
>> 
>> $ less config/bundle.wxi | grep '> 1650
>> ---
>> Data picked from my local test project.
>> 
>> http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinMsiBundler.java.html:745
>>  
>>  + " Guid=\"" + UUID.randomUUID().toString() + "\""
>> Use of random GUIDs for components is not recommended and potentially can 
>> result in issues with application updates. The recommended approach is to 
>> generate stable GUIDs - 
>> http://wixtoolset.org/documentation/manual/v3/howtos/general/generate_guids.html,
>>  
>> http://windows-installer-xml-wix-toolset.687559.n2.nabble.com/How-does-heat-maintain-consistent-GUIDs-td7599757.html.
>>  
>> Algorithm to create stable GUIDs is explained at 
>> https://tools.ietf.org/html/rfc4122#page-13. However we can avoid the hassle 
>> of generating stable GUIDs if we would put only one file in every component. 
>> In this case WiX is able to generate stable GUIDs for us.
>> 
>> - Alexey
>> 
>>> On 4/27/2019 8:46 PM, Philip Race wrote:
>>> 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  changes for [1] which is the implementation bug for 
> JEP-343.
> 
> The webrev at [2] is the total cumulative webrev of changes for the 
> jpackage tool, currently in the JDK-8200758-branch branch of the open 
> sandbox repository.
> 
> The webrev at [3] shows the changes from EA-05 to EA-06.
 sorry - the links are reversed from what is stated above. [2] is the 
 incremental webrev since EA 05, [3] is the cumulativewebrev
 /Andy
> 
> The latest EA-6 (build 49) is posted at [4].
> 
> Please send feedback to core-libs-dev@openjdk.java.net
> 
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8200758
> 
> [2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/
> 
> [3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/
> 
> [4] http://jdk.java.net/jpackage/
> 
> 
> /Andy
> 
> 
 
>> 
>

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

2019-05-02 Thread Kevin Rushforth

Here are a few follow-on comments. As with my earlier comments, none of 
these need to be addressed prior to integration.



1. I found a few more classes that do I/O and could benefit from using 
try-with-resources:
   IOUtils, LinuxAppImageBuilder, LinuxDebBundler, LinuxRpmBundler, 
MacAppImageBuilder, etc.



JLinkBundlerHelper.java:

2. JRE_MODULES_FILENAME and SERVER_JRE_MODULES_FILENAME are unused 
(obsolete) and should be removed.



VersionExtractor.java:

3. The isLessThan method only looks at MAJOR.MINOR so might not be 
flexible enough for some applications



LinuxAppBundler.java:

3. Several places where non-public (package-scope) API is exported 
publicly; these should all be package-scope itself or else 
BundlerParamInfo should be public




LinuxAppImageBuilder.java:

3. createUtf8File is unused (I went looking because I was curious how 
and why we would use such a method). I see similarly-unused methods of 
the same name in the Mac and Windows AppImageBuilder classes.



MacAppImageBuilder.java:

4. Line 818: is the following still needed?

    || p.toString().contains(
    "/Contents/MacOS/JavaAppletPlugin")


WindowsDefender.java + WindowsRegistry.java:

5. Is the check and warning for Windows Defender really needed? Have we 
seen problems as a result of it running while jpackage is building an 
app installer?


-- Kevin

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
BUILD_JPACKAGE_APPLAUNCHERWEXE targets should be moved to
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/make/lib/Lib-jdk.jpackage.gmk.html.
Reason: these targets don't output executables into images/jdk/bin
directory. They produce artifacts that stored as resources in jpackage
just like other targets defined in Lib-jdk.jpackage.gmk.

---
$ less config/bundle.wxi | grep 'http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinMsiBundler.java.html:745

- Alexey

-phil.

On 4/24/19, 5:47 PM, Andy Herrick wrote:

On 4/24/2019 8:44 PM, Andy Herrick wrote:
Please review changes for [1] which is the implementation bug for
JEP-343.

The webrev at [2] is the total cumulative webrev of changes for the
jpackage tool, currently in the JDK-8200758-branch branch of the
open sandbox repository.

The webrev at [3] shows the changes from EA-05 to EA-06.
sorry - the links are reversed from what is stated above. [2] is the
incremental webrev since EA 05, [3] is the cumulativewebrev

/Andy

The latest EA-6 (build 49) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net

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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/

[4] http://jdk.java.net/jpackage/

/Andy

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

JDK-8223264  has been 
filed to address this.


/Andy


On 5/2/2019 2:25 PM, Phil Race wrote:
Although our build system doesn't complain, my local Linux gcc 
generates several
warnings which prevent jpackage from building. The attached patch 
makes it happy.



There are several of these :-

jpackage/open/src/jdk.jpackage/share/native/libapplauncher/IniFile.cpp: 
In member function ‘virtual bool IniFile::GetSection(TString, 
OrderedMap, 
std::__cxx11::basic_string >&)’:
/home/prrace/jpackage/open/src/jdk.jpackage/share/native/libapplauncher/IniFile.cpp:192:25: 
error: ‘section’ may be used uninitialized in this function 
[-Werror=maybe-uninitialized]

 IniSectionData* section;

A couple of complains about not checking the return value of chdir. My 
patch throws an exception

which I think is better than ignoring it and using the wrong directory.
jpackage/open/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp: 
In member function ‘virtual void 
LinuxPlatform::SetCurrentDirectory(TString)’:
/home/prrace/jpackage/open/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp:129:52: 
error: ignoring return value of ‘int chdir(const char*)’, declared 
with attribute warn_unused_result [-Werror=unused-result]

 chdir(PlatformString(Value).toPlatformString());

    ^

And this :
jpackage/open/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp: 
In member function ‘virtual void PosixProcess::SetInput(TString)’:
/home/prrace/jpackage/open/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp:313:56: 
error: ignoring return value of ‘ssize_t write(int, const void*, 
size_t)’, declared with attribute warn_unused_result 
[-Werror=unused-result]

 write(FInputHandle, Value.data(), Value.size());
    ^
cc1plus: all warnings being treated as errors


-phil.

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


Semyon:

Although we removed --mac-app-store-entitlements and 
--mac-app-store-category options from help text, and disabled the 
mac-app-store target (so these two options have no effect) when we 
decided to drop support for this in the the initial release, we never 
actually removed the options, or the (now dead) code behind them.  At 
the very least we should have removed them from ValidOptions, but it 
will be cleaner to remove them entirely.


Please file a bug to address this.

/Andy




On 5/2/2019 1:39 PM, semyon.sadet...@oracle.com wrote:
The --mac-app-store-entitlements option was only used for the Mac App 
Store deployment. Since it is not supported anymore this option is not 
used anywhere and need to be removed.


Also the jdk.jpackage.internal. MacAppStoreBundler class and related 
resources are the dead code.


--Semyon

On 4/24/19 5:44 PM, Andy Herrick wrote:
Please review  changes for [1] which is the implementation bug for 
JEP-343.


The webrev at [2] is the total cumulative webrev of changes for the 
jpackage tool, currently in the JDK-8200758-branch branch of the open 
sandbox repository.


The webrev at [3] shows the changes from EA-05 to EA-06.

The latest EA-6 (build 49) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net


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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/

[4] http://jdk.java.net/jpackage/


/Andy

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

2019-05-02 Thread Phil Race

Although our build system doesn't complain, my local Linux gcc generates 
several
warnings which prevent jpackage from building. The attached patch makes 
it happy.



There are several of these :-

jpackage/open/src/jdk.jpackage/share/native/libapplauncher/IniFile.cpp: 
In member function ‘virtual bool IniFile::GetSection(TString, 
OrderedMap, 
std::__cxx11::basic_string >&)’:
/home/prrace/jpackage/open/src/jdk.jpackage/share/native/libapplauncher/IniFile.cpp:192:25: 
error: ‘section’ may be used uninitialized in this function 
[-Werror=maybe-uninitialized]

 IniSectionData* section;

A couple of complains about not checking the return value of chdir. My 
patch throws an exception

which I think is better than ignoring it and using the wrong directory.
jpackage/open/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp: 
In member function ‘virtual void 
LinuxPlatform::SetCurrentDirectory(TString)’:
/home/prrace/jpackage/open/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp:129:52: 
error: ignoring return value of ‘int chdir(const char*)’, declared with 
attribute warn_unused_result [-Werror=unused-result]

 chdir(PlatformString(Value).toPlatformString());

    ^

And this :
jpackage/open/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp: 
In member function ‘virtual void PosixProcess::SetInput(TString)’:
/home/prrace/jpackage/open/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp:313:56: 
error: ignoring return value of ‘ssize_t write(int, const void*, 
size_t)’, declared with attribute warn_unused_result [-Werror=unused-result]

 write(FInputHandle, Value.data(), Value.size());
    ^
cc1plus: all warnings being treated as errors


-phil.
diff --git a/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp b/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp
--- a/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp
+++ b/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp
@@ -126,7 +126,12 @@
 }
 
 void LinuxPlatform::SetCurrentDirectory(TString Value) {
-chdir(PlatformString(Value).toPlatformString());
+
+   if (chdir(PlatformString(Value).toPlatformString())) {
+   TString message = PlatformString::Format(
+  _T("Error: Unable to chdir to %s"), Value);
+throw Exception(message);
+   }
 }
 
 TString LinuxPlatform::GetPackageRootDirectory() {
diff --git a/src/jdk.jpackage/share/native/libapplauncher/IniFile.cpp b/src/jdk.jpackage/share/native/libapplauncher/IniFile.cpp
--- a/src/jdk.jpackage/share/native/libapplauncher/IniFile.cpp
+++ b/src/jdk.jpackage/share/native/libapplauncher/IniFile.cpp
@@ -159,7 +159,7 @@
 bool IniFile::GetValue(const TString SectionName,
 const TString Key, TString& Value) {
 bool result = false;
-IniSectionData* section;
+IniSectionData* section = NULL;
 
 if (FMap.GetValue(SectionName, section) == true && section != NULL) {
 result = section->GetValue(Key, Value);
@@ -171,7 +171,7 @@
 bool IniFile::SetValue(const TString SectionName,
 const TString Key, TString Value) {
 bool result = false;
-IniSectionData* section;
+IniSectionData* section = NULL;
 
 if (FMap.GetValue(SectionName, section) && section != NULL) {
 result = section->SetValue(Key, Value);
@@ -189,7 +189,7 @@
 bool result = false;
 
 if (FMap.ContainsKey(SectionName) == true) {
-IniSectionData* section;
+IniSectionData* section = NULL;
 
 if (FMap.GetValue(SectionName, section) == true && section != NULL) {
 OrderedMap data = section->GetData();
diff --git a/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp b/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp
--- a/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp
+++ b/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp
@@ -95,7 +95,11 @@
 }
 
 void PosixPlatform::SetCurrentDirectory(TString Value) {
-chdir(StringToFileSystemString(Value));
+if (chdir(StringToFileSystemString(Value))) {
+TString message = PlatformString::Format(
+   _T("Error: Unable to chdir to %s"), Value);
+ throw Exception(message);
+   }
 }
 
 Module PosixPlatform::LoadLibrary(TString FileName) {
@@ -310,7 +314,9 @@
 
 void PosixProcess::SetInput(TString Value) {
 if (FInputHandle != 0) {
-write(FInputHandle, Value.data(), Value.size());
+if (write(FInputHandle, Value.data(), Value.size()) == -1) {
+printf("Error writing data\n");
+} 
 }
 }

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

2019-05-02 Thread Alexey Semenyuk

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
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/make/lib/Lib-jdk.jpackage.gmk.html.
Reason: these targets don't output executables into images/jdk/bin
directory. They produce artifacts that stored as resources in jpackage
just like other targets defined in Lib-jdk.jpackage.gmk.

---
$ less config/bundle.wxi | grep 'http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinMsiBundler.java.html:745
+ " Guid=\"" + UUID.randomUUID().toString() + "\""
Use of random GUIDs for components is not recommended and potentially
can result in issues with application updates. The recommended approach
is to generate stable GUIDs -
http://wixtoolset.org/documentation/manual/v3/howtos/general/generate_guids.html,
http://windows-installer-xml-wix-toolset.687559.n2.nabble.com/How-does-heat-maintain-consistent-GUIDs-td7599757.html.
Algorithm to create stable GUIDs is explained at
https://tools.ietf.org/html/rfc4122#page-13. However we can avoid the
hassle of generating stable GUIDs if we would put only one file in every
component. In this case WiX is able to generate stable GUIDs for us.

- Alexey

-phil.

On 4/24/19, 5:47 PM, Andy Herrick wrote:

On 4/24/2019 8:44 PM, Andy Herrick wrote:
Please review changes for [1] which is the implementation bug for
JEP-343.

The webrev at [2] is the total cumulative webrev of changes for the
jpackage tool, currently in the JDK-8200758-branch branch of the
open sandbox repository.

The webrev at [3] shows the changes from EA-05 to EA-06.
sorry - the links are reversed from what is stated above. [2] is the
incremental webrev since EA 05, [3] is the cumulativewebrev

/Andy

The latest EA-6 (build 49) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net

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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/

[4] http://jdk.java.net/jpackage/

/Andy

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

2019-05-02 Thread semyon . sadetsky

The --mac-app-store-entitlements option was only used for the Mac App 
Store deployment. Since it is not supported anymore this option is not 
used anywhere and need to be removed.


Also the jdk.jpackage.internal. MacAppStoreBundler class and related 
resources are the dead code.


--Semyon

On 4/24/19 5:44 PM, Andy Herrick wrote:
Please review  changes for [1] which is the implementation bug for 
JEP-343.


The webrev at [2] is the total cumulative webrev of changes for the 
jpackage tool, currently in the JDK-8200758-branch branch of the open 
sandbox repository.


The webrev at [3] shows the changes from EA-05 to EA-06.

The latest EA-6 (build 49) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net


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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/

[4] http://jdk.java.net/jpackage/


/Andy

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

2019-05-02 Thread Roger Riggs


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 method and the logging being via 
static methods.
There will need to be a disclaimer and perhaps an exception should be 
thrown.


A future improvement:
The implementation should be methods on the instance created by the 
ToolProvider

and the logging relative to that instance/delegated where needed.
Main can then be a simple lookup of the tool provider and invoke.

jpackage.main.Main:

 - Main.run returns -1, which then is used as exit status, -1 is not 
the usual exit status for a C/Unix main.


65,  the run() method is usually not static and should be re-named to 
avoid confusion


92: Implies something should be logged on a failure.

65: Run (Pw, Pw, args...) doesn't use try ... finally to ensure log is 
flushed.


65: 'throws Exception' implies it can throw an exception but is 
ambiguous as to whether

it returns an error number or throws on what kinds of errors?

91: Ambiguous as to whether processArguments() throws or return false!

CommandLine:
59: Would be more flexible and powerful to use List consistently...

67: use arg.startsWith() for cleaner code.

102: Seems wasteful to parse all the arguments twice.

jdk.jpackage.internal classes:

AbstractAppImageBuilder:
 57: The constructor does not need to throw IOException

60:  are .diz files common enough to preemptively exclude (w/o 
documentation)


89: Can the mix of old File API and new Path/Files APIs be avoided?

Adding javadoc to the abstract builder methods will help future maintainers.

203: is a bit more generous than most CLASSPATH parsing and might lead 
to non-obvious bugs.

   For example, a path component with a space in it!

229:  There is no mention of debugging support in JEP 343.
  Where is this functionality defined or is it to be identified as 
undocumented internal implementation


Regards, Roger

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

I filed JDK-8223241  
to address each of these issues, and included import statement wildcard 
usage as suggested (removing it from JDK-8223189 
)


/Andy


On 5/1/2019 7:17 PM, Kevin Rushforth wrote:
I got most of the way through the platform-independent Java code 
today. Here is my feedback so far.


NOTE: I don't think any of these need to be addressed prior to 
integration, so you can either fix them in a follow-on webrev or file 
a bug for fixing after integration.


GENERAL:

* Several files (e.g., Arguments.java) have unused imports that can be 
removed. Maybe this can be done along with expanding wild-card imports.



src/jdk.jpackage/share/classes/module-info.java:

* I see that jdk.jpackage requires java.desktop. It looks like this is 
only needed by the Linux installers to get the size of application 
icons. If so, then perhaps this dependency could be moved to 
`linux/classes/module-info.java.extra`?



src/jdk.jpackage/share/classes/jdk/jpackage/internal/JPackageToolProvider.java: 



* Should JPackageToolProvider::run log the exception it catches? 
Although normal error handling shouldn't throw exceptions, it might be 
useful.



src/jdk.jpackage/share/classes/jdk/jpackage/main/Main.java:

* The following will prefix the version message with "jpackage version "

    private static final String version = bundle.getString("MSG_Version")
    + " " + System.getProperty("java.version");

Similar tools such as jlink and jmod don't prefix their output, but 
instead simply print the value of the "java.version" property.



src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/ResourceLocator.java: 



* It would be helpful to add a comment to highlight its usage. Also, 
for classes such as this where it is not expected that anyone should 
create an instance, a private constructor might be useful?



src/jdk.jpackage/share/classes/jdk/jpackage/internal/Arguments.java:

* The following two constants can be (default) package-scope, since it 
isn't used outside the package (and if it were, the BundlerParamInfo 
class would need to be public in order for it to be useful) :


    public static final BundlerParamInfo CREATE_APP_IMAGE =
    public static final BundlerParamInfo CREATE_INSTALLER =


* The "hasAppImage" field is set but not used (which might be OK, 
unless you meant to use it for argument validation)



src/jdk.jpackage/share/classes/jdk/jpackage/internal/AbstractAppImageBuilder.java: 



* This file could use a class comment

* What is the purpose of the following?

    excludeFileList.add(".*\\.diz");

If it really is needed, a comment would be helpful.


* The following on line 195 could use try-with-resources:

    PrintStream out = new PrintStream(cfgFileName);


src/jdk.jpackage/share/classes/jdk/jpackage/internal/AbstractBundler.java: 



* IMAGES_ROOT can be package-scope


src/jdk.jpackage/share/classes/jdk/jpackage/internal/AbstractImageBundler.java: 



* Spelling error on line 36

> or as an intermeadiate step in "create-installer" mode.

should be "intermediate"


* In extractFlagsFromVersion, other than throwing an exception, is 
there a need to pattern match older JDK versions? The minimum that 
jpackage needs to support is JDK 11, so we should be able to assume 
JEP 322 compliant version numbers. Might this allow for a simplification?



src/jdk.jpackage/share/classes/jdk/jpackage/internal/BundleParams.java:

* I think the following comment is obsolete, since the support for the 
"JavaFX-Application-Class" jar manifest entry is no longer in jpackage:


    // Note we look for both JavaFX executable jars and regular 
executable jars
    // As long as main "application" entry point is the same it is 
main class

    // (i.e. for FX jar we will use JavaFX manifest entry ...)


-- Kevin

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

2019-05-01 Thread Kevin Rushforth

I got most of the way through the platform-independent Java code today. 
Here is my feedback so far.


NOTE: I don't think any of these need to be addressed prior to 
integration, so you can either fix them in a follow-on webrev or file a 
bug for fixing after integration.


GENERAL:

* Several files (e.g., Arguments.java) have unused imports that can be 
removed. Maybe this can be done along with expanding wild-card imports.



src/jdk.jpackage/share/classes/module-info.java:

* I see that jdk.jpackage requires java.desktop. It looks like this is 
only needed by the Linux installers to get the size of application 
icons. If so, then perhaps this dependency could be moved to 
`linux/classes/module-info.java.extra`?



src/jdk.jpackage/share/classes/jdk/jpackage/internal/JPackageToolProvider.java:

* Should JPackageToolProvider::run log the exception it catches? 
Although normal error handling shouldn't throw exceptions, it might be 
useful.



src/jdk.jpackage/share/classes/jdk/jpackage/main/Main.java:

* The following will prefix the version message with "jpackage version "

    private static final String version = bundle.getString("MSG_Version")
    + " " + System.getProperty("java.version");

Similar tools such as jlink and jmod don't prefix their output, but 
instead simply print the value of the "java.version" property.



src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/ResourceLocator.java:

* It would be helpful to add a comment to highlight its usage. Also, for 
classes such as this where it is not expected that anyone should create 
an instance, a private constructor might be useful?



src/jdk.jpackage/share/classes/jdk/jpackage/internal/Arguments.java:

* The following two constants can be (default) package-scope, since it 
isn't used outside the package (and if it were, the BundlerParamInfo 
class would need to be public in order for it to be useful) :


    public static final BundlerParamInfo CREATE_APP_IMAGE =
    public static final BundlerParamInfo CREATE_INSTALLER =


* The "hasAppImage" field is set but not used (which might be OK, unless 
you meant to use it for argument validation)



src/jdk.jpackage/share/classes/jdk/jpackage/internal/AbstractAppImageBuilder.java:

* This file could use a class comment

* What is the purpose of the following?

    excludeFileList.add(".*\\.diz");

If it really is needed, a comment would be helpful.


* The following on line 195 could use try-with-resources:

    PrintStream out = new PrintStream(cfgFileName);


src/jdk.jpackage/share/classes/jdk/jpackage/internal/AbstractBundler.java:

* IMAGES_ROOT can be package-scope


src/jdk.jpackage/share/classes/jdk/jpackage/internal/AbstractImageBundler.java:

* Spelling error on line 36

> or as an intermeadiate step in "create-installer" mode.

should be "intermediate"


* In extractFlagsFromVersion, other than throwing an exception, is there 
a need to pattern match older JDK versions? The minimum that jpackage 
needs to support is JDK 11, so we should be able to assume JEP 322 
compliant version numbers. Might this allow for a simplification?



src/jdk.jpackage/share/classes/jdk/jpackage/internal/BundleParams.java:

* I think the following comment is obsolete, since the support for the 
"JavaFX-Application-Class" jar manifest entry is no longer in jpackage:


    // Note we look for both JavaFX executable jars and regular 
executable jars
    // As long as main "application" entry point is the same it is main 
class

    // (i.e. for FX jar we will use JavaFX manifest entry ...)


-- Kevin

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

2019-05-01 Thread Andy Herrick




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 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 jcheck 
to fail:
src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppImageBuilder.java:326: 
Trailing whitespace
src/jdk.jpackage/share/classes/jdk/jpackage/internal/CLIHelp.java:58: 
Tab character
src/jdk.jpackage/share/classes/jdk/jpackage/internal/JLinkBundlerHelper.java:217: 
Trailing whitespace
src/jdk.jpackage/share/classes/jdk/jpackage/internal/ValidOptions.java:55: 
Trailing whitespace



3. I recommend to replace the wild-card imports with explicit imports, 
for example:
src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxDebBundler.java 
(java.util.* and java.io.*)
(I think the wild-card static import is fine, just not the import 
every class from a package)


I'll try to remember to note these as I go through the review. This 
one could be done as a follow-up bug rather than doing it prior to 
integration if you prefer.


-- Kevin

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

2019-05-01 Thread Andy Herrick

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
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 setlocale() call there ?

What does this mean for a user whose desktop is (say) German, or French, or 
Japanese ?

When the Java app is launched from this environment is it inheriting this US 
locale ? I hope not.

We have the same on Mac :-

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/macosx/native/jpackageapplauncher/main.m.html

and windows :-

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/native/jpackageapplauncher/WinLauncher.cpp.html

   64 ::setlocale(LC_ALL, "en_US.utf8");


2) C++ files containing C

src/jdk.jpackage/windows/native/libjpackage/WindowsRegistry.cpp



src/jdk.jpackage/windows/native/libjpackage/jpackage.cpp



src/jdk.jpackage/windows/native/libwixhelper/libwixhelper.cpp



have their entire contents wrapped in

   36 #ifdef __cplusplus
   37 extern "C" {
   38 #endif

  159 #ifdef __cplusplus
  160 }
  161 #endif

wouldn't it be better to put them in .c files ?


-phil.

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

2019-04-30 Thread Alexander Matveev


Hi Andy,

Looks good overall.

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/Info-lite.plist.template.html
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/Info.plist.template.html
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/Runtime-Info.plist.template.html
All these files have:
CFBundleSignature

Based on doc "CFBundleSignature is the application’s creator signature, 
a four-character code that identifies document files belonging to this 
application."
Should it be unique per application? It seems that all bundles will have 
same signature, not sure if it is correct.


http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/launchd.plist.template.html
Looks like old file from daemon support. Probably no longer needed.

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/MacResources.properties.html
Line 28 and 37 - Double space between sentences.
Some missing "." in some messages like line 32 and 34.

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/HelpResources.properties.html
Line 47 - appRuntimeIMage -> appRuntimeImage

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/ResourceLocator.java.html
Do we need this file?

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/resources/template.iss.html
Line 49-55 - Seems to be related to service support, which is not 
currently supported. I think this file can be cleanup.


Minor issues that can be fixed as follow up cleanup task:
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxDebBundler.java.html
Line 305 - Remove if not needed.

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxRpmBundler.java.html
Line 433 - Lets remove this TODO and file a bug if needed.
Line 587 - Remove TODO.

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/linux/classes/jdk/jpackage/internal/resources/LinuxResources.properties.html
Line 27, 70 and 74 - No need for double space after ".".

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp.html
Line 864-871 - Remove if not needed.

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppStoreBundler.java.html
Line 120 - Remove if not needed.
Line 314 - Remove.
Line 364-365 - Remove.

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacPkgBundler.java.html
Line 521 and 524 - Remove

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/share/classes/jdk/jpackage/internal/StandardBundlerParam.java.html
Line 138 - Remove TODO

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/native/libapplauncher/FileAttribute.h.html
Line 36 and 39 - Remove.

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/native/libapplauncher/FilePath.cpp.html
Line 389-391 - Remove
Line 398-400 - Remove

Thanks,
Alexander


On 4/24/2019 5:47 PM, Andy Herrick wrote:


On 4/24/2019 8:44 PM, Andy Herrick wrote:
Please review  changes for [1] which is the implementation bug for 
JEP-343.


The webrev at [2] is the total cumulative webrev of changes for the 
jpackage tool, currently in the JDK-8200758-branch branch of the open 
sandbox repository.


The webrev at [3] shows the changes from EA-05 to EA-06.
sorry - the links are reversed from what is stated above. [2] is the 
incremental webrev since EA 05, [3] is the cumulativewebrev

/Andy


The latest EA-6 (build 49) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net


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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/

[4] http://jdk.java.net/jpackage/


/Andy

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

2019-04-30 Thread Kevin Rushforth


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 jcheck to 
fail:
src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppImageBuilder.java:326: 
Trailing whitespace
src/jdk.jpackage/share/classes/jdk/jpackage/internal/CLIHelp.java:58: 
Tab character
src/jdk.jpackage/share/classes/jdk/jpackage/internal/JLinkBundlerHelper.java:217: 
Trailing whitespace
src/jdk.jpackage/share/classes/jdk/jpackage/internal/ValidOptions.java:55: 
Trailing whitespace



3. I recommend to replace the wild-card imports with explicit imports, 
for example:
src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxDebBundler.java 
(java.util.* and java.io.*)
(I think the wild-card static import is fine, just not the import every 
class from a package)


I'll try to remember to note these as I go through the review. This one 
could be done as a follow-up bug rather than doing it prior to 
integration if you prefer.


-- Kevin

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

2019-04-30 Thread Phil Race


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 setlocale() call there ?

What does this mean for a user whose desktop is (say) German, or French, or 
Japanese ?

When the Java app is launched from this environment is it inheriting this US 
locale ? I hope not.

We have the same on Mac :-

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/macosx/native/jpackageapplauncher/main.m.html

and windows :-

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/native/jpackageapplauncher/WinLauncher.cpp.html

  64 ::setlocale(LC_ALL, "en_US.utf8");


2) C++ files containing C

src/jdk.jpackage/windows/native/libjpackage/WindowsRegistry.cpp



src/jdk.jpackage/windows/native/libjpackage/jpackage.cpp



src/jdk.jpackage/windows/native/libwixhelper/libwixhelper.cpp



have their entire contents wrapped in

  36 #ifdef __cplusplus
  37 extern "C" {
  38 #endif

 159 #ifdef __cplusplus
 160 }
 161 #endif

wouldn't it be better to put them in .c files ?


-phil.

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

2019-04-29 Thread Andy Herrick


jpackage reviewers:

We hope to move JEP 343 to "Proposed to Target" next week, so we would 
like expedite the review process as much as possible.


The total change is huge, so I am including the below descriptions of 
some of the central and important classes included:


jdk.jpackage.main.Main:

    Everything starts with either Main.main() (or 
JPackageToolProvider.run() which calls Main.run()), but nothing other 
than processing @filename, --help, and --version, is done in Main.  The 
real work of jpackager is delegated to Arguments.processArguments().


jdk.jpackage.internal.Arguments:

    Arguments.processArguments() processes each arg, filling the 
DeployParams object with the produced BundleArguments.  The DeployParams 
are then used to create a BundleParams object. 
Arguments.generateBundle() is then called with a Map of the BundleParams 
to actually do the work.    Argument.generateBundle() then calls the 
execute() method of each required Bundler.


The Parameters:

    A BundlerParamInfo object exists for every parameter (be it a 
StandardBundlerParam, or one specific to a given Bundler).  These each 
contain the function for extracting the id, value type, value, and 
default value for the parameter, as well as various methods to convert 
to/from Strings.  Each is defined as a static final object (again in 
either StandardBundlerParam or one of the Bundlers) so it's name can be 
used to fetch its value and type. The initial list of parameters is 
passed to the Bundler as argument to the execute() method called by 
Argument.generateBundle().


The Bundlers:

    Each platform has a collection of Bundlers, one for an Application 
Image, and one for each available installer type, all of which 
ultimately implement the Bundler interface.  For example, on windows 
there is WinAppBundler, WinMsiBundler, and WinExeBundler.


Application Image Bundlers:

    The Application Image Bundlers  extend AbstractImageBundler which 
in turn extends AbstractBundler which implements the Bundler interface.  
They  will gather  the application resources, create the cfg file, and 
may call JLinkBundlerHelper.execute() where jlink itself will be called 
via the ToolProvider interface to construct the java runtime to include 
in the application image. (If a pre defined runtime image is provided it 
will be copied in as is, instead of created in JLinkBundlerHelper).


Application Installer Bundlers:

    Each Platform dependent installer-type has an associated Bundler 
that Extends AbstractBundler (either directly or indirectly).  This 
Bundler will generally prepare some specific resource template using 
fetchResource() or preprocessTextResource (in AbstractBundler) .  This 
is where user can use --resourceDir option to override the template for 
a specific installer type with a templat of his own.  The processed 
templates become input to the program that generates the installer.  An 
exception of this is on MacOSX, when the Info.plist must be part of the 
application image, so the Info.plist.template is processed when 
generating the Application Image Bundle, rather than when processing the 
Application Installer Bundler.


Resources:

    All localizable Strings are in jdk/jpackage/internal/resources, 
either in HelpResources.properties, MainResources.properties, or the 
platform dependent Resources.properties.  The "-ja" and 
"-zh_CN" are just copies of the english files at this time, and will be 
localized after this code is integrated into the JDK.


Please feel free to post any questions to me while reviewing this code.

/Andy


On 4/24/2019 8:44 PM, Andy Herrick wrote:
Please review  changes for [1] which is the implementation bug for 
JEP-343.


The webrev at [2] is the total cumulative webrev of changes for the 
jpackage tool, currently in the JDK-8200758-branch branch of the open 
sandbox repository.


The webrev at [3] shows the changes from EA-05 to EA-06.

The latest EA-6 (build 49) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net


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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/

[4] http://jdk.java.net/jpackage/


/Andy

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

2019-04-29 Thread Andy Herrick





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 checking for target OS:

ifeq ($(call isTargetOs, windows), true)

Otherwise this looks good from a build perspective. 


I have filed JDK-8223080 
 to address this.


/Andy

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

2019-04-29 Thread Erik Joelsson


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 
jdk.internal.vm.ci, where a comment is explaining why its needed in 
those particular cases. Why is it needed for jdk.jpackage? We would like 
to avoid snowflakes if possible.


Modules.gmk:

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 checking for target OS:

ifeq ($(call isTargetOs, windows), true)

Otherwise this looks good from a build perspective.

/Erik

On 2019-04-27 17:46, Philip Race wrote:
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  changes for [1] which is the implementation bug for 
JEP-343.


The webrev at [2] is the total cumulative webrev of changes for the 
jpackage tool, currently in the JDK-8200758-branch branch of the 
open sandbox repository.


The webrev at [3] shows the changes from EA-05 to EA-06.
sorry - the links are reversed from what is stated above. [2] is the 
incremental webrev since EA 05, [3] is the cumulativewebrev

/Andy


The latest EA-6 (build 49) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net


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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/

[4] http://jdk.java.net/jpackage/


/Andy

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

2019-04-27 Thread Philip Race

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  changes for [1] which is the implementation bug for 
JEP-343.


The webrev at [2] is the total cumulative webrev of changes for the 
jpackage tool, currently in the JDK-8200758-branch branch of the open 
sandbox repository.


The webrev at [3] shows the changes from EA-05 to EA-06.
sorry - the links are reversed from what is stated above. [2] is the 
incremental webrev since EA 05, [3] is the cumulativewebrev

/Andy


The latest EA-6 (build 49) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net


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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/

[4] http://jdk.java.net/jpackage/


/Andy

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

2019-04-24 Thread Andy Herrick




On 4/24/2019 8:44 PM, Andy Herrick wrote:
Please review  changes for [1] which is the implementation bug for 
JEP-343.


The webrev at [2] is the total cumulative webrev of changes for the 
jpackage tool, currently in the JDK-8200758-branch branch of the open 
sandbox repository.


The webrev at [3] shows the changes from EA-05 to EA-06.
sorry - the links are reversed from what is stated above. [2] is the 
incremental webrev since EA 05, [3] is the cumulativewebrev

/Andy


The latest EA-6 (build 49) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net


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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.05-06/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/

[4] http://jdk.java.net/jpackage/


/Andy

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

2019-01-28 Thread Andy Herrick





On 1/27/2019 4:55 AM, Alan Bateman wrote:

On 11/01/2019 19:41, Andy Herrick wrote:
This is the second update to the Request For Review of the 
implementation of the Java Packager Tool (jpackager) as described in 
JEP 343: Packaging Tool 

I've started to play with the latest EA build, trying to figure it out 
in conjunction with the current wording in JEP 343.


I think the description/definition of "application image" needs to be 
beefed up. The current JEP doesn't reference JEP 220 and it's not 
immediately obvious how they relate. The HelloWorld example in the JEP 
shows the runtime next to an app directory but this doesn't match what 
I am seeing when I use `jpackage create-image`. Instead I see the java 
runtime is generated into PlugIns/Java.runtime. It's not clear why 
there is a "PlugIns" directory.
I filed JDK-8217902  
to investigate if we can make the layout the same on Mac as it is on 
Linux and Windows. (otherwise we need to update the JEP)


Can we do anything about simple cases where the application is a 
single executable JAR? When I use `jpackage create-image` for such 
cases then I'm forced to provide a directory to --input and  the main 
class to --class, neither of these options should be needed for simple 
cases like this, esp. when the tool also has --main-jar (which might 
need to be renamed to be consistent with other tools).
you should not need to specify --class, if the main-jar has a Main-Class 
manifest entry this is a bug filed as:JDK-8217791 



When I use `jpackage create-image` then it creates an embedded 
run-time image that is missing a lot of modules. For example, if my 
applications is indirectly using sun.misc.Unsafe, as some libraries 
do, then it will fail at run-time because the jdk.unsupported module 
is not included. The examples I tried were also missing java.sql, 
java.net.http and several more. Is this a bug or a relic from the old 
javapackager tool?

still need to investigate this.


The JEP suggests that the tool can create a native launcher but it's 
not immediately obvious how to do that. An example in the JEP would be 
useful to have. I did get it working with `--secondary-launcher` but 
that is a really strange option. I wonder if this could be renamed to 
`--launcher` and also its input changed to be closer to the equivalent 
in jlink. I also wonder if it would be saner to have the properties 
specified directly to the option rather than in another properties file.


I also tried `jpackage create-installer`. When I specify `--app-image` 
I assumed I could specify an application image that I created 
previously `jpackage create-image` but it doesn't like that. The error 
for this usage is:
Error: App image directory "myimage" is invalid and does not contain 
"app" and/or "runtime" sub-directories
this is also a bug (related to JDK-8217902 
) I will add that to 
the description.


Are you planning to keep `jpackage create-jre-installer`? I can't tell 
if this is intended to be renamed or not but I think it would be 
confusing to have "JRE" in the name of CLI options. Also it's not 
clear what "jre-name" means. I think it would be saner to simplify 
this to allow an installer be created for a given run-time image, 
never the runtime that the jpackage tool is running on.
looking into this as part of: JDK-8217894 


/Andy


That's all I have for now. I see there is a lot of feedback and issues 
being tracked so I'll try it again once the implementation is a bit 
further along.


-Alan

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

2019-01-27 Thread Alan Bateman


On 11/01/2019 19:41, Andy Herrick wrote:
This is the second update to the Request For Review of the 
implementation of the Java Packager Tool (jpackager) as described in 
JEP 343: Packaging Tool 

I've started to play with the latest EA build, trying to figure it out 
in conjunction with the current wording in JEP 343.


I think the description/definition of "application image" needs to be 
beefed up. The current JEP doesn't reference JEP 220 and it's not 
immediately obvious how they relate. The HelloWorld example in the JEP 
shows the runtime next to an app directory but this doesn't match what I 
am seeing when I use `jpackage create-image`. Instead I see the java 
runtime is generated into PlugIns/Java.runtime. It's not clear why there 
is a "PlugIns" directory.


Can we do anything about simple cases where the application is a single 
executable JAR? When I use `jpackage create-image` for such cases then 
I'm forced to provide a directory to --input and  the main class to 
--class, neither of these options should be needed for simple cases like 
this, esp. when the tool also has --main-jar (which might need to be 
renamed to be consistent with other tools).


When I use `jpackage create-image` then it creates an embedded run-time 
image that is missing a lot of modules. For example, if my applications 
is indirectly using sun.misc.Unsafe, as some libraries do, then it will 
fail at run-time because the jdk.unsupported module is not included. The 
examples I tried were also missing java.sql, java.net.http and several 
more. Is this a bug or a relic from the old javapackager tool?


The JEP suggests that the tool can create a native launcher but it's not 
immediately obvious how to do that. An example in the JEP would be 
useful to have. I did get it working with `--secondary-launcher` but 
that is a really strange option. I wonder if this could be renamed to 
`--launcher` and also its input changed to be closer to the equivalent 
in jlink. I also wonder if it would be saner to have the properties 
specified directly to the option rather than in another properties file.


I also tried `jpackage create-installer`. When I specify `--app-image` I 
assumed I could specify an application image that I created previously 
`jpackage create-image` but it doesn't like that. The error for this 
usage is:
Error: App image directory "myimage" is invalid and does not contain 
"app" and/or "runtime" sub-directories


Are you planning to keep `jpackage create-jre-installer`? I can't tell 
if this is intended to be renamed or not but I think it would be 
confusing to have "JRE" in the name of CLI options. Also it's not clear 
what "jre-name" means. I think it would be saner to simplify this to 
allow an installer be created for a given run-time image, never the 
runtime that the jpackage tool is running on.


That's all I have for now. I see there is a lot of feedback and issues 
being tracked so I'll try it again once the implementation is a bit 
further along.


-Alan

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

2019-01-18 Thread Magnus Ihse Bursie


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 Corporation.  All rights reserved.
do I need something to include CXXFLAGS_JDKEXE into LDFLAGS ? (there 
is no other LDFLAGS line...)

Ah, good catch. You should add
LDFLAGS := $(LDFLAGS_JDKEXE), \

to your setup.

/Magnus


here's the non-console APPLAUNCHERWEXE case:


# Build non-console version of launcher
ifeq ($(OPENJDK_TARGET_OS), windows)

  $(eval $(call SetupJdkExecutable, BUILD_JPACKAGE_APPLAUNCHERWEXE, \
  NAME := jpackageapplauncherw, \
  OUTPUT_DIR := 
$(JDK_OUTPUTDIR)/modules/$(MODULE)/jdk/jpackage/internal/resources, \
  SYMBOLS_DIR := 
$(SUPPORT_OUTPUTDIR)/native/$(MODULE)/jpackageapplauncherw, \

  SRC := $(JPACKAGE_APPLAUNCHEREXE_SRC), \
  TOOLCHAIN := TOOLCHAIN_LINK_CXX, \
  OPTIMIZATION := LOW, \
  CFLAGS := $(CXXFLAGS_JDKEXE), \
  CFLAGS_windows := -EHsc -DUNICODE -D_UNICODE, \
  LDFLAGS_windows := -nologo, \ 
<-

  LIBS := $(LIBCXX), \
  LIBS_windows :=  user32.lib shell32.lib advapi32.lib, \
  ))

  TARGETS += $(BUILD_JPACKAGE_APPLAUNCHERWEXE)
endif


/Andy


On 1/15/2019 8:09 AM, Magnus Ihse Bursie wrote:

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 unnecessary compiler flags and parameters to SetupJdkExecutable:

   65 CFLAGS_linux := -fPIC, \
   66 CFLAGS_solaris := -KPIC, \
   67 CFLAGS_macosx := -fPIC, \
 I wonder if these really are needed. Normally, only shared libraries 
neeed PIC code. (And for those we set it automatically.)


   69 LDFLAGS_windows := -nologo, \
This should not be needed, it's incorporated in CXXFLAGS_JDKEXE. 
(Also in the second block, for jpackageapplauncherw).


   72 LIBS_solaris :=  -lc, \
Same here; this should not be needed. It's in GLOBAL_LIBS on Solaris, 
and is always included.


   75 VERSIONINFO_RESOURCE := $(GLOBAL_VERSION_INFO_RESOURCE), \
This is setup by default by SetupJdkExecutable, so you can remove it. 
(Also in the second block, for jpackageapplauncherw).


Finally, I do believe that the setups of jpackageapplauncher and 
jpackageapplauncherw should be done in Launcher-jdk.jpackage.gmk, not 
Lib-jdk.jpackage.gmk. Since they are to be shipped as resources, they 
are not "really" launchers in our normal sense, but they are at least 
more launchers than they are libraries.


As we've talked about privately, in the future I'd like to see 
Windows too use the SetupBuildLauncher method for creating the 
launcher, but this is clearly good enough for inclusion in the mainline.


I also have a question about 
test/jdk/tools/jpackage/resources/license.txt. What is it used for? 
And why the odd (incorrect?) spelling of license?


/Magnus

On 2019-01-11 20:41, Andy Herrick wrote:
This is the second update to the Request For Review of the 
implementation of the Java Packager Tool (jpackager) as described in 
JEP 343: Packaging Tool 




This webrev corresponds to the second EA build, Build 8 (2019/1/8), 
posted at http://jdk.java.net/jpackage/


This update renames the package used to "jdk.jpackage", removes the 
Single Instance Service (and CLI option --singleton), adds several 
other CLI options, adds more automated tests, and contains fixes to 
the following issues (since update 1 on 11/09/2018):


JDK-8212164 resolve jre.list and jre.module.list
JDK-8212936 Makefile and other improvements for jpackager
JDK-8213385 jpackager Command-Line Argument File.
JDK-8213392 Enhance --help and --version
JDK-8213425 Analyze output from Source code scanner and fix 
where needed.

JDK-8213747 Makefile Improvements to Lib-jdk.packager.gmk
JDK-8213748 jpackager native launcher for macosx, linux.
JDK-8213756 SingleInstance runtime improvements
JDK-8213962 JPackageCreateImageRuntimeModuleTest fails
JDK-8213963 Flatten out jpackager packages and resources.
JDK-8214021 Create additional automated tests for jpackager
JDK-8214051 rename jpackager tool to jpackage
JDK-8214070 Analyze and Fix issues reported by Parfait
JDK-8214143 Reduce Resource files
JDK-8214495 Change behavior of --license-file
JDK-8214575 Exe installers will install application over 
existing installation
JDK-8214899 rename papplauncher and it's library and move src to 
appropriate places
JDK-8214982 jpackage causes failures in existing HelpFlagsTest 
and VersionCheck tests
JDK-8215020 create-jre-installer exe fails when --runtime-image

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

2019-01-17 Thread Andy Herrick


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 Corporation.  All rights reserved.
do I need something to include CXXFLAGS_JDKEXE into LDFLAGS ? (there is 
no other LDFLAGS line...)


here's the non-console APPLAUNCHERWEXE case:


# Build non-console version of launcher
ifeq ($(OPENJDK_TARGET_OS), windows)

  $(eval $(call SetupJdkExecutable, BUILD_JPACKAGE_APPLAUNCHERWEXE, \
  NAME := jpackageapplauncherw, \
  OUTPUT_DIR := 
$(JDK_OUTPUTDIR)/modules/$(MODULE)/jdk/jpackage/internal/resources, \
  SYMBOLS_DIR := 
$(SUPPORT_OUTPUTDIR)/native/$(MODULE)/jpackageapplauncherw, \

  SRC := $(JPACKAGE_APPLAUNCHEREXE_SRC), \
  TOOLCHAIN := TOOLCHAIN_LINK_CXX, \
  OPTIMIZATION := LOW, \
  CFLAGS := $(CXXFLAGS_JDKEXE), \
  CFLAGS_windows := -EHsc -DUNICODE -D_UNICODE, \
  LDFLAGS_windows := -nologo, \ <-
  LIBS := $(LIBCXX), \
  LIBS_windows :=  user32.lib shell32.lib advapi32.lib, \
  ))

  TARGETS += $(BUILD_JPACKAGE_APPLAUNCHERWEXE)
endif


/Andy


On 1/15/2019 8:09 AM, Magnus Ihse Bursie wrote:

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 unnecessary compiler flags and parameters to SetupJdkExecutable:

   65 CFLAGS_linux := -fPIC, \
   66 CFLAGS_solaris := -KPIC, \
   67 CFLAGS_macosx := -fPIC, \
 I wonder if these really are needed. Normally, only shared libraries 
neeed PIC code. (And for those we set it automatically.)


   69 LDFLAGS_windows := -nologo, \
This should not be needed, it's incorporated in CXXFLAGS_JDKEXE. (Also 
in the second block, for jpackageapplauncherw).


   72 LIBS_solaris :=  -lc, \
Same here; this should not be needed. It's in GLOBAL_LIBS on Solaris, 
and is always included.


   75 VERSIONINFO_RESOURCE := $(GLOBAL_VERSION_INFO_RESOURCE), \
This is setup by default by SetupJdkExecutable, so you can remove it. 
(Also in the second block, for jpackageapplauncherw).


Finally, I do believe that the setups of jpackageapplauncher and 
jpackageapplauncherw should be done in Launcher-jdk.jpackage.gmk, not 
Lib-jdk.jpackage.gmk. Since they are to be shipped as resources, they 
are not "really" launchers in our normal sense, but they are at least 
more launchers than they are libraries.


As we've talked about privately, in the future I'd like to see Windows 
too use the SetupBuildLauncher method for creating the launcher, but 
this is clearly good enough for inclusion in the mainline.


I also have a question about 
test/jdk/tools/jpackage/resources/license.txt. What is it used for? 
And why the odd (incorrect?) spelling of license?


/Magnus

On 2019-01-11 20:41, Andy Herrick wrote:
This is the second update to the Request For Review of the 
implementation of the Java Packager Tool (jpackager) as described in 
JEP 343: Packaging Tool 




This webrev corresponds to the second EA build, Build 8 (2019/1/8), 
posted at http://jdk.java.net/jpackage/


This update renames the package used to "jdk.jpackage", removes the 
Single Instance Service (and CLI option --singleton), adds several 
other CLI options, adds more automated tests, and contains fixes to 
the following issues (since update 1 on 11/09/2018):


JDK-8212164 resolve jre.list and jre.module.list
JDK-8212936 Makefile and other improvements for jpackager
JDK-8213385 jpackager Command-Line Argument File.
JDK-8213392 Enhance --help and --version
JDK-8213425 Analyze output from Source code scanner and fix where 
needed.

JDK-8213747 Makefile Improvements to Lib-jdk.packager.gmk
JDK-8213748 jpackager native launcher for macosx, linux.
JDK-8213756 SingleInstance runtime improvements
JDK-8213962 JPackageCreateImageRuntimeModuleTest fails
JDK-8213963 Flatten out jpackager packages and resources.
JDK-8214021 Create additional automated tests for jpackager
JDK-8214051 rename jpackager tool to jpackage
JDK-8214070 Analyze and Fix issues reported by Parfait
JDK-8214143 Reduce Resource files
JDK-8214495 Change behavior of --license-file
JDK-8214575 Exe installers will install application over existing 
installation
JDK-8214899 rename papplauncher and it's library and move src to 
appropriate places
JDK-8214982 jpackage causes failures in existing HelpFlagsTest 
and VersionCheck tests
JDK-8215020 create-jre-installer exe fails when --runtime-image 
is specified.
JDK-8215036 Create initial set of tests for jpackage 
create-installer mode

JDK-8215453 remove unused

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

2019-01-15 Thread Magnus Ihse Bursie


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 
unnecessary compiler flags and parameters to SetupJdkExecutable:


  65 CFLAGS_linux := -fPIC, \
  66 CFLAGS_solaris := -KPIC, \
  67 CFLAGS_macosx := -fPIC, \

 I wonder if these really are needed. Normally, only shared libraries 
neeed PIC code. (And for those we set it automatically.)


  69 LDFLAGS_windows := -nologo, \

This should not be needed, it's incorporated in CXXFLAGS_JDKEXE. (Also 
in the second block, for jpackageapplauncherw).


  72 LIBS_solaris :=  -lc, \

Same here; this should not be needed. It's in GLOBAL_LIBS on Solaris, 
and is always included.


  75 VERSIONINFO_RESOURCE := $(GLOBAL_VERSION_INFO_RESOURCE), \

This is setup by default by SetupJdkExecutable, so you can remove it. 
(Also in the second block, for jpackageapplauncherw).


Finally, I do believe that the setups of jpackageapplauncher and 
jpackageapplauncherw should be done in Launcher-jdk.jpackage.gmk, not 
Lib-jdk.jpackage.gmk. Since they are to be shipped as resources, they 
are not "really" launchers in our normal sense, but they are at least 
more launchers than they are libraries.


As we've talked about privately, in the future I'd like to see Windows 
too use the SetupBuildLauncher method for creating the launcher, but 
this is clearly good enough for inclusion in the mainline.


I also have a question about 
test/jdk/tools/jpackage/resources/license.txt. What is it used for? And 
why the odd (incorrect?) spelling of license?


/Magnus

On 2019-01-11 20:41, Andy Herrick wrote:
This is the second update to the Request For Review of the 
implementation of the Java Packager Tool (jpackager) as described in 
JEP 343: Packaging Tool 




This webrev corresponds to the second EA build, Build 8 (2019/1/8), 
posted at http://jdk.java.net/jpackage/


This update renames the package used to "jdk.jpackage", removes the 
Single Instance Service (and CLI option --singleton), adds several 
other CLI options, adds more automated tests, and contains fixes to 
the following issues (since update 1 on 11/09/2018):


JDK-8212164 resolve jre.list and jre.module.list
JDK-8212936 Makefile and other improvements for jpackager
JDK-8213385 jpackager Command-Line Argument File.
JDK-8213392 Enhance --help and --version
JDK-8213425 Analyze output from Source code scanner and fix where 
needed.

JDK-8213747 Makefile Improvements to Lib-jdk.packager.gmk
JDK-8213748 jpackager native launcher for macosx, linux.
JDK-8213756 SingleInstance runtime improvements
JDK-8213962 JPackageCreateImageRuntimeModuleTest fails
JDK-8213963 Flatten out jpackager packages and resources.
JDK-8214021 Create additional automated tests for jpackager
JDK-8214051 rename jpackager tool to jpackage
JDK-8214070 Analyze and Fix issues reported by Parfait
JDK-8214143 Reduce Resource files
JDK-8214495 Change behavior of --license-file
JDK-8214575 Exe installers will install application over existing 
installation
JDK-8214899 rename papplauncher and it's library and move src to 
appropriate places
JDK-8214982 jpackage causes failures in existing HelpFlagsTest and 
VersionCheck tests
JDK-8215020 create-jre-installer exe fails when --runtime-image is 
specified.
JDK-8215036 Create initial set of tests for jpackage 
create-installer mode

JDK-8215453 remove unused BundlerParams and fix misleading messages
JDK-8215515 Add a command line option to override internal resources.
JDK-8215900 Without --files args, only jars in the top level of 
--input are added to class-path

JDK-8215903 modify behavior of retaining temporary output dir
JDK-8216190 Remove Single Instance Service support from jpackage
JDK-8216313 Add ToolProvider information to jdk.jpackage API docs
JDK-8216373 temporary build-root left behind when using secondary 
launcher(s)

JDK-8216492 Update copyright of all new jpackage files to 2019

Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.03

Note: SingleInstanceService API was removed (see 
https://bugs.openjdk.java.net/browse/JDK-8216190).
An example stand alone implementation can be found at: 
http://cr.openjdk.java.net/~herrick/jpackage/Singleton.java


please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

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

2019-01-13 Thread Andy Herrick


yes

On 1/13/2019 2:50 PM, Alan Bateman wrote:

On 11/01/2019 19:41, Andy Herrick wrote:
This is the second update to the Request For Review of the 
implementation of the Java Packager Tool (jpackager) as described in 
JEP 343: Packaging Tool 

You're making great progress. Is the table of CLI options in the JEP 
in sync with the this update?


-Alan

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

2019-01-13 Thread Alan Bateman


On 11/01/2019 19:41, Andy Herrick wrote:
This is the second update to the Request For Review of the 
implementation of the Java Packager Tool (jpackager) as described in 
JEP 343: Packaging Tool 

You're making great progress. Is the table of CLI options in the JEP in 
sync with the this update?


-Alan

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

2018-12-11 Thread Sverre Moe

The help output of jpackager should mention how to set the classpath for
the various bundle resource files.
I have not found a working solution how to set the classpath.
  jpackager -classpath build/deploy/
  jpackager -cp build/deploy/

DEB
Using default package resource [menu icon]  (add
package/linux/application.png to the class path to customize)
Using default package resource [Menu shortcut descriptor]  (add
package/linux/application.desktop to the class path to customize)
Using default package resource [DEB control file]  (add
package/linux/control to the class path to customize)

RPM
Using default package resource [menu icon]  (add
package/linux/application.png to the class path to customize)
Using default package resource [Menu shortcut descriptor]  (add
package/linux/application.desktop to the class path to customize)
Using default package resource [RPM spec file]  (add
package/linux/application.spec to the class path to customize)

The application image can be set with the --icon argument.
--icon build/deploy/package/linux/application.png
Using custom package resource [menu icon]  (loaded from file
/home/user/workspace/application/build/deploy/package/linux/application.png)


Perhaps jpackager should have an package directory argument
--package-dir build/deploy


/Sverre


Den tor. 15. nov. 2018 kl. 15:05 skrev Andy Herrick :

>
> On 11/10/2018 8:12 AM, Sverre Moe wrote:
>
> I have been using the jpackager that Johan Vos backported for OpenJDK 11.
> For this I have some points of improvement I would like to mention.
>
> 1)
> The control file for debian package does not set correct description
>
> --name test
> --description This is a Test Application
>
> /tmp/jdk.packager607148779833718376/linux/control
> Package: test
> Description: test
>
> The RPM gets it correctly
> Summary : test
> Description :
> This is a Test Application
>
>
> 2)
> Category is not set on either DEB or RPM
>   --category
>   Category or group of the application.
> --category "Some/Category/Application"
> Group: Unspecified
> Section: unknown
>
> 3)
> The jpackager command line should have the flag --release in addition to
> --version. The only way to set a different release than "1" is to supply a
> custom spec file for RPM and control file for DEB.
> package/linux/test.spec
> Version: 1.0.0
> Release: RC1
> package/linux/control
> Version: 1.0.0-RC1
>
> I have filed issue JDK-8213941
>  and we will be looking
> into it.
>
> Thanks.
>
> /Andy
>
>
> /Sverre
>
>

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

2018-11-15 Thread Andy Herrick




On 11/10/2018 8:12 AM, Sverre Moe wrote:

I have been using the jpackager that Johan Vos backported for OpenJDK 11.
For this I have some points of improvement I would like to mention.

1)
The control file for debian package does not set correct description

--name test
--description This is a Test Application

/tmp/jdk.packager607148779833718376/linux/control
Package: test
Description: test

The RPM gets it correctly
Summary     : test
Description :
This is a Test Application


2)
Category is not set on either DEB or RPM
  --category
          Category or group of the application.
--category "Some/Category/Application"
Group: Unspecified
Section: unknown

3)
The jpackager command line should have the flag --release in addition 
to --version. The only way to set a different release than "1" is to 
supply a custom spec file for RPM and control file for DEB.

package/linux/test.spec
Version: 1.0.0
Release: RC1
package/linux/control
Version: 1.0.0-RC1

I have filed issue JDK-8213941 
 and we will be 
looking into it.


Thanks.

/Andy



/Sverre

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

2018-11-14 Thread Roger Riggs


Hi,

Does --limit-modules work as intended, are there any tests?
With a command such as:

jpackager -Djlink.debug=true create-image \
    --input out/artifacts \
    --output out \
    --limit-modules java.base \
    --main-jar Hello-jar/Hello.jar \
    --class Main

It complains that:
Exception: jdk.tools.jlink.plugin.PluginException: 
java.lang.module.FindException: Module jdk.compiler not found, required 
by jdk.jdeps


After adding a few more modules it stops complaining about missing 
dependencies:
 --limit-modules 
java.base,jdk.compiler,jdk.internal.le,jdk.internal.ed,jdk.internal.opt \



But still,

The generated runtime/bin/java --show-modules includes the full list of 
jdk modules.



In the code:

JLinkBundlerHelper.ModuleHelper ignores the limitMods argument and produces
a complete set of redistributable modules.


It should be possible to build a runtime with only the java.base module.

What should I expect?

Thanks, Roger




On 11/09/2018 05: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 jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an initial 
set of automated tests, and contains fixes to the following issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the next 
few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file association 
on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests

Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

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

2018-11-14 Thread Roger Riggs


Hi,

On 11/14/2018 09:52 AM, Andy Herrick wrote:


On 11/13/2018 5:50 PM, Philip Race wrote:



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 being public because they are all in a module and not exported.

I would put them all under 
share/classes/jdk/jpackagers/internal/ and

save a directory level.


That isn't how the rest of the JDK is organised.
Platform specific classes are split where you have "share" in the 
path above.
So whilst the other issues are more arguable I don't think the build 
team

would like platform specific classes under share. There is already an
objection to that for the native files.

To the "too many files in one package/directory" point.
I think that might happen at the directory level if Andy went through 
with

his suggestion but I don't think it will happen with what I proposed and
we probably should get some benefit from being able to make classes + 
methods

package private.


but my proposal only increases the number of files in each directory 
as follows:


share/classes/jdk/jpackager/internal   - from 28 to 31

windows/classes/jdk/jpackager/internal - from 6 to 7

macosx/classes/jdk/jpackager/internal   - from 6 to 7

linux/classes/jdk/jpackager/internal    - from 3 to 4

(adding Main.java, that makes are only 50 source files total)

so I hardly see any any impact of having "too many files in one 
package/directory"


and for Phil's part of the change, although the platform dependent 
source files would be in the same package as the platform independent 
files, they are still in a different source dir, and every platform 
specific source file's name starts with the platform ("Win", "Mac", or 
"Linux")

This should be fine, since there is no need to for cross-platform support,
the reason for the top level platform directory is so the build can be 
selectively

include the right files.

I assume the service loader mechanism deals with all the issues in 
locating platform specific files.



finally a question about resources:

Each source file that references resources has it's own resource file 
(23 resource files in each of 3 languages for a total of 69 properties 
files).


That seems like overkill to me. One result is duplicate resources 
whenever a key is referenced in more than one source file.


Wouldn't it simplify things to combine these into a smaller set of 
resource files ?
agreed, there should be fewer resource files to track and maintain and 
NO duplication.


Roger




/Andy




So I think what I proposed is about right ..

phil


$.02, Roger


On 11/13/2018 03:46 PM, Andy Herrick wrote:

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 in ./linux/classes/jdk/jpackager/internal/builders/linux 
- why not just ./linux/classes/jdk/jpackager/internal


1 file is in ./macosx/classes/jdk/jpackager/internal/builders/mac - 
why not just ./macosx/classes/jdk/jpackager/internal


1 file is in 
./windows/classes/jdk/jpackager/internal/builders/windows - why not 
just ./windows/classes/jdk/jpackager/internal


then just move the associated resources -

Basically put everything except Main in same package - everything 
would be easier to find, and we could make almost all methods 
package-private (the only exception would be the few things called 
by Main, and the ToolProvider.



On 11/13/2018 2:54 PM, Phil Race wrote:
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 

src/jdk.jpackager/linux/classes/jdk/jpackager/internal/linux/LinuxRpmBundler.java 



There's not likely to be a clash, so is there some other reason 
not to want these

in the same package as the shared internals like
src/jdk.jpackager/share/classes/jdk/jpackager/internal/Param.java

?

-phil.

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

2018-11-14 Thread Andy Herrick




On 11/13/2018 5:50 PM, Philip Race wrote:



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 being public because they are all in a module and not exported.

I would put them all under share/classes/jdk/jpackagers/internal/ 
and

save a directory level.


That isn't how the rest of the JDK is organised.
Platform specific classes are split where you have "share" in the path 
above.

So whilst the other issues are more arguable I don't think the build team
would like platform specific classes under share. There is already an
objection to that for the native files.

To the "too many files in one package/directory" point.
I think that might happen at the directory level if Andy went through 
with

his suggestion but I don't think it will happen with what I proposed and
we probably should get some benefit from being able to make classes + 
methods

package private.


but my proposal only increases the number of files in each directory as 
follows:


share/classes/jdk/jpackager/internal   - from 28 to 31

windows/classes/jdk/jpackager/internal - from 6 to 7

macosx/classes/jdk/jpackager/internal   - from 6 to 7

linux/classes/jdk/jpackager/internal    - from 3 to 4

(adding Main.java, that makes are only 50 source files total)

so I hardly see any any impact of having "too many files in one 
package/directory"


and for Phil's part of the change, although the platform dependent 
source files would be in the same package as the platform independent 
files, they are still in a different source dir, and every platform 
specific source file's name starts with the platform ("Win", "Mac", or 
"Linux")



finally a question about resources:

Each source file that references resources has it's own resource file 
(23 resource files in each of 3 languages for a total of 69 properties 
files).


That seems like overkill to me. One result is duplicate resources 
whenever a key is referenced in more than one source file.


Wouldn't it simplify things to combine these into a smaller set of 
resource files ?



/Andy




So I think what I proposed is about right ..

phil


$.02, Roger


On 11/13/2018 03:46 PM, Andy Herrick wrote:

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 in ./linux/classes/jdk/jpackager/internal/builders/linux - 
why not just ./linux/classes/jdk/jpackager/internal


1 file is in ./macosx/classes/jdk/jpackager/internal/builders/mac - 
why not just ./macosx/classes/jdk/jpackager/internal


1 file is in 
./windows/classes/jdk/jpackager/internal/builders/windows - why not 
just ./windows/classes/jdk/jpackager/internal


then just move the associated resources -

Basically put everything except Main in same package - everything 
would be easier to find, and we could make almost all methods 
package-private (the only exception would be the few things called 
by Main, and the ToolProvider.



On 11/13/2018 2:54 PM, Phil Race wrote:
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 

src/jdk.jpackager/linux/classes/jdk/jpackager/internal/linux/LinuxRpmBundler.java 



There's not likely to be a clash, so is there some other reason not 
to want these

in the same package as the shared internals like
src/jdk.jpackager/share/classes/jdk/jpackager/internal/Param.java

?

-phil.

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

2018-11-13 Thread Philip Race





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 being public because they are all in a module and not exported.

I would put them all under share/classes/jdk/jpackagers/internal/ and
save a directory level.


That isn't how the rest of the JDK is organised.
Platform specific classes are split where you have "share" in the path 
above.

So whilst the other issues are more arguable I don't think the build team
would like platform specific classes under share. There is already an
objection to that for the native files.

To the "too many files in one package/directory" point.
I think that might happen at the directory level if Andy went through with
his suggestion but I don't think it will happen with what I proposed and
we probably should get some benefit from being able to make classes + 
methods

package private.

So I think what I proposed is about right ..

phil


$.02, Roger


On 11/13/2018 03:46 PM, Andy Herrick wrote:

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 in ./linux/classes/jdk/jpackager/internal/builders/linux - 
why not just ./linux/classes/jdk/jpackager/internal


1 file is in ./macosx/classes/jdk/jpackager/internal/builders/mac - 
why not just ./macosx/classes/jdk/jpackager/internal


1 file is in 
./windows/classes/jdk/jpackager/internal/builders/windows - why not 
just ./windows/classes/jdk/jpackager/internal


then just move the associated resources -

Basically put everything except Main in same package - everything 
would be easier to find, and we could make almost all methods 
package-private (the only exception would be the few things called by 
Main, and the ToolProvider.



On 11/13/2018 2:54 PM, Phil Race wrote:
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 

src/jdk.jpackager/linux/classes/jdk/jpackager/internal/linux/LinuxRpmBundler.java 



There's not likely to be a clash, so is there some other reason not 
to want these

in the same package as the shared internals like
src/jdk.jpackager/share/classes/jdk/jpackager/internal/Param.java

?

-phil.


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 in ./linux/classes/jdk/jpackager/internal/builders/linux - 
why not just ./linux/classes/jdk/jpackager/internal


1 file is in ./macosx/classes/jdk/jpackager/internal/builders/mac - 
why not just ./macosx/classes/jdk/jpackager/internal


1 file is in 
./windows/classes/jdk/jpackager/internal/builders/windows - why not 
just ./windows/classes/jdk/jpackager/internal


then just move the associated resources -

Basically put everything except Main in same package - everything 
would be easier to find, and we could make almost all methods 
package-private (the only exception would be the few things called by 
Main, and the ToolProvider.


/Andy

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

2018-11-13 Thread Sverre Moe

Hello,
May I chime in a little on the jpackager.
I have been using it with OpenJDK 11, as backported by Johan Vos from Gluon.
It has worked fine, but I have noticed some flaws.

1)
The control file for DEB package does not set correct description

--name test
--description This is a Test Application

/tmp/jdk.packager607148779833718376/linux/control
Package: test
Description: test

The RPM gets it correctly
Summary : test
Description :
This is a Test Application

2)
Category is not set on either DEB or RPM
  --category
  Category or group of the application.
--category "Some/Category/Application"
Group: Unspecified
Section: unknown

Perhaps I have misunderstood what this category is for, because I see this
it is set in the generated application.desktop, but It should definitely
also be set in the RPM and DEB.

3)
There should be a --release flag to jpackager, at least for RPM. The only
other way to set release would be to supply a custom spec file.
Version: 1.0.0
Release: RC1


/Sverre


Den tir. 13. nov. 2018 kl. 22:30 skrev Andy Herrick :

>
> On 11/13/2018 4:08 PM, Roger Riggs wrote:
> > Hi,
> >
> > A few high level comments:
> >
> > The JDK already has a command option parser (JoptSimple) in the module
> > jdk.internal.opt
> > and the System Logger.  Why not use them for the argument parsing and
> > logging?
>
> We have an RFE to convert argument parsing to joptsimple that I filed
> last July JDK-8208300. 
>
> I spent a few days at the time prototyping it, and concluded it was a
> multi-week project.
>
> The team decided it was not a priority for JDK12 and it is now targeted
> to JDK13.
>
> >
> > Similarly, we've been encouraging developers to use the java.nio.file
> > APIs and
> > get away from java.io.File.  Try-with-resources could be used in a few
> > places
> > to improve the closing of resources.
> >
> > I can also see many places where the Streams functions could be used
> > to make the code easier to read (and write).  That's a missed
> > opportunity.
> >
> > What granularity of comments are you looking for?
>
> We are looking for three types of comments:
>
> 1.) Specific show-stopper issues that would prevent you from approving
> the inclusion of this project in JDK12
>
> 2.) Specific small problems that could be addressed in the limited time
> left for JDK12.
>
> 3.) Any other problems or ideas for improvement that we should consider
> for JDK13 or future releases.
>
> /Andy
>
> >
> > Thanks, Roger
> >
> >
> > On 11/09/2018 05: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 jdk.jpackager and
> >> jdk.jpackager.runtime, removes the JNLPConverter demo, adds an
> >> initial set of automated tests, and contains fixes to the following
> >> issues:
> >>
> >> JDK-8213324 jpackager deletes existing app directory without warning
> >> JDK-8213166 jpackager --argument arg is broken
> >> JDK-8213163 --app-image arg does not work creating exe installers
> >> JDK-8212089 Prepare packager for localization
> >> JDK-8212537 Create method and class description comments for main
> >> functionality
> >> JDK-8213332 Create minimal automated tests for jpackager
> >> JDK-821 Fix issues found in jpackager with automated tests
> >> JDK-8213394 Stop using Log.info() except for expected output.
> >> JDK-8213345 Secondary Launchers broken on mac.
> >> JDK-8213156 rename packages for jpackager
> >> JDK-8213244 Fix all warnings in jpackager java code
> >> JDK-8212143 Remove native code that supports UserJvmOptionsService
> >> JDK-8213162 Association description in Inno Setup cannot contain
> >> double quotes
> >>
> >> The following additional issues are targeted to be address in the
> >> next few weeks:
> >> JDK-8212936 Makefile and other improvements for jpackager
> >> JDK-8212164 resolve jre.list and jre.module.list
> >> JDK-8213392 Enhance --help and --version
> >> JDK-8208652 File name is not passed to main() via file
> >> association on OS X
> >> JDK-8212538 Determine standard way to determine if a Modular jar
> >> JDK-8213558 Create more unit tests
> >>
> >> Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/
> >>
> >> please send feedback to core-libs-dev@openjdk.java.net
> >>
> >> /Andy Herrick
> >>
> >
>

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




On 11/13/2018 4:08 PM, Roger Riggs wrote:

Hi,

A few high level comments:

The JDK already has a command option parser (JoptSimple) in the module 
jdk.internal.opt
and the System Logger.  Why not use them for the argument parsing and 
logging?


We have an RFE to convert argument parsing to joptsimple that I filed 
last July JDK-8208300. 


I spent a few days at the time prototyping it, and concluded it was a 
multi-week project.


The team decided it was not a priority for JDK12 and it is now targeted 
to JDK13.




Similarly, we've been encouraging developers to use the java.nio.file 
APIs and
get away from java.io.File.  Try-with-resources could be used in a few 
places

to improve the closing of resources.

I can also see many places where the Streams functions could be used
to make the code easier to read (and write).  That's a missed 
opportunity.


What granularity of comments are you looking for?


We are looking for three types of comments:

1.) Specific show-stopper issues that would prevent you from approving 
the inclusion of this project in JDK12


2.) Specific small problems that could be addressed in the limited time 
left for JDK12.


3.) Any other problems or ideas for improvement that we should consider 
for JDK13 or future releases.


/Andy



Thanks, Roger


On 11/09/2018 05: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 jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an 
initial set of automated tests, and contains fixes to the following 
issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the 
next few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file 
association on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests

Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

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

2018-11-13 Thread Roger Riggs


Hi,

A few high level comments:

The JDK already has a command option parser (JoptSimple) in the module 
jdk.internal.opt
and the System Logger.  Why not use them for the argument parsing and 
logging?


Similarly, we've been encouraging developers to use the java.nio.file 
APIs and
get away from java.io.File.  Try-with-resources could be used in a few 
places

to improve the closing of resources.

I can also see many places where the Streams functions could be used
to make the code easier to read (and write).  That's a missed opportunity.

What granularity of comments are you looking for?

Thanks, Roger


On 11/09/2018 05: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 jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an initial 
set of automated tests, and contains fixes to the following issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the next 
few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file association 
on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests

Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

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 in ./linux/classes/jdk/jpackager/internal/builders/linux - why 
not just ./linux/classes/jdk/jpackager/internal


1 file is in ./macosx/classes/jdk/jpackager/internal/builders/mac - why 
not just ./macosx/classes/jdk/jpackager/internal


1 file is in ./windows/classes/jdk/jpackager/internal/builders/windows - 
why not just ./windows/classes/jdk/jpackager/internal


then just move the associated resources -

Basically put everything except Main in same package - everything would 
be easier to find, and we could make almost all methods package-private 
(the only exception would be the few things called by Main, and the 
ToolProvider.



On 11/13/2018 2:54 PM, Phil Race wrote:
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 

src/jdk.jpackager/linux/classes/jdk/jpackager/internal/linux/LinuxRpmBundler.java 



There's not likely to be a clash, so is there some other reason not to 
want these

in the same package as the shared internals like
src/jdk.jpackager/share/classes/jdk/jpackager/internal/Param.java

?

-phil.


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 in ./linux/classes/jdk/jpackager/internal/builders/linux - why 
not just ./linux/classes/jdk/jpackager/internal


1 file is in ./macosx/classes/jdk/jpackager/internal/builders/mac - why 
not just ./macosx/classes/jdk/jpackager/internal


1 file is in ./windows/classes/jdk/jpackager/internal/builders/windows - 
why not just ./windows/classes/jdk/jpackager/internal


then just move the associated resources -

Basically put everything except Main in same package - everything would 
be easier to find, and we could make almost all methods package-private 
(the only exception would be the few things called by Main, and the 
ToolProvider.


/Andy

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

2018-11-13 Thread Phil Race

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
src/jdk.jpackager/linux/classes/jdk/jpackager/internal/linux/LinuxRpmBundler.java

There's not likely to be a clash, so is there some other reason not to 
want these

in the same package as the shared internals like
src/jdk.jpackager/share/classes/jdk/jpackager/internal/Param.java

?

-phil.

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 + 
"\\AppData\\LocalLow\\Sun\\Java\\JPackager\\tmp";

  80 } else if (os.contains("mac") || os.contains("os x")) {
  81 return System.getProperty("user.home")
  82 + "/Library/Application 
Support/Oracle/Java/JPackager/tmp";

  83 } else if (os.contains("nix") || os.contains("nux")
  84 || os.contains("aix")) {
  85 return System.getProperty("user.home") + 
"/.java/jpackager/tmp";

  86 }
  87
  88 return System.getProperty("java.io.tmpdir");


This seems unduly complex, and I don't understand the implication of
supporting AIX .. or some unknown "Unix", when packager is targeted
only at mac, linux + windows.
user.home is specified to be the user's home directory so I would 
think it should use that consistently everywhere. I assume "Sun" and 
"Oracle" can be dropped from the file location too.


Agreed - the resulting paths will all start with 
System.getProperty("user.home") and the "Sun" and "Oracle" 
sub-directories will be removed both here and in the matching native 
launcher code.  Added that to JDK-8213756 



/Andy




-Alan

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




On 11/12/2018 6:36 PM, Alan Snyder wrote:

Can someone say whether the two-step package creation feature is implemented 
and explain how to use it?


Yes -

1.) "jpackager create-image -o output -n app-name ..."

2.) "jpackager create-installer -o installer-output -n installer-name 
--app-image 'output\app-name' ..." (windows)


"jpackager create-installer -o installer-output -n installer-name 
--app-image 'output/app-name.app' ..." (for mac)


"jpackager create-installer -o installer-output -n installer-name 
--app-image 'output\app' ..." (for linux)




What is the plan for documentation? The command line help is inadequate.


Yes - there will be further documentation, it's content and form are 
still under discussion.


/Andy




On Oct 26, 2018, at 1:09 PM, Alan Snyder wrote:

I noticed the following statement in the JEP:

  In this latter case, the tool can either create a native package from a 
previously created application image, or it can create a native package 
directly.

I cannot tell from the command summary whether this option has been implemented 
or not.

Without this feature, the ability to create a signed DMG is not very useful, 
except in the case where the tool creates exactly the right application image.

  Alan

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 



/Andy

On 11/12/2018 4:40 PM, 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 + "\\AppData\\LocalLow\\Sun\\Java\\JPackager\\tmp";
   80 } else if (os.contains("mac") || os.contains("os x")) {
   81 return System.getProperty("user.home")
   82 + "/Library/Application 
Support/Oracle/Java/JPackager/tmp";
   83 } else if (os.contains("nix") || os.contains("nux")
   84 || os.contains("aix")) {
   85 return System.getProperty("user.home") + 
"/.java/jpackager/tmp";
   86 }
   87
   88 return System.getProperty("java.io.tmpdir");

This seems unduly complex, and I don't understand the implication of
supporting AIX .. or some unknown "Unix", when packager is targeted
only at mac, linux + windows.

I think its sufficient to look for the strings windows, macos and linux.
And if you want a persistent storage location then default to the "unix"
location if there's no match .. although I am not sure it makes sense
on platforms that aren't targeted by jpackager.

-phil.


-phil.

On 11/12/18, 12: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
when the API is used from a running desktop application.

I might ask if we need this at all, but I definitely think it should
not be required on all platforms if needed only for Mac even if
we might want it on windows in a future version.

Do we not envisage cases where you need the runtime API for
some kind of daemon service for which there should be a singleton ?
Do you really want to force the desktop module to be dragged along
in such a case ?

I think we should remove this dependency even if it means losing a
MacOS feature at least for now.

-phil.

On 11/11/18, 2:40 PM, Andy Herrick wrote:

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 jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an 
initial set of automated tests, and contains fixes to the 
following issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the 
next few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file 
association on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests
Note: The above issues are targeted to 'internal' - meaning we 
expect to resolve them in the sandbox before the initial push to 
JDK12.
Issues targeted to '12' are expected to be fixed after the initial 
push.


/Andy


Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

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

2018-11-13 Thread Alan Bateman


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 + 
"\\AppData\\LocalLow\\Sun\\Java\\JPackager\\tmp";

  80 } else if (os.contains("mac") || os.contains("os x")) {
  81 return System.getProperty("user.home")
  82 + "/Library/Application 
Support/Oracle/Java/JPackager/tmp";

  83 } else if (os.contains("nix") || os.contains("nux")
  84 || os.contains("aix")) {
  85 return System.getProperty("user.home") + 
"/.java/jpackager/tmp";

  86 }
  87
  88 return System.getProperty("java.io.tmpdir");


This seems unduly complex, and I don't understand the implication of
supporting AIX .. or some unknown "Unix", when packager is targeted
only at mac, linux + windows.
user.home is specified to be the user's home directory so I would think 
it should use that consistently everywhere. I assume "Sun" and "Oracle" 
can be dropped from the file location too.


-Alan

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

2018-11-12 Thread Bernd Eckenfels

Hello,

I wonder if in the Windows temp case LocalLow is (still) the right place. If 
this Installer is not started in the low context it will endanger its extracted 
temp file by making them accessible to low integrity processes, right?

And if this should search %TEMP%., %LOCALAPPDATA%\temp first.

But the variables are known to have problems too, To be robust and Windows 
ready  compliant it looks like another jpackager runtime function for 
SHGetKnownFolderPath would be good (pretty sure it’s already used somewhere, 
just not with a stable API exposed?)

Gruss
Bernd
--
http://bernd.eckenfels.net


Von: core-libs-dev  im Auftrag von 
Philip Race 
Gesendet: Montag, November 12, 2018 10:55 PM
An: Andy Herrick
Cc: build-dev; core-libs-dev@openjdk.java.net
Betreff: 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 } else if (os.contains("mac") || os.contains("os x")) {
81 return System.getProperty("user.home")
82 + "/Library/Application Support/Oracle/Java/JPackager/tmp";
83 } else if (os.contains("nix") || os.contains("nux")
84 || os.contains("aix")) {
85 return System.getProperty("user.home") + "/.java/jpackager/tmp";
86 }
87
88 return System.getProperty("java.io.tmpdir");


This seems unduly complex, and I don't understand the implication of
supporting AIX .. or some unknown "Unix", when packager is targeted
only at mac, linux + windows.

I think its sufficient to look for the strings windows, macos and linux.
And if you want a persistent storage location then default to the "unix"
location if there's no match .. although I am not sure it makes sense
on platforms that aren't targeted by jpackager.

-phil.
>
> -phil.
>
> On 11/12/18, 12: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
>> when the API is used from a running desktop application.
>>
>> I might ask if we need this at all, but I definitely think it should
>> not be required on all platforms if needed only for Mac even if
>> we might want it on windows in a future version.
>>
>> Do we not envisage cases where you need the runtime API for
>> some kind of daemon service for which there should be a singleton ?
>> Do you really want to force the desktop module to be dragged along
>> in such a case ?
>>
>> I think we should remove this dependency even if it means losing a
>> MacOS feature at least for now.
>>
>> -phil.
>>
>> On 11/11/18, 2:40 PM, Andy Herrick wrote:
>>> 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 <https://bugs.openjdk.java.net/browse/JDK-8200758>
>>>>
>>>> This refresh renames the packages used to jdk.jpackager and
>>>> jdk.jpackager.runtime, removes the JNLPConverter demo, adds an
>>>> initial set of automated tests, and contains fixes to the following
>>>> issues:
>>>>
>>>> JDK-8213324 jpackager deletes existing app directory without warning
>>>> JDK-8213166 jpackager --argument arg is broken
>>>> JDK-8213163 --app-image arg does not work creating exe installers
>>>> JDK-8212089 Prepare packager for localization
>>>> JDK-8212537 Create method and class description comments for main
>>>> functionality
>>>> JDK-8213332 Create minimal automated tests for jpackager
>>>> JDK-821 Fix issues found in jpackager with automated tests
>>>> JDK-8213394 Stop using Log.info() except for expected output.
>>>> JDK-8213345 Secondary Launchers broken on mac.
>>>> JDK-8213156 rename packages for jpackager
>>>> JDK-8213244 Fix all warnings in jpackager java code
>>>> JDK-8212143 Remove native code that supports UserJvmOptionsService
>>>> JDK-8213162 Association description in Inno Setup cannot contain
>>>> double quotes
>>>>
>>>> The following additional issues are targeted to be address in the
>>>> next few weeks:
>>>> JDK-8212936

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

2018-11-12 Thread Alan Snyder

Can someone say whether the two-step package creation feature is implemented 
and explain how to use it?

What is the plan for documentation? The command line help is inadequate.

> On Oct 26, 2018, at 1:09 PM, Alan Snyder wrote:
> 
> I noticed the following statement in the JEP:
> 
>  In this latter case, the tool can either create a native package from a 
> previously created application image, or it can create a native package 
> directly.
> 
> I cannot tell from the command summary whether this option has been 
> implemented or not.
> 
> Without this feature, the ability to create a signed DMG is not very useful, 
> except in the case where the tool creates exactly the right application image.
> 
>  Alan
>

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} for current process.

and
 122  * Unregisters {@code SingleInstanceListener} for current process.
 123  * If the {@code SingleInstanceListener} object is not registered, or
 124  * {@code slistener} is {@code null}, then the unregistration is 
skipped.


"the current process"

There may be more like that.

and as I mentioned offline, I think the correct word is "deregister" not 
"unregister"

both in comments and method names.

-phill

On 11/12/18, 2:38 PM, Andy Herrick wrote:



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 for receiving and
handling file open events. Probably it only makes sense or gets used
when the API is used from a running desktop application.

I might ask if we need this at all, but I definitely think it should
not be required on all platforms if needed only for Mac even if
we might want it on windows in a future version.

Do we not envisage cases where you need the runtime API for
some kind of daemon service for which there should be a singleton ?
Do you really want to force the desktop module to be dragged along
in such a case ?

I think we should remove this dependency even if it means losing a
MacOS feature at least for now.


1.) we could remove that functionality (I don't even really 
understand the use case for it), and remove the two arg 
registerSingleInstance method from the public interface, and remove 
the requires statement in module-info.java.


I was thinking remove it as above since since even the javadoc for 
that method

feels obliged to say it is just for macos :-

  95 /**
  96  * Registers {@code SingleInstanceListener} for current 
process.
  97  * If the {@code SingleInstanceListener} object is already 
registered, or
  98  * {@code slistener} is {@code null}, then the registration 
is skipped.

  99  *
 100  * @param slistener the listener to handle the single 
instance behaviour.
 101  * @param setFileHandler if {@code true}, the listener is 
notified when the
 102  * application is asked to open a list of files. If 
OS is not MacOS,

 103  * the parameter is ignored.
 104  */


yuck. If such optional functionality is needed it needs to be done via
adding a "doesPlatformSupportFileHandler() method rather than saying 
the above.


-phil.


OK - I filed JDK-8213756 
 to handle removing 
this API, fixing the call to new SingleInstanceImpl() to be wrapped in 
synchronized null check, and fixing comments you referred to .


/Andy




or ...

2.) we could move that functionality to a platform dependent class, 
having dummy methods of setOpenFileHandler in the windows and linux  
platform dependent class.  Then we could add a 
module-info.java.extra in src/jdk.jpackager.runtime/macos/classes 
with that requires statement.


/Andy



-phil.

On 11/11/18, 2:40 PM, Andy Herrick wrote:

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 jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an 
initial set of automated tests, and contains fixes to the 
following issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the 
next few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file 
association on OS X

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

2018-11-12 Thread Andy Herrick




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 for receiving and
handling file open events. Probably it only makes sense or gets used
when the API is used from a running desktop application.

I might ask if we need this at all, but I definitely think it should
not be required on all platforms if needed only for Mac even if
we might want it on windows in a future version.

Do we not envisage cases where you need the runtime API for
some kind of daemon service for which there should be a singleton ?
Do you really want to force the desktop module to be dragged along
in such a case ?

I think we should remove this dependency even if it means losing a
MacOS feature at least for now.


1.) we could remove that functionality (I don't even really 
understand the use case for it), and remove the two arg 
registerSingleInstance method from the public interface, and remove 
the requires statement in module-info.java.


I was thinking remove it as above since since even the javadoc for 
that method

feels obliged to say it is just for macos :-

  95 /**
  96  * Registers {@code SingleInstanceListener} for current process.
  97  * If the {@code SingleInstanceListener} object is already 
registered, or
  98  * {@code slistener} is {@code null}, then the registration 
is skipped.

  99  *
 100  * @param slistener the listener to handle the single 
instance behaviour.
 101  * @param setFileHandler if {@code true}, the listener is 
notified when the
 102  * application is asked to open a list of files. If 
OS is not MacOS,

 103  * the parameter is ignored.
 104  */


yuck. If such optional functionality is needed it needs to be done via
adding a "doesPlatformSupportFileHandler() method rather than saying 
the above.


-phil.


OK - I filed JDK-8213756 
 to handle removing 
this API, fixing the call to new SingleInstanceImpl() to be wrapped in 
synchronized null check, and fixing comments you referred to .


/Andy




or ...

2.) we could move that functionality to a platform dependent class, 
having dummy methods of setOpenFileHandler in the windows and linux  
platform dependent class.  Then we could add a module-info.java.extra 
in src/jdk.jpackager.runtime/macos/classes with that requires statement.


/Andy



-phil.

On 11/11/18, 2:40 PM, Andy Herrick wrote:

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 jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an 
initial set of automated tests, and contains fixes to the 
following issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the 
next few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file 
association on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests
Note: The above issues are targeted to 'internal' - meaning we 
expect to resolve them in the sandbox before the initial push to 
JDK12.
Issues targeted to '12' are expected to be fixed after the initial 
push.


/Andy


Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

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. Probably it only makes sense or gets used
when the API is used from a running desktop application.

I might ask if we need this at all, but I definitely think it should
not be required on all platforms if needed only for Mac even if
we might want it on windows in a future version.

Do we not envisage cases where you need the runtime API for
some kind of daemon service for which there should be a singleton ?
Do you really want to force the desktop module to be dragged along
in such a case ?

I think we should remove this dependency even if it means losing a
MacOS feature at least for now.


1.) we could remove that functionality (I don't even really understand 
the use case for it), and remove the two arg registerSingleInstance 
method from the public interface, and remove the requires statement in 
module-info.java.


I was thinking remove it as above since since even the javadoc for that 
method

feels obliged to say it is just for macos :-

  95 /**
  96  * Registers {@code SingleInstanceListener} for current process.
  97  * If the {@code SingleInstanceListener} object is already registered, 
or
  98  * {@code slistener} is {@code null}, then the registration is skipped.
  99  *
 100  * @param slistener the listener to handle the single instance 
behaviour.
 101  * @param setFileHandler if {@code true}, the listener is notified 
when the
 102  * application is asked to open a list of files. If OS is not 
MacOS,
 103  * the parameter is ignored.
 104  */


yuck. If such optional functionality is needed it needs to be done via
adding a "doesPlatformSupportFileHandler() method rather than saying the above.

-phil.


or ...

2.) we could move that functionality to a platform dependent class, 
having dummy methods of setOpenFileHandler in the windows and linux  
platform dependent class.  Then we could add a module-info.java.extra 
in src/jdk.jpackager.runtime/macos/classes with that requires statement.


/Andy



-phil.

On 11/11/18, 2:40 PM, Andy Herrick wrote:

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 jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an 
initial set of automated tests, and contains fixes to the following 
issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the 
next few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file 
association on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests
Note: The above issues are targeted to 'internal' - meaning we 
expect to resolve them in the sandbox before the initial push to JDK12.
Issues targeted to '12' are expected to be fixed after the initial 
push.


/Andy


Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

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

2018-11-12 Thread Andy Herrick




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
when the API is used from a running desktop application.

I might ask if we need this at all, but I definitely think it should
not be required on all platforms if needed only for Mac even if
we might want it on windows in a future version.

Do we not envisage cases where you need the runtime API for
some kind of daemon service for which there should be a singleton ?
Do you really want to force the desktop module to be dragged along
in such a case ?

I think we should remove this dependency even if it means losing a
MacOS feature at least for now.


1.) we could remove that functionality (I don't even really understand 
the use case for it), and remove the two arg registerSingleInstance 
method from the public interface, and remove the requires statement in 
module-info.java.


or ...

2.) we could move that functionality to a platform dependent class, 
having dummy methods of setOpenFileHandler in the windows and linux  
platform dependent class.  Then we could add a module-info.java.extra in 
src/jdk.jpackager.runtime/macos/classes with that requires statement.


/Andy



-phil.

On 11/11/18, 2:40 PM, Andy Herrick wrote:

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 jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an 
initial set of automated tests, and contains fixes to the following 
issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the 
next few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file 
association on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests
Note: The above issues are targeted to 'internal' - meaning we expect 
to resolve them in the sandbox before the initial push to JDK12.

Issues targeted to '12' are expected to be fixed after the initial push.

/Andy


Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

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 } else if (os.contains("mac") || os.contains("os x")) {
  81 return System.getProperty("user.home")
  82 + "/Library/Application 
Support/Oracle/Java/JPackager/tmp";
  83 } else if (os.contains("nix") || os.contains("nux")
  84 || os.contains("aix")) {
  85 return System.getProperty("user.home") + 
"/.java/jpackager/tmp";
  86 }
  87
  88 return System.getProperty("java.io.tmpdir");


This seems unduly complex, and I don't understand the implication of
supporting AIX .. or some unknown "Unix", when packager is targeted
only at mac, linux + windows.

I think its sufficient to look for the strings windows, macos and linux.
And if you want a persistent storage location then default to the "unix"
location if there's no match .. although I am not sure it makes sense
on platforms that aren't targeted by jpackager.

-phil.


-phil.

On 11/12/18, 12: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
when the API is used from a running desktop application.

I might ask if we need this at all, but I definitely think it should
not be required on all platforms if needed only for Mac even if
we might want it on windows in a future version.

Do we not envisage cases where you need the runtime API for
some kind of daemon service for which there should be a singleton ?
Do you really want to force the desktop module to be dragged along
in such a case ?

I think we should remove this dependency even if it means losing a
MacOS feature at least for now.

-phil.

On 11/11/18, 2:40 PM, Andy Herrick wrote:

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 jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an 
initial set of automated tests, and contains fixes to the following 
issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the 
next few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file 
association on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests
Note: The above issues are targeted to 'internal' - meaning we 
expect to resolve them in the sandbox before the initial push to JDK12.
Issues targeted to '12' are expected to be fixed after the initial 
push.


/Andy


Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

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 a new instance and assigns
it to the static variable.

 114 instance = new SingleInstanceImpl();

Shouldn't this be wrapped in a synchronized null check ?

And what is the contract for equality of a SingleInstanceListener.

The API defines it as an interface but says nothing more .

And I'm not sure what is meant by the 2nd part of this below :-

36 /**
  37  * This method should be implemented by the application to
  38  * handle the single instance behaviour - how should the application
  39  * handle the arguments when another instance of the application is
  40  * invoked with params.

It is phrased like a question without a question mark.

Is it supposed to mean more like :

36 /**
  37  * This method must be implemented by the application to
  38  * handle the single instance behaviour. For example it will
  * need to resolve cases where the parameter list of the new
  * activation in conflict with the existing activation




-phil.

On 11/12/18, 12: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
when the API is used from a running desktop application.

I might ask if we need this at all, but I definitely think it should
not be required on all platforms if needed only for Mac even if
we might want it on windows in a future version.

Do we not envisage cases where you need the runtime API for
some kind of daemon service for which there should be a singleton ?
Do you really want to force the desktop module to be dragged along
in such a case ?

I think we should remove this dependency even if it means losing a
MacOS feature at least for now.

-phil.

On 11/11/18, 2:40 PM, Andy Herrick wrote:

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 jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an 
initial set of automated tests, and contains fixes to the following 
issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the 
next few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file 
association on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests
Note: The above issues are targeted to 'internal' - meaning we expect 
to resolve them in the sandbox before the initial push to JDK12.

Issues targeted to '12' are expected to be fixed after the initial push.

/Andy


Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

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