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

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

2019-12-03 Thread Andy Herrick

On 12/2/2019 2:07 PM, Phil Race wrote:

This makes it very difficult to do more than a cursory re-review.

There is one thing I just spotted.

I believe these two scripts
http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/test/jdk/tools/jpackage/test_jpackage.sh.html
http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/test/jdk/tools/jpackage/run_tests.sh.html

OK - run_tests.sh has been modified to not download jtreg from an
external server.

/Andy

Should not be part of what is pushed.

They should be removed from the webrev.

-phil.

On 11/27/19 6:07 AM, Andy Herrick wrote:

yes - The attempted incremental patch is not much use. The source
files were moved several times, and despite using "hg addremove -s
60", many of the files show as a remove and a new file.

/Andy

On 11/26/2019 9:01 PM, Philip Race wrote:
> I believe otherwise it is an accurate incremental webrev of the
jpackage changes since EA-5.

It is also not very incremental.
*
*src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/main/Main.java

is definitely not "new" ...

-phil.

On 11/26/19, 2:17 PM, Andy Herrick wrote:
yes, this incremental webrev contains the JNLPConverter code,
which it should not.

I believe otherwise it is an accurate incremental webrev of the
jpackage changes since EA-5.

/Andy

On 11/26/2019 4:53 PM, Phil Race wrote:

Andy,

I am puzzled by what I see here
> The webrev at [3] shows the changes since EA-06 (Build
13-jpackage+1-49 ) up to the current point.

> [3] http://cr.openjdk.java.net/~herrick/8212780/webrev.06-10/

This includes the JNLPConverter which isn't what I expected to see
and

(correctly) isn't in the cumulative webrev

Since [3] seemed like the most obvious thing to do a review of the
recent
changes I'd like to be sure it has the correct content and I am
not sure it does ...

-phil.

On 11/26/19 1:36 PM, Kevin Rushforth wrote:

This all looks good.

-- Kevin

On 11/26/2019 12:56 PM, Erik Joelsson wrote:

(adding build-dev)

Build changes look good.

/Erik

On 2019-11-20 09:37, Andy Herrick wrote:
I posted new composite webrev [7], and git patch [8] after
pushing fix for JDK-8234402 [6].

[7] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/

[8]
http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-11.git.patch

/Andy

On 11/19/2019 3:13 PM, Kevin Rushforth wrote:
I took the "git diff" patch [5] that you uploaded yesterday,
applied it, and verified that it is the same as what is in the
JDK-8200758-branch branch of the sandbox. It builds and runs
fine for me.

I ran jcheck and it found the following three files with
whitespace errors that will need to be fixed before you push:

src/jdk.incubator.jpackage/linux/classes/jdk/incubator/jpackage/internal/PackageProperty.java:49:
Trailing whitespace
src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/ToolProviderFactory.java:35:
Trailing whitespace
test/jdk/tools/jpackage/share/AddLauncherBase.java:137:
Trailing whitespace

The second of these will go away with the fix for JDK-8234402
[6], so you don't need to do anything there. Once the fix for
JDK-8234402 is pushed to sandbox, I presume you will update
the webrev, right?

Pending the fix for JDK-8234402 and the needed white-space
fixes, this is a +1 from me (although I am not a jdk Project
Reviewer, so you will need at least one review from someone
who is).

-- Kevin

[5]
http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-10.git.patch

[6] https://bugs.openjdk.java.net/browse/JDK-8234402

On 11/13/2019 4:23 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 since EA-06 (Build
13-jpackage+1-49 ) up to the current point.

The latest EA (Build 14-jpackage+1-49 ) is posted at [4].

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

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

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

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

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

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

2019-12-02 Thread Phil Race

This makes it very difficult to do more than a cursory re-review.

There is one thing I just spotted.

I believe these two scripts
http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/test/jdk/tools/jpackage/test_jpackage.sh.html
http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/test/jdk/tools/jpackage/run_tests.sh.html

Should not be part of what is pushed.

They should be removed from the webrev.

-phil.

On 11/27/19 6:07 AM, Andy Herrick wrote:

yes - The attempted incremental patch is not much use.  The source 
files were moved several times, and despite using "hg addremove -s 
60", many of the files show as a remove and a new file.

/Andy

On 11/26/2019 9:01 PM, Philip Race wrote:
> I believe otherwise it is an accurate incremental webrev of the 
jpackage changes since EA-5.

It is also not very incremental.
*
*src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/main/Main.java

is definitely not "new" ...

-phil.

On 11/26/19, 2:17 PM, Andy Herrick wrote:
yes,  this incremental webrev contains the JNLPConverter code, which 
it should not.

I believe otherwise it is an accurate incremental webrev of the 
jpackage changes since EA-5.

/Andy

On 11/26/2019 4:53 PM, Phil Race wrote:

Andy,

I am puzzled by what I see here
> The webrev at [3] shows the changes since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.

> [3] http://cr.openjdk.java.net/~herrick/8212780/webrev.06-10/

This includes the JNLPConverter which isn't what I expected to see and
(correctly) isn't in the cumulative webrev 

Since [3] seemed like the most obvious thing to do a review of the 
recent
changes I'd like to be sure it has the correct content and I am not 
sure it does ...

-phil.

On 11/26/19 1:36 PM, Kevin Rushforth wrote:

This all looks good.

+1

-- Kevin

On 11/26/2019 12:56 PM, Erik Joelsson wrote:

(adding build-dev)

Build changes look good.

/Erik

