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
