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