On 2019-11-20 09:37, Andy Herrick wrote:
I posted new composite webrev [7], and git patch [8] after 
pushing fix for JDK-8234402 [6].

[7] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/

[8] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-11.git.patch

/Andy

On 11/19/2019 3:13 PM, Kevin Rushforth wrote:
I took the "git diff" patch [5] that you uploaded yesterday, 
applied it, and verified that it is the same as what is in the 
JDK-8200758-branch branch of the sandbox. It builds and runs 
fine for me.

I ran jcheck and it found the following three files with 
whitespace errors that will need to be fixed before you push:

src/jdk.incubator.jpackage/linux/classes/jdk/incubator/jpackage/internal/PackageProperty.java:49: 
Trailing whitespace
src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/ToolProviderFactory.java:35: 
Trailing whitespace
test/jdk/tools/jpackage/share/AddLauncherBase.java:137: 
Trailing whitespace

The second of these will go away with the fix for JDK-8234402 
[6], so you don't need to do anything there. Once the fix for 
JDK-8234402 is pushed to sandbox, I presume you will update the 
webrev, right?

Pending the fix for JDK-8234402 and the needed white-space 
fixes, this is a +1 from me (although I am not a jdk Project 
Reviewer, so you will need at least one review from someone who 
is).

-- Kevin

[5] 
http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-10.git.patch

[6] https://bugs.openjdk.java.net/browse/JDK-8234402

On 11/13/2019 4:23 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 since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.

The latest EA (Build 14-jpackage+1-49 ) is posted at [4].

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

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

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

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

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

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

2019-11-27 Thread Andy Herrick

yes - The attempted incremental patch is not much use.  The source files 
were moved several times, and despite using "hg addremove -s 60", many 
of the files show as a remove and a new file.

/Andy

On 11/26/2019 9:01 PM, Philip Race wrote:
> I believe otherwise it is an accurate incremental webrev of the 
jpackage changes since EA-5.

It is also not very incremental.
*
*src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/main/Main.java

is definitely not "new" ...

-phil.

On 11/26/19, 2:17 PM, Andy Herrick wrote:
yes,  this incremental webrev contains the JNLPConverter code, which 
it should not.

I believe otherwise it is an accurate incremental webrev of the 
jpackage changes since EA-5.

/Andy

On 11/26/2019 4:53 PM, Phil Race wrote:

Andy,

I am puzzled by what I see here
> The webrev at [3] shows the changes since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.

> [3] http://cr.openjdk.java.net/~herrick/8212780/webrev.06-10/

This includes the JNLPConverter which isn't what I expected to see and
(correctly) isn't in the cumulative webrev 

Since [3] seemed like the most obvious thing to do a review of the 
recent
changes I'd like to be sure it has the correct content and I am not 
sure it does ...

-phil.

On 11/26/19 1:36 PM, Kevin Rushforth wrote:

This all looks good.

+1

-- Kevin

On 11/26/2019 12:56 PM, Erik Joelsson wrote:

(adding build-dev)

Build changes look good.

/Erik

On 2019-11-20 09:37, Andy Herrick wrote:
I posted new composite webrev [7], and git patch [8] after 
pushing fix for JDK-8234402 [6].

[7] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/

[8] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-11.git.patch

/Andy

On 11/19/2019 3:13 PM, Kevin Rushforth wrote:
I took the "git diff" patch [5] that you uploaded yesterday, 
applied it, and verified that it is the same as what is in the 
JDK-8200758-branch branch of the sandbox. It builds and runs 
fine for me.

I ran jcheck and it found the following three files with 
whitespace errors that will need to be fixed before you push:

src/jdk.incubator.jpackage/linux/classes/jdk/incubator/jpackage/internal/PackageProperty.java:49: 
Trailing whitespace
src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/ToolProviderFactory.java:35: 
Trailing whitespace
test/jdk/tools/jpackage/share/AddLauncherBase.java:137: Trailing 
whitespace

The second of these will go away with the fix for JDK-8234402 
[6], so you don't need to do anything there. Once the fix for 
JDK-8234402 is pushed to sandbox, I presume you will update the 
webrev, right?

Pending the fix for JDK-8234402 and the needed white-space 
fixes, this is a +1 from me (although I am not a jdk Project 
Reviewer, so you will need at least one review from someone who 
is).

-- Kevin

[5] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-10.git.patch
[6] https://bugs.openjdk.java.net/browse/JDK-8234402

On 11/13/2019 4:23 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 since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.

The latest EA (Build 14-jpackage+1-49 ) is posted at [4].

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

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

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

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

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

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

2019-11-26 Thread Philip Race

> I believe otherwise it is an accurate incremental webrev of the 
jpackage changes since EA-5.

It is also not very incremental.
*
*src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/main/Main.java

is definitely not "new" ...

-phil.

On 11/26/19, 2:17 PM, Andy Herrick wrote:
yes,  this incremental webrev contains the JNLPConverter code, which 
it should not.

I believe otherwise it is an accurate incremental webrev of the 
jpackage changes since EA-5.

/Andy

On 11/26/2019 4:53 PM, Phil Race wrote:

Andy,

I am puzzled by what I see here
> The webrev at [3] shows the changes since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.

> [3] http://cr.openjdk.java.net/~herrick/8212780/webrev.06-10/

This includes the JNLPConverter which isn't what I expected to see and
(correctly) isn't in the cumulative webrev 

Since [3] seemed like the most obvious thing to do a review of the 
recent
changes I'd like to be sure it has the correct content and I am not 
sure it does ...

-phil.

On 11/26/19 1:36 PM, Kevin Rushforth wrote:

