Hi Steve,
> On Aug 29, 2016, at 12:43 PM, Steve Drach <[email protected]> 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
