RFR 8020802: Need an ability to create jar files that are invariant to the pack200 packing/unpacking

[Kumar Srinivasan]

Approved.

Kumar

On 10/22/2013 9:08 AM, Alexander Zuev wrote:
> Kumar,
>
>   new version of webrev where i have fixed all the coding style 
> glitches i've found is here:
> http://cr.openjdk.java.net/~kizune/8020802/webrev.04
>
> /Alex
>
> On 10/22/13 19:32, Kumar Srinivasan wrote:
>>
>> Hi Alex,
>>
>>>   thanks for suggestion, dt.jar seems to be a fine test subject.
>>> Fixed @test field and the .jar used in the test.
>>
>> I am sorry! I misled you yesterday, it should simply be @test
>>
>>> New webrev is here: http://cr.openjdk.java.net/~kizune/8020802/webrev.03
>>
>> Here are some nits, mostly conventions.
>>
>> Main.java
>> Suggestion breaking this line:
>> 204                 String tmpbase = (fname == null) ? "tmpjar" : fname.substring(fname.indexOf(File.separatorChar) + 1);
>>
>> This makes it clearer
>> Ref: 
>> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-136091.html#248
>> String tmpbase = (fname == null)
>> 	?"tmpjar"
>> 	:fname.substring(fname.indexOf(File.separatorChar) + 1);
>> Can tmpbase be made final ?
>>
>> TestNormal.java
>>
>> I don't like the way this line is broken up,  break after the Paren, 
>> usually it is a comma.
>> or use a static final for Line.separator, that might make it shorter.
>>   
>>   66         Process proc = Runtime.getRuntime()
>>   67                 .exec(java_home + File.separator + "bin" + File.separator + cmd);
>>
>> There are other coding convention issues wrt. if/else and while, 
>> please check.
>> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449
>>
>> Thanks
>> Kumar
>>
>>
>>>
>>> /Alex
>>>
>>> On 10/22/13 24:33, Kumar Srinivasan wrote:
>>>> Alex,
>>>>
>>>> The @test should be just TestNormal not TestNormal.java
>>>>
>>>> if you need an alternate/small jar you can use one in $JDK/lib/
>>>> ex: jconsole.jar or dt.jar
>>>>
>>>> Kumar
>>>>
>>>>
>>>> On 10/21/2013 7:41 AM, Alexander Zuev wrote:
>>>>> Alan,
>>>>>
>>>>>   thanks for a review, see my comments inline.
>>>>>
>>>>> On 10/20/13 23:15, Alan Bateman wrote:
>>>>>> On 19/10/2013 16:14, Kumar Srinivasan wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> This looks good.
>>>>>>>
>>>>>>> Hi Sherman, Alan,
>>>>>>>
>>>>>>> Could one of you review this, please
>>>>>>>
>>>>>> The "-n" options seems okay but I wonder if there has any thought 
>>>>>> given to having an option on jarsigner to normalize it before 
>>>>>> signing?
>>>>> That was requirement for PM - looks like they have more usages for 
>>>>> this option than just signing a jar file.
>>>>>> On the patch itself then I think it needs to be cleaned up before 
>>>>>> pushing it. From a quick look:
>>>>>>
>>>>>> - It's using a raw type for the packer properties, should be 
>>>>>> Map<String, String>
>>>>> Ok, will fix that.
>>>>>> - Is any reason not to use try-with-resources?
>>>>> Because not all of the resources used are inherited from 
>>>>> AutoCloseable - you can't use File
>>>>> in try-with-resources block so to use it i will have to rewrite 
>>>>> code a bit. Looks like it's a little bit too late
>>>>> considering i will need to test it on all the platforms.
>>>>>> - I assume the catching of Exception in createTemporaryFile 
>>>>>> should be IOException|SecurityException
>>>>> Yes, will change that.
>>>>>> - Is the @test tag in the test right?
>>>>> You mean that @summary does not match the issue summary and does 
>>>>> not include bug ID? Can be fixed too.
>>>>>> - The test appears to extract jfr.jar but I don't think we can 
>>>>>> assume this exists
>>>>> I'm trying to find a .jar file that is in the jdk distribution, 
>>>>> contains class files and of a reasonable size - attempt with
>>>>> tools.jar caused test to timeout on Windows machine even with 
>>>>> 20minutes timeout - and i do not want to make a test
>>>>> with the timeout greater than even 10 minutes - much less one of 
>>>>> 20+minutes.
>>>>> Any suggestions? I'm putting the jfr.jar so far but can change it 
>>>>> later - test stabilization can be done after the Oct 24th.
>>>>>> - Some of the jar tests use sun.tools.jar.Main and this might be 
>>>>>> useful here
>>>>> I'm trying to check the command-line option and want it to go over 
>>>>> the full path of command-line option parsing - just to be sure.
>>>>> Do not think it's a big deal.
>>>>>> One other thing, Mathias's mail to jdk8-dev [1] asks for changes 
>>>>>> that require translation to be in the master no longer than 
>>>>>> 10/24. Does the update to jar.properties count?
>>>>> Yes, i've been pinged by the documentation team already so i'm 
>>>>> trying to push this stuff as soon as possible.
>>>>> Of course without giving up on the quality of the code.
>>>>>
>>>>> The new webrev can be found at 
>>>>> http://cr.openjdk.java.net/~kizune/8020802/webrev.02
>>>>>
>>>>> /Alex
>>>>>>
>>>>>> -Alan
>>>>>>
>>>>>> [1] 
>>>>>> http://mail.openjdk.java.net/pipermail/jdk8-dev/2013-October/003443.html
>>>>>
>>>>
>>>
>>
>