> On Aug 30, 2016, at 7:58 PM, Steve Drach <[email protected]> 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
