RFR: JDK-8233270: Add support to jtreg helpers to unpack packages

[Phil Race]

On 12/6/19 12:51 PM, Alexey Semenyuk wrote:
>
>
> On 12/5/2019 8:44 PM, Alexander Matveev wrote:
>> Hi Alexey,
>>
>> 1) Remove this file:
>> test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JavaTool.java.rej
> Done. webrev updated in place.
>
>> 2) Agree with Phil, they probably should be pushed as two separate bugs.
> Decoupling is not straightforward unfortunately: tests that cover 
> additional launchers functionality contain fixes for both [1] and [2] 
> CRs.

So the synopsis reads like this is all about updating tests, and your 
email says
"go read the bugs" and nothing more.
I think most people like to see a description of the problem and the 
proposed solution
written in the review request.

I think the source code updates are what you need to highlight.
And I don't think you have explained why it is so hard.
Saying is is hard is not the same thing as explaining why.

I think you should decouple unless you can provide that explanation.
And definitely you need to provide explanation of the fix etc.

-phil.

> If there would be no strong arguments against pushing the fixes in a 
> single patch, I'd rather avoid decoupling.
>
>> 3) Do you know how to run installer tests with new changes? It is not 
>> clear from code.
> Fix for [1] moved code to install packages produced by jpackage from 
> manage_packages.sh script into jtreg helper classes.
> If you run tests locally with test_jpackage.sh script, nothing will 
> change for you.
>
> - Alexey
>>
>> Changes itself looks fine.
>>
>> Thanks,
>> Alexander
>>
>> On 12/5/2019 5:33 PM, Philip Race wrote:
>>> I don't understand the relationship between these two bugs.
>>>
>>> -phil
>>>
>>> On 12/5/19, 2:47 PM, Alexey Semenyuk wrote:
>>>> Please review  fixes for [1] and [2]. Both target jpackage tool.
>>>>
>>>> The webrev is at [3].
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8233270
>>>>
>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8230933
>>>>
>>>> [3] http://cr.openjdk.java.net/~asemenyuk/8233270/webrev.00/
>>>>
>>
>