new webrev addressing issues below: http://cr.openjdk.java.net/~sdrach/8153654/webrev.09/ <http://cr.openjdk.java.net/~sdrach/8153654/webrev.09/>
>> 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. The product team has decided not to move forward on those two issues for now, they will remain unresolved. > > 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). fixed > 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. I’m not sure what you mean. > > 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). done > > versionedStream method may be better to be moved to VersionHelper. done > > 442 return (version == jf.getVersion().major() && vStr.length() > offset + > 1) > > This version only selects the entries in the base section and versioned > section. I thought that would work, at least I had a scenario in my head where it worked ;-) But it doesn’t so I changed it. > > 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 One can not put new public classes in the versioned section, so that layout is not legal > > 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. All the existing test input source files do not have the copyright header. These little classes are just there to feed the test that does have the required copyright > > 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. That’s not an existing property, so I left it where it is with all the multi-release properties > > MultiReleaseException.java > 47 public MultiReleaseException(String[] err) { > > It would be better to have a key parameter: MultiReleaseException(String key, > String… param) done > VersionHelper.java > 40 public static String get(String origin) { > > s/origin/classname to make the param clearer. done > 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. I did that but I think it’s more confusing > >> 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. See my earlier comment > > Mandy
