> On Aug 30, 2016, at 7:58 PM, Steve Drach <steve.dr...@oracle.com> wrote: > >> >> 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.
OK. What about putting these helper methods in jdk.internal.jar package as jlink and jdeps both use them to would avoid duplicated code. >> >> 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. My point is that JarFile::getEntry should not return such JarEntry - is that true? In that case, how SharedSecrets.javaUtilJarAccess().getRealName(jarfile, e) would return META-INF/versions/$n where n < 9? This is not an expected error and so InternalError (or assert) is more appropriate than throwing MultiReleaseException. > >> >> 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 But such JAR file can be created. What about adding a non-public class under: META-INF/versions/9/q/Zee.class META-INF/versions/10/q/Zee.class Keeping public q/Bar.class is okay as JarFile::getName(“q/Bar.class”) should return null if not allowed for any Runtime.Version. > >> >> 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 I put the copyright headers in all source files, including tiny test classes. > >> >> 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 err.invalid.arg.for.option is an existing property. > >> 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 User guides, man page to include examples would help that. Mandy