This all looks good.

+1

-- Kevin

On 11/26/2019 12:56 PM, Erik Joelsson wrote:

(adding build-dev)

Build changes look good.

/Erik

On 2019-11-20 09:37, Andy Herrick wrote:
I posted new composite webrev [7], and git patch [8] after pushing 
fix for JDK-8234402 [6].

[7] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/

[8] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-11.git.patch

/Andy

On 11/19/2019 3:13 PM, Kevin Rushforth wrote:
I took the "git diff" patch [5] that you uploaded yesterday, 
applied it, and verified that it is the same as what is in the 
JDK-8200758-branch branch of the sandbox. It builds and runs fine 
for me.

I ran jcheck and it found the following three files with 
whitespace errors that will need to be fixed before you push:

src/jdk.incubator.jpackage/linux/classes/jdk/incubator/jpackage/internal/PackageProperty.java:49: 
Trailing whitespace
src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/ToolProviderFactory.java:35: 
Trailing whitespace
test/jdk/tools/jpackage/share/AddLauncherBase.java:137: Trailing 
whitespace

The second of these will go away with the fix for JDK-8234402 
[6], so you don't need to do anything there. Once the fix for 
JDK-8234402 is pushed to sandbox, I presume you will update the 
webrev, right?

Pending the fix for JDK-8234402 and the needed white-space fixes, 
this is a +1 from me (although I am not a jdk Project Reviewer, 
so you will need at least one review from someone who is).

-- Kevin

[5] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-10.git.patch
[6] https://bugs.openjdk.java.net/browse/JDK-8234402

On 11/13/2019 4:23 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 since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.

The latest EA (Build 14-jpackage+1-49 ) is posted at [4].

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

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

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

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

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

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

2019-11-26 Thread Andy Herrick

yes,  this incremental webrev contains the JNLPConverter code, which it 
should not.

I believe otherwise it is an accurate incremental webrev of the jpackage 
changes since EA-5.

/Andy

On 11/26/2019 4:53 PM, Phil Race wrote:

Andy,

I am puzzled by what I see here
> The webrev at [3] shows the changes since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.

> [3] http://cr.openjdk.java.net/~herrick/8212780/webrev.06-10/

This includes the JNLPConverter which isn't what I expected to see and
(correctly) isn't in the cumulative webrev 

Since [3] seemed like the most obvious thing to do a review of the recent
changes I'd like to be sure it has the correct content and I am not 
sure it does ...

-phil.

On 11/26/19 1:36 PM, Kevin Rushforth wrote:

This all looks good.

+1

-- Kevin

On 11/26/2019 12:56 PM, Erik Joelsson wrote:

(adding build-dev)

Build changes look good.

/Erik

On 2019-11-20 09:37, Andy Herrick wrote:
I posted new composite webrev [7], and git patch [8] after pushing 
fix for JDK-8234402 [6].

[7] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/

[8] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-11.git.patch

/Andy

On 11/19/2019 3:13 PM, Kevin Rushforth wrote:
I took the "git diff" patch [5] that you uploaded yesterday, 
applied it, and verified that it is the same as what is in the 
JDK-8200758-branch branch of the sandbox. It builds and runs fine 
for me.

I ran jcheck and it found the following three files with 
whitespace errors that will need to be fixed before you push:

src/jdk.incubator.jpackage/linux/classes/jdk/incubator/jpackage/internal/PackageProperty.java:49: 
Trailing whitespace
src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/ToolProviderFactory.java:35: 
Trailing whitespace
test/jdk/tools/jpackage/share/AddLauncherBase.java:137: Trailing 
whitespace

The second of these will go away with the fix for JDK-8234402 [6], 
so you don't need to do anything there. Once the fix for 
JDK-8234402 is pushed to sandbox, I presume you will update the 
webrev, right?

Pending the fix for JDK-8234402 and the needed white-space fixes, 
this is a +1 from me (although I am not a jdk Project Reviewer, so 
you will need at least one review from someone who is).

-- Kevin

[5] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-10.git.patch
[6] https://bugs.openjdk.java.net/browse/JDK-8234402

On 11/13/2019 4:23 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 since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.

The latest EA (Build 14-jpackage+1-49 ) is posted at [4].

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

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

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

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

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

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

2019-11-26 Thread Phil Race

Andy,

I am puzzled by what I see here
> The webrev at [3] shows the changes since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.

> [3] http://cr.openjdk.java.net/~herrick/8212780/webrev.06-10/

This includes the JNLPConverter which isn't what I expected to see and
(correctly) isn't in the cumulative webrev 

Since [3] seemed like the most obvious thing to do a review of the recent
changes I'd like to be sure it has the correct content and I am not sure 
it does ...

-phil.

On 11/26/19 1:36 PM, Kevin Rushforth wrote:

This all looks good.

+1

-- Kevin

On 11/26/2019 12:56 PM, Erik Joelsson wrote:

(adding build-dev)

Build changes look good.

/Erik

On 2019-11-20 09:37, Andy Herrick wrote:
I posted new composite webrev [7], and git patch [8] after pushing 
fix for JDK-8234402 [6].

[7] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/

[8] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-11.git.patch

/Andy

On 11/19/2019 3:13 PM, Kevin Rushforth wrote:
I took the "git diff" patch [5] that you uploaded yesterday, 
applied it, and verified that it is the same as what is in the 
JDK-8200758-branch branch of the sandbox. It builds and runs fine 
for me.

I ran jcheck and it found the following three files with whitespace 
errors that will need to be fixed before you push:

