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/