RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package

[Mandy Chung]

On 10/21/2014 6:43 AM, Claes Redestad wrote:
>
> http://cr.openjdk.java.net/~redestad/8060130/webrev.07/

Looks good.

A minor nit: Package.java line 636:   it can return EMPTY_MANIFEST that 
will set manifest to non-null so that an invalid file would avoid 
opening the file every time getManifest is called (although this case 
rarely happens).  line 639 can then be simplified.

GetSystemPackage.java test
    This is great.  Thanks for adding the test.

Can you break the long lines such as the call to JarBuilder.addClassFile 
and runSubprocess.  Have you run this on windows?  I think you need to 
use File.separator rather than "/" in the -Xbootclasspath/p argument.

This test uses a special class loader that delegates to null class 
loader only.  Since you have the verification in place, it'd be good to 
also call Package.getPackage and Package.getPackages to verify that the 
same "package2" instance is returned.  As a sanity check, you could 
check "java.lang" package must be present.

In the verifyPackage, it throws Exception.  You can simply throw 
RuntimeException that would avoid the need of the checked exception.  
Nit: line 35-37, 43-46 - no need to declare these import as java.lang.* 
are imported by default.

Copyright year should be 2014.

>
>
> I'd prefer sticking to the double-checked idiom here, unless you insist.
>
> I've cleaned up the code to avoid assignment in if-clauses, which 
> according to
> anonymous sources makes the code more readable. Perhaps this addresses
> some of your concerns?
>

Looks okay.  Initialize manifest to EMPTY_MANIFEST would clean that a 
little.

>
> Since I don't want to add binary JAR files, I opted to add a test 
> which creates two JAR files,
> each with a single class (with/without manifest) and then proceeds to 
> spawn processes
> to verify that:
>

Thanks for adding the test.

> - when the JAR with manifest is on bootclasspath, the package can be 
> found via
> Package.getSystemPackage and the package object reflects values added 
> to the manifest
> - when the JAR without manifest is on bootclaspath, the package can be 
> found via
> Package.getSystemPackage but is empty apart from name
> - adding the test.classes directory to bootclasspath behaves the same 
> as adding the JAR
> without manifest
> - for any case where the class/package is not on the bootclasspath, 
> the package information
> can not be found via Package.getSystemPackage().
>
> Does this cover everything?
>

This is great.   See my comment above.

> I guess there might be a way to make @run main/othervm or 
> main/bootclasspath pick
> up a dynamically generated JAR file, but I've failed to find a way 
> that would make this work without
> pregenerating the JARs. Suggestions on how this can be simplified are 
> welcome.
>

What you have is fine.   The other way is to do it in a shell test that 
is not preferable as you would have to manage FS for different OSes etc.

Mandy