src/jdk.incubator.jpackage/linux/classes/jdk/incubator/jpackage/internal/PackageProperty.java:49: 
Trailing whitespace
src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/ToolProviderFactory.java:35: 
Trailing whitespace
test/jdk/tools/jpackage/share/AddLauncherBase.java:137: Trailing 
whitespace

The second of these will go away with the fix for JDK-8234402 [6], 
so you don't need to do anything there. Once the fix for 
JDK-8234402 is pushed to sandbox, I presume you will update the 
webrev, right?

Pending the fix for JDK-8234402 and the needed white-space fixes, 
this is a +1 from me (although I am not a jdk Project Reviewer, so 
you will need at least one review from someone who is).

-- Kevin

[5] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-10.git.patch
[6] https://bugs.openjdk.java.net/browse/JDK-8234402

On 11/13/2019 4:23 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 since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.

The latest EA (Build 14-jpackage+1-49 ) is posted at [4].

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

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

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

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

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

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

2019-11-26 Thread Kevin Rushforth


This all looks good.

+1

-- Kevin


On 11/26/2019 12:56 PM, Erik Joelsson wrote:

(adding build-dev)

Build changes look good.

/Erik

On 2019-11-20 09:37, Andy Herrick wrote:
I posted new composite webrev [7], and git patch [8] after pushing 
fix for JDK-8234402 [6].


[7] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/

[8] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-11.git.patch

/Andy

On 11/19/2019 3:13 PM, Kevin Rushforth wrote:
I took the "git diff" patch [5] that you uploaded yesterday, applied 
it, and verified that it is the same as what is in the 
JDK-8200758-branch branch of the sandbox. It builds and runs fine 
for me.


I ran jcheck and it found the following three files with whitespace 
errors that will need to be fixed before you push:


src/jdk.incubator.jpackage/linux/classes/jdk/incubator/jpackage/internal/PackageProperty.java:49: 
Trailing whitespace
src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/ToolProviderFactory.java:35: 
Trailing whitespace
test/jdk/tools/jpackage/share/AddLauncherBase.java:137: Trailing 
whitespace


The second of these will go away with the fix for JDK-8234402 [6], 
so you don't need to do anything there. Once the fix for JDK-8234402 
is pushed to sandbox, I presume you will update the webrev, right?


Pending the fix for JDK-8234402 and the needed white-space fixes, 
this is a +1 from me (although I am not a jdk Project Reviewer, so 
you will need at least one review from someone who is).


-- Kevin

[5] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-10.git.patch
[6] https://bugs.openjdk.java.net/browse/JDK-8234402


On 11/13/2019 4:23 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 since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.


The latest EA (Build 14-jpackage+1-49 ) is posted at [4].

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


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

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

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

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

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

2019-11-26 Thread Erik Joelsson


(adding build-dev)

Build changes look good.

/Erik

On 2019-11-20 09:37, Andy Herrick wrote:
I posted new composite webrev [7], and git patch [8] after pushing fix 
for JDK-8234402 [6].


[7] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/

[8] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-11.git.patch

/Andy

On 11/19/2019 3:13 PM, Kevin Rushforth wrote:
I took the "git diff" patch [5] that you uploaded yesterday, applied 
it, and verified that it is the same as what is in the 
JDK-8200758-branch branch of the sandbox. It builds and runs fine for 
me.


I ran jcheck and it found the following three files with whitespace 
errors that will need to be fixed before you push:


src/jdk.incubator.jpackage/linux/classes/jdk/incubator/jpackage/internal/PackageProperty.java:49: 
Trailing whitespace
src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/ToolProviderFactory.java:35: 
Trailing whitespace
test/jdk/tools/jpackage/share/AddLauncherBase.java:137: Trailing 
whitespace


The second of these will go away with the fix for JDK-8234402 [6], so 
you don't need to do anything there. Once the fix for JDK-8234402 is 
pushed to sandbox, I presume you will update the webrev, right?


Pending the fix for JDK-8234402 and the needed white-space fixes, 
this is a +1 from me (although I am not a jdk Project Reviewer, so 
you will need at least one review from someone who is).


-- Kevin

[5] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-10.git.patch
[6] https://bugs.openjdk.java.net/browse/JDK-8234402


On 11/13/2019 4:23 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 since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.


The latest EA (Build 14-jpackage+1-49 ) is posted at [4].

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


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

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

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

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

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

2019-11-26 Thread Andy Herrick

Forwarding review thread to build-dev.

/Andy

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

I posted new composite webrev [7], and git patch [8] after pushing fix 
for JDK-8234402 [6].

[7] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/

[8] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-11.git.patch

/Andy

On 11/19/2019 3:13 PM, Kevin Rushforth wrote:
I took the "git diff" patch [5] that you uploaded yesterday, applied 
it, and verified that it is the same as what is in the 
JDK-8200758-branch branch of the sandbox. It builds and runs fine for me.

I ran jcheck and it found the following three files with whitespace 
errors that will need to be fixed before you push:

src/jdk.incubator.jpackage/linux/classes/jdk/incubator/jpackage/internal/PackageProperty.java:49: 
Trailing whitespace
src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/ToolProviderFactory.java:35: 
Trailing whitespace
test/jdk/tools/jpackage/share/AddLauncherBase.java:137: Trailing 
whitespace

The second of these will go away with the fix for JDK-8234402 [6], so 
you don't need to do anything there. Once the fix for JDK-8234402 is 
pushed to sandbox, I presume you will update the webrev, right?

