RFR: 8153654: Update jdeps to be multi-release jar aware
Mandy Chung
mandy.chung at oracle.com
Mon Aug 29 23:59:41 UTC 2016
Hi Steve,
> On Aug 29, 2016, at 12:43 PM, Steve Drach <steve.drach at oracle.com> wrote:
>
> Hi,
>
> Please review the following two changesets that enables jdeps to use multi-release jar files. The output from the tool shows versioned dependencies by prefixing them with “version #/“ where version # is 9, 10, etc.
>
> webrevs: http://cr.openjdk.java.net/~sdrach/8153654/webrev.08/
This looks quite good. JDK-8163798 and JDK-8164665 will define public APIs to get the versioned entries and real name which I think are useful for tools. It’s fine to proceed with this change and update jdeps to use the public APIs when available.
This patch parse MR-JAR only if —-multi-release option is specified. It would also be useful to analyze all versioned entries for the default option (i.e. analyze direct dependencies from class files) that can be done in a separate RFE.
Comments to this patch:
ClassFileReader.java
386 if (Integer.parseInt(v) < 9) {
387 String[] msg = {"err.multirelease.jar.malformed",
388 jarfile.getName(), rn
389 };
390 throw new MultiReleaseException(msg);
391 }
To get here, I can think of the case when it’s not a MRJAR (so JarFile::stream returns all entries). If it’s a MR-JAR, the JarFile will be opened with a valid version. versionedStream should only return the proper versioned entries. Maybe you should emit warning (add it to skippedEntries if this happens) or make it an assert.
382 if (rn.startsWith("META-INF/versions/")) {
383 int n = rn.indexOf('/', 18); // "META-INF/versions/".length()
421 final String META_INF_VERSIONS = "META-INF/versions/“;
429 if (name.length() > META_INF_VERSIONS.length()) {
They both extract the version of this entry. This can be refactored to add a method like VersionHelper::getVersion(JarEntry).
versionedStream method may be better to be moved to VersionHelper.
442 return (version == jf.getVersion().major() && vStr.length() > offset + 1)
This version only selects the entries in the base section and versioned section.
Can you add the following scenario in the new test and Bar and Gee are public and shows the result when -—multi-release 9 or 10 or base?
p/Foo.class
META-INF/versions/9/p/Foo.class
META-INF/versions/9/q/Bar.class
META-INF/versions/10/q/Bar.class
META-INF/versions/10/q/Gee.class
JDK-8163798 would cover more scenarios in depth. I’m okay to use the versionedStream you have and leave that to JDK-8163798. BTW the copyright header is missing in the new tests.
JdepsTask.java
401 throw new BadArgs("err.multirelease.option.illegal", arg);
You can simply use err.invalid.arg.for.option which I think is simple and adequate.
MultiReleaseException.java
47 public MultiReleaseException(String[] err) {
It would be better to have a key parameter: MultiReleaseException(String key, String… param).
VersionHelper.java
40 public static String get(String origin) {
s/origin/classname to make the param clearer.
jdeps.properties: what about a shorter version:
--multi-release <version> Specifies the version when processing
multi-release jar files. <version> can be
>= 9 or base.
> http://cr.openjdk.java.net/~sdrach/8153654/webrev.jdk/
The use of shared secrets is just temporary and I hope it will soon be replaced with a public API when JDK-8164665 is resolved.
Mandy
More information about the core-libs-dev
mailing list