On 2014-10-22 23:35, Mandy Chung wrote:
On 10/21/2014 6:43 AM, Claes Redestad wrote:
http://cr.openjdk.java.net/~redestad/8060130/webrev.07/
Looks good.
Thanks!
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.
We need to consider the case where JarInputStream.getManifest() returns
null, which would mean we'd either end up no simpler on line 639 or
simply adding a similar check around the value returned from
jis.getManifest(). I also think I saw that referencing the static
EMPTY_PACKAGES from within the privileged anonymous class adds synthetic
access methods...
More importantly, the current approach should already ensure any file is
only scanned once by virtue of always setting manifest to a non-null
value in the end. No?
GetSystemPackage.java test
This is great. Thanks for adding the test.
Thanks!
Can you break the long lines such as the call to
JarBuilder.addClassFile and runSubprocess.
Sure. Cleaned it up a bit.
Have you run this on windows? I think you need to use File.separator
rather than "/" in the -Xbootclasspath/p argument.
Yes, I've run the test via JPRT and verified it gets picked up and run
using the default testset, so Windows, Solaris, Mac and Linux should be
covered.
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.
I've added some sanity checks as suggested. Sadly, it seems that since
the application classloader will define and load package2 first in this
test, there'll always be a Package defined in the ClassLoader.pkgs that
will mask the Package instance in Package.pkgs when retrieved via the
public methods in Package. If JDK-8061804 is resolved, we could change
to check for identity in the Package.getPackages() case, which would
improve the specificness of the test... For now, perhaps we could trick
things in this test and put our dummy class into java.lang in our test
here and ensure that the package retrieved is identical. Worth the hassle?
In the verifyPackage, it throws Exception. You can simply throw
RuntimeException that would avoid the need of the checked exception.
Sure. I've restructured a bit to keep line lengths in check.
Nit: line 35-37, 43-46 - no need to declare these import as
java.lang.* are imported by default.
Some IDEs...
Copyright year should be 2014.
Fixed.
http://cr.openjdk.java.net/~redestad/8060130/webrev.08
/Claes
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