Pending the fix for JDK-8234402 and the needed white-space fixes, this 
is a +1 from me (although I am not a jdk Project Reviewer, so you will 
need at least one review from someone who is).

-- Kevin

[5] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-10.git.patch
[6] https://bugs.openjdk.java.net/browse/JDK-8234402

On 11/13/2019 4:23 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 since EA-06 (Build 
13-jpackage+1-49 ) up to the current point.

The latest EA (Build 14-jpackage+1-49 ) is posted at [4].

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

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

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

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

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

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-...@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-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-...@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 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-...@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 Andy Herrick

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-...@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 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-...@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

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





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-...@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-...@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-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-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-...@openjdk.java.net

/Andy Herrick

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 Roger Riggs


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.

$.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 Andy Herrick


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

2018-11-13 Thread Andy Herrick



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

2018-11-13 Thread Andy Herrick

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-...@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 Philip Race


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-...@openjdk.java.net

/Andy Herrick

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

2018-11-12 Thread Philip Race




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-...@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-...@openjdk.java.net

/Andy Herrick

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

2018-11-12 Thread Philip Race


  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-...@openjdk.java.net

/Andy Herrick

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

2018-11-12 Thread Philip Race


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-...@openjdk.java.net

/Andy Herrick

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

2018-11-12 Thread Philip Race


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-...@openjdk.java.net

/Andy Herrick

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

2018-11-11 Thread Andy Herrick

yes of the nine build system issues brought up by Magnus and Erik 
(listed in JDK-8212936 ) , only one has been addressed so far.


This is at the top of the list of remaining issues we will be working ont:

The following additional issues are targeted to be address in the next few 
weeks:
JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
...

I may need some expert help  to implement some of these changes.

/Andy

On 11/11/2018 3:02 AM, Alan Bateman wrote:

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


I see Andy has a separate issue in JIRA tracking this:
  https://bugs.openjdk.java.net/browse/JDK-8212936

but I agree the bug management around this feature isn't clear to 
potential reviewers. My guess is he may be using JIRA issues to track 
feedback/changes between each iteration of the webrev so this is why 
affects and fixVersion are set to "internal". I think you need to make 
clear if the make file issues need to be addressed for the initial 
push - I suspect they do.


-Alan

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

2018-11-11 Thread Alan Bateman


On 10/11/2018 21:59, Magnus Ihse Bursie wrote:

I can't see that you've addressed any of the build system changes Erik and I 
requested? Or is this just a preliminary review, and you intend to go at least 
one more round before attempting to push? If so, this was not very clear to me.


I see Andy has a separate issue in JIRA tracking this:
  https://bugs.openjdk.java.net/browse/JDK-8212936

but I agree the bug management around this feature isn't clear to 
potential reviewers. My guess is he may be using JIRA issues to track 
feedback/changes between each iteration of the webrev so this is why 
affects and fixVersion are set to "internal". I think you need to make 
clear if the make file issues need to be addressed for the initial push 
- I suspect they do.


-Alan

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

2018-11-10 Thread Magnus Ihse Bursie

I can't see that you've addressed any of the build system changes Erik and I 
requested? Or is this just a preliminary review, and you intend to go at least 
one more round before attempting to push? If so, this was not very clear to me. 

/Magnus

> 9 nov. 2018 kl. 23:30 skrev Andy Herrick :
> 
> 
> 
>> 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
>> 
>> Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/
>> 
>> please send feedback to core-libs-...@openjdk.java.net
>> 
>> /Andy Herrick
>

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

2018-11-09 Thread Andy Herrick





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

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

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

/Andy Herrick

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

2018-11-01 Thread Magnus Ihse Bursie





On 2018-10-24 19:18, Erik Joelsson wrote:

Hello,

Nice to see this finally happening!

Are we actually adding a new demo? I thought we were working towards 
getting rid of the demos completely.


CompileJavaModules.gmk:

The jdk.packager_CLEAN_FILES could be replaced with a simple 
"jdk.packager_CLEAN := .properties" unless you actually need to add 
all the platform specific files to builds for all platforms (which I 
doubt?). To clarify, the current patch will add all linux, windows and 
mac properties to builds of all OSes. Is that intended?


Modules.gmk:

I would rather have the filter be conditional on an inclusive list of 
platforms. Other OpenJDK contributors are building for other OSes like 
AIX and we don't want to have to keep track of all those. The list of 
OSes that jpackager supports is well defined, so better check for 
those explicitly.


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

I believe the qualified export needs to be put in a 
module-info.java.extra file since we filter out the target module on 
unsupported platforms.


Launcher-jdk.packager.gmk:

* Please use $$(call FindExecutableDirForModule, $$(MODULE)) for the 
DEST dir in the copy.
* Please adjust the indentation of the big windows/else blocks. We 
indent everything with 2 spaces in conditional blocks.
* A minor style nit on the copy-and-chmod macro, since it's meant to 
be used as a recipe, we usually indent those with tabs to indicate 
that they are recipe lines, in this case, one tab is enough even 
though the surrounding define should be indented with 2 spaces (don't 
combine tab and spaces here).
* The (new) SetupJdkExecutable macro handles VERSIONINFO_RESOURCE 
automatically for you unless you have specific needs. This looks like 
the defaults should be fine.
* Since this is just for windows, the TOOLCHAIN_LINK_CXX should not 
make any difference. (It's only needed for GCC to force linking with 
g++ instead of gcc) So please remove.

* You could consider using FindSrcDirsForComponent for the src dir.

Lib-jdk.packager.gmk:

