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