The latest web revs. Answers to questions in-line below: http://cr.openjdk.java.net/~sdrach/8153654/webrev.10/ <http://cr.openjdk.java.net/~sdrach/8153654/webrev.10/>
http://cr.openjdk.java.net/~sdrach/8153654/webrev.jdk/ <http://cr.openjdk.java.net/~sdrach/8153654/webrev.jdk/> >>> >>> 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. VersionedStream is now in jdk.internal.util.jar > > >>> >>> 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. I put an assert in. > >> >>> >>> 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 If I make Bar and Gee non-public, I can build a jar with them in it. See the second test I added in test.tools.jdep.MultireleaseJar > > 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. jar tool won’t validate a q.Bar as a public 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. >> >> 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. Done. > >> >>> >>> 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. Okay. Unfortunately we lose a better error message. > >> >>> 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 >