* The native source files are not organized according to the standards 
introduced with JEP 201. If they were, most of these SetupJdkLibrary 
calls would automatically find the correct sources. I realize there is 
a special case for the windows papplauncherc executable as it's built 
from the same sources as papplauncher, so that will need a special 
case. Building of the executables should be moved to 
Launcher-jdk.packager.gmk however.
* Why are you changing the build output dir and copying debuginfo 
files around? We have a standard way of building and places where 
files are put. If that is not working for you, we need to fix it on a 
global level. Having each native library being built differently is 
not good for maintainability.
* VERSIONINFO_RESOURCE is handled automatically so does not need to be 
specified.
* Please indent the full contents of logic blocks with 2 spaces. Not 
having proper indents makes it much harder to read the code.
* Several lines are too long for future side by side comparisons, 
please break them up. You can use the # lines as a soft guidance.


On top of Erik's comments, I also have the following to add from a build 
perspective:


* In make/CompileJavaModules.gmk:
Do you really need to use GENERATE_JDKBYTECODE_NOWARNINGS? When you add 
new code to OpenJDK, it really *should* be able to build without 
warnings. This has previously only been used for legacy code that has 
not yet been brought up to OpenJDK standards. Introducing new code with 
warnings silenced feels like a step in the wrong direction.


* In make/launcher/Launcher-jdk.packager.gmk:
The setup of BUILD_JPACKAGEREXE is only done for windows, but you have 
"DISABLED_WARNINGS_gcc := unused-result implicit-fallthrough". This does 
not make sense.


The CFLAGS listed look redundant. At the very least I know that -nologo 
is already present in CXXFLAGS_JDKEXE; I believe the same goes for the 
rest (or most of the rest) of the flags. Please only add flags if you 
really need them, since all special configuration/combinations is a 
potential cause for trouble in the future. This same applies to LDFLAGS.


* In src/jdk.packager/unix/scripts/jpackager:

This file  resides in a non-standard directory. We have a small list of 
directories that are supposed to come after the $MODULE/share|$OS/ part 
of the path, and "scripts" is not one of them. While there is no rule 
"forbidding" new kinds of directories, I strongly recommend against this.


Looking more closely at the file, I wonder if you really need it? It's 
sole purpose seems to be to launch java -m 
jdk.packager/jdk.packager.main.Main. For that, we have a much better 
solution. Just change make/launcher/Launcher-jdk.packager.gmk to include 
the following contents:


$(eval $(call SetupBuildLauncher, jpackager, \
    MAIN_CLASS := jdk.packager.main.Main, \
))

It will create a "jpackager" binary. Which works for Windows too, so 
maybe you won't even

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

2018-10-30 Thread Andy Herrick





On 10/30/2018 10:11 AM, Andy Herrick wrote:



On 10/24/2018 10:22 AM, Alan Bateman wrote:

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



jpackager is a simple packaging tool, based on the JavaFX 
|javapackager| tool, that:


 * Supports native packaging formats to give the end user a natural
   installation experience. These formats include |msi| and |exe| on
   Windows, |pkg| and |dmg| on MacOS, and |deb| and |rpm| on Linux.
 * Allows launch-time parameters to be specified at packaging time.
 * Can be invoked directly, from the command line, or programmatically,
   via the |ToolProvider| API.

Webrev:

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


cc'ing build-dev as it's important to get it reviewed there.

What is the plan for tests to go with this tool? I see there is one 
test in the webrev to do some argument validation but I don't see 
anything else right now.
We plan to incorporate the initial feedback from this review, and 
include an initial set of automated tests in a refresh sometime next 
week.

We will continue to develop and automate tests for future updates.


What is the status of the JNLPConverter tool? I see it is included as 
a "demo" but maybe it would be better to host somewhere else as this 
is for developers migrating Java Web Start applications.

Our current plan is to deliver it only as a demo.
After further discussion, we have decided to remove the JNLPConversion 
tool , and find another home for it.


/Andy



Would it be possible to update the JEP with all the CLI options? That 
would be useful for review and also useful for those invoking it with 
the ToolProvider API.

Done.


If I read the webrev correctly then it adds two modules, one with the 
jpackager tool and the other with an API. It would be useful to get a 
bit more information on the split. Also I think the name of the API 
module and the package that it exports needs discussion to make sure 
that the right names are chosen.
Yes - though we are currently using jdk.packager.services, we are open 
to other suggestions as the name for these. "jdk.packager.runtime" has 
also been suggested.


-Alan

/Andy

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

2018-10-30 Thread Kevin Rushforth


inline

On 10/30/2018 9:09 AM, Alan Bateman wrote:

On 30/10/2018 14:11, Andy Herrick wrote:


:


If I read the webrev correctly then it adds two modules, one with 
the jpackager tool and the other with an API. It would be useful to 
get a bit more information on the split. Also I think the name of 
the API module and the package that it exports needs discussion to 
make sure that the right names are chosen.
Yes - though we are currently using jdk.packager.services, we are 
open to other suggestions as the name for these. 
"jdk.packager.runtime" has also been suggested.
Alex has suggested jdk.jpackager to avoid giving the impression that 
it's the "JDK packager". Also several existing tool modules have the 
tool name in the module name (jdk.jdeps, jdk.jlink, jdk.jshell, ...).


Yes, I think jdk.jpackager is a fine name for this module.

Is the API to ensure that only one instance of the application is 
running really tied to the jpackager tool? Could it be used by 
applications that aren't packaged in with this tool? I'm asking in 
case there is a better name for this API module.


