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

Tue Dec 3 20:11:13 UTC 2019

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.
>>>>>>
>>>>>> +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-dev at 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/
>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>>
>