In its current form it is tied to jpackager. Andy might be able to 
comment on how tightly-coupled it is, and what it would take to 
generalize it, but that wasn't a goal to make it usable for apps that 
aren't packaged using jpackager. Other things that will likely go into 
this runtime support module in the future:


* Service daemon support
* User JVM args support

-- Kevin

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

2018-10-30 Thread Alan Bateman


On 30/10/2018 14:11, Andy Herrick wrote:

:


What is the status of the JNLPConverter tool? I see it is included as 
a "demo" but maybe it would be better to host somewhere else as this 
is for developers migrating Java Web Start applications.

Our current plan is to deliver it only as a demo.
This looks like a migration tool rather than a demo so not clear to me 
that this is the right approach. Also as the tool is for migration from 
Java Web Start when maybe it should be a tool hosted with the Oracle JDK 
download rather than something to put into the JDK.




:


If I read the webrev correctly then it adds two modules, one with the 
jpackager tool and the other with an API. It would be useful to get a 
bit more information on the split. Also I think the name of the API 
module and the package that it exports needs discussion to make sure 
that the right names are chosen.
Yes - though we are currently using jdk.packager.services, we are open 
to other suggestions as the name for these. "jdk.packager.runtime" has 
also been suggested.
Alex has suggested jdk.jpackager to avoid giving the impression that 
it's the "JDK packager". Also several existing tool modules have the 
tool name in the module name (jdk.jdeps, jdk.jlink, jdk.jshell, ...).


Is the API to ensure that only one instance of the application is 
running really tied to the jpackager tool? Could it be used by 
applications that aren't packaged in with this tool? I'm asking in case 
there is a better name for this API module.


-Alan

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

2018-10-30 Thread Andy Herrick





On 10/24/2018 10:22 AM, Alan Bateman wrote:

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



jpackager is a simple packaging tool, based on the JavaFX 
|javapackager| tool, that:


 * Supports native packaging formats to give the end user a natural
   installation experience. These formats include |msi| and |exe| on
   Windows, |pkg| and |dmg| on MacOS, and |deb| and |rpm| on Linux.
 * Allows launch-time parameters to be specified at packaging time.
 * Can be invoked directly, from the command line, or programmatically,
   via the |ToolProvider| API.

Webrev:

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


cc'ing build-dev as it's important to get it reviewed there.

What is the plan for tests to go with this tool? I see there is one 
test in the webrev to do some argument validation but I don't see 
anything else right now.
We plan to incorporate the initial feedback from this review, and 
include an initial set of automated tests in a refresh sometime next week.

We will continue to develop and automate tests for future updates.


What is the status of the JNLPConverter tool? I see it is included as 
a "demo" but maybe it would be better to host somewhere else as this 
is for developers migrating Java Web Start applications.

Our current plan is to deliver it only as a demo.


Would it be possible to update the JEP with all the CLI options? That 
would be useful for review and also useful for those invoking it with 
the ToolProvider API.

Done.


If I read the webrev correctly then it adds two modules, one with the 
jpackager tool and the other with an API. It would be useful to get a 
bit more information on the split. Also I think the name of the API 
module and the package that it exports needs discussion to make sure 
that the right names are chosen.
Yes - though we are currently using jdk.packager.services, we are open 
to other suggestions as the name for these. "jdk.packager.runtime" has 
also been suggested.


-Alan

/Andy

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

2018-10-25 Thread Kevin Rushforth

Andy added the a comment [1] to the JEP with the command line options.
I'll format it and add it to the JEP itself soon, but until then you can
see it in the comments to help you review it.

The tests will come shortly (Andy can comment on the state of this).
They will be a mix of automated tests and (especially for the
installers) manual tests.

The module with the API is meant to be a small runtime module that would
be jlinked in with the application (whereas the tool itself typically
wouldn't be). The working name is jdk.packager.services (inherited from
javapacakger). An alternative name we had considered was
jdk.packager.runtime? Or maybe jdk.packager.support? Do you have any
suggestions?

The exported package is named jdk.packager.services.singleton, since the
singleton app feature is the only one that currently needs runtime
support. The intention would be to add other packages as new features
that require runtime support are added in the future (e.g., daemon
services or JRE argument support). We haven't given any thought to
alternative names, other than if we change the module name to something
like jdk.packager.runtime, we would likely want to use that to inform
the name of the package. Do you have any recommendations?

Perhaps we can discuss the JNLPConverter demo in a separate thread,
since I see Erik raised a similar question?

Andy can reply to anything I missed or to clarify further.

-- Kevin

[1]
https://bugs.openjdk.java.net/browse/JDK-8200758?focusedCommentId=14217780=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14217780

On 10/24/2018 7:22 AM, Alan Bateman wrote:

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

jpackager is a simple packaging tool, based on the JavaFX
|javapackager| tool, that:

* Supports native packaging formats to give the end user a natural
installation experience. These formats include |msi| and |exe| on
Windows, |pkg| and |dmg| on MacOS, and |deb| and |rpm| on Linux.
* Allows launch-time parameters to be specified at packaging time.
* Can be invoked directly, from the command line, or programmatically,
via the |ToolProvider| API.

Webrev:

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

cc'ing build-dev as it's important to get it reviewed there.

What is the plan for tests to go with this tool? I see there is one
test in the webrev to do some argument validation but I don't see
anything else right now.

What is the status of the JNLPConverter tool? I see it is included as
a "demo" but maybe it would be better to host somewhere else as this
is for developers migrating Java Web Start applications.

Would it be possible to update the JEP with all the CLI options? That
would be useful for review and also useful for those invoking it with
the ToolProvider API.

If I read the webrev correctly then it adds two modules, one with the
jpackager tool and the other with an API. It would be useful to get a
bit more information on the split. Also I think the name of the API
module and the package that it exports needs discussion to make sure
that the right names are chosen.

-Alan

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

2018-10-24 Thread Erik Joelsson


Hello,

Nice to see this finally happening!

Are we actually adding a new demo? I thought we were working towards 
getting rid of the demos completely.


CompileJavaModules.gmk:

The jdk.packager_CLEAN_FILES could be replaced with a simple 
"jdk.packager_CLEAN := .properties" unless you actually need to add all 
the platform specific files to builds for all platforms (which I 
doubt?). To clarify, the current patch will add all linux, windows and 
mac properties to builds of all OSes. Is that intended?


Modules.gmk:

I would rather have the filter be conditional on an inclusive list of 
platforms. Other OpenJDK contributors are building for other OSes like 
AIX and we don't want to have to keep track of all those. The list of 
OSes that jpackager supports is well defined, so better check for those 
explicitly.


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

I believe the qualified export needs to be put in a 
module-info.java.extra file since we filter out the target module on 
unsupported platforms.


Launcher-jdk.packager.gmk:

* Please use $$(call FindExecutableDirForModule, $$(MODULE)) for the 
DEST dir in the copy.
* Please adjust the indentation of the big windows/else blocks. We 
indent everything with 2 spaces in conditional blocks.
* A minor style nit on the copy-and-chmod macro, since it's meant to be 
used as a recipe, we usually indent those with tabs to indicate that 
they are recipe lines, in this case, one tab is enough even though the 
surrounding define should be indented with 2 spaces (don't combine tab 
and spaces here).
* The (new) SetupJdkExecutable macro handles VERSIONINFO_RESOURCE 
automatically for you unless you have specific needs. This looks like 
the defaults should be fine.
* Since this is just for windows, the TOOLCHAIN_LINK_CXX should not make 
any difference. (It's only needed for GCC to force linking with g++ 
instead of gcc) So please remove.

* You could consider using FindSrcDirsForComponent for the src dir.

Lib-jdk.packager.gmk:

* The native source files are not organized according to the standards 
introduced with JEP 201. If they were, most of these SetupJdkLibrary 
calls would automatically find the correct sources. I realize there is a 
special case for the windows papplauncherc executable as it's built from 
the same sources as papplauncher, so that will need a special case. 
Building of the executables should be moved to Launcher-jdk.packager.gmk 
however.
* Why are you changing the build output dir and copying debuginfo files 
around? We have a standard way of building and places where files are 
put. If that is not working for you, we need to fix it on a global 
level. Having each native library being built differently is not good 
for maintainability.
* VERSIONINFO_RESOURCE is handled automatically so does not need to be 
specified.
* Please indent the full contents of logic blocks with 2 spaces. Not 
having proper indents makes it much harder to read the code.
* Several lines are too long for future side by side comparisons, please 
break them up. You can use the # lines as a soft guidance.


/Erik



On 2018-10-24 07:22, Alan Bateman wrote:

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



jpackager is a simple packaging tool, based on the JavaFX 
|javapackager| tool, that:


 * Supports native packaging formats to give the end user a natural
   installation experience. These formats include |msi| and |exe| on
   Windows, |pkg| and |dmg| on MacOS, and |deb| and |rpm| on Linux.
 * Allows launch-time parameters to be specified at packaging time.
 * Can be invoked directly, from the command line, or programmatically,
   via the |ToolProvider| API.

Webrev:

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


cc'ing build-dev as it's important to get it reviewed there.

What is the plan for tests to go with this tool? I see there is one 
test in the webrev to do some argument validation but I don't see 
anything else right now.


What is the status of the JNLPConverter tool? I see it is included as 
a "demo" but maybe it would be better to host somewhere else as this 
is for developers migrating Java Web Start applications.


Would it be possible to update the JEP with all the CLI options? That 
would be useful for review and also useful for those invoking it with 
the ToolProvider API.


If I read the webrev correctly then it adds two modules, one with the 
jpackager tool and the other with an API. It would be useful to get a 
bit more information on the split. Also I think the name of the API 
module and the package that it exports needs discussion to make sure 
that the right names are chosen.


-Alan

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

2018-10-24 Thread Alan Bateman


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



jpackager is a simple packaging tool, based on the JavaFX 
|javapackager| tool, that:


 * Supports native packaging formats to give the end user a natural
   installation experience. These formats include |msi| and |exe| on
   Windows, |pkg| and |dmg| on MacOS, and |deb| and |rpm| on Linux.
 * Allows launch-time parameters to be specified at packaging time.
 * Can be invoked directly, from the command line, or programmatically,
   via the |ToolProvider| API.

Webrev:

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


cc'ing build-dev as it's important to get it reviewed there.

What is the plan for tests to go with this tool? I see there is one test 
in the webrev to do some argument validation but I don't see anything 
else right now.


What is the status of the JNLPConverter tool? I see it is included as a 
"demo" but maybe it would be better to host somewhere else as this is 
for developers migrating Java Web Start applications.


Would it be possible to update the JEP with all the CLI options? That 
would be useful for review and also useful for those invoking it with 
the ToolProvider API.


If I read the webrev correctly then it adds two modules, one with the 
jpackager tool and the other with an API. It would be useful to get a 
bit more information on the split. Also I think the name of the API 
module and the package that it exports needs discussion to make sure 
that the right names are chosen.


-Alan

50 matches

Mail list logo