Sorry, forget to Cc core-libs on my response to Peter’s questions
> Begin forwarded message:
>
> From: Steve Drach <[email protected]>
> Subject: Re: RFR 8163798: Add a versionedStream method to JarFile
> Date: August 26, 2016 at 9:44:39 AM PDT
> To: Peter Levart <[email protected]>
>
> Hi Peter,
>
>> Javadoc for the new method mentions public and non-public classes:
>>
>> "@{code JarEntries}
>> * containing non-public classes in the
>> * versioned directory that corresponds to the version returned by
>> * {@link JarFile#getVersion()} are included in the stream because they are
>> * typically accessed by the public classes."
>>
>> Why?
>
> The public “interface” for a multi-release jar is the set of public or
> protected classes in the base (or root) section of the jar. By definition,
> no new (i.e. without a corresponding base class) public or protected classes
> can be contained in a versioned directory. Versioned directories can contain
> new package-private classes. In practice these package-private classes are
> dependencies for the public/protected classes in
> in the same versioned directory.
>
>> The logic of multi-release jar files is indifferent towards the access
>> attributes of class files. It never parses the class file to interpret the
>> access attributes of the containing class.
>
> I know. I thought about doing that but rejected it as too heavyweight and
> just decided to use the heuristic that real class files have the suffix
> “.class” and that if there was not a corresponding base class, then the class
> has to be package-private according to the “rules” of multi-release jar
> files. The jar tool enforces these rules by actually parsing the class files.
>
>> The class-level javadoc of JarFile does mention "public classes", but I view
>> this just as a recommendation how they should be constructed.
>>
>> The logic of multi-release jar files is indifferent towards the types of
>> resources contained in the jar entries too, as far as I can see. Your logic
>> in versionedStream() is not. The proposed new method is the only place in
>> JarFile and ZipFile where the check for ".class" suffix of the entry name is
>> performed. Why this special treatement of class files?
>
> I hope the explanation above is sufficient. The versionedStream looks at the
> jar file in an inverted or backwards way. In the usual case, a
> multi-release jar file entry is obtained by
> JarFile.getJarEntry(base-entry-name). In this case if the jar file was
> opened for version 10, we’d not be able to see or get package-private classes
> in the version 9 directory. Oh, one thing I should mention, Mr. Jar’s can be
> sparsely populated in the versioned directories, So if a version 10 public
> class was not in the version 10 directory, but was in the version 9
> directory, then the version 9 one would be returned.
>
>> I also don't understand why the versioned stream should contain the
>> untranslated base versioned directories. For example, the following entries
>> for the v10 stream in the test:
>>
>> "META-INF/versions/9/"
>> "META-INF/versions/10/“
>
> I just did it because they are returned by JarFile::stream. I’m ambivalent
> about it, so I wanted to see the comments if any.
>
>>
>> All other resources and sub-directories in the META-INF/versions/XX/...
>> directories are "normalized" (meaning that the versioned prefix is stripped
>> from the names in the returned entries). Normalizing above directories would
>> translate them to "root" directory, which is never included as an entry of a
>> normal jar or zip file. So I think they could simply be skipped in the
>> resulting versioned stream. Couldn't they?
>
> Yes.
>
>>
>> Considering all of the above, I think versionedStream() method could be much
>> simpler. For example, something like the following:
>>
>>
>> public Stream<JarEntry> versionedStream() {
>> return
>> !isMultiRelease()
>> ? stream()
>> : stream()
>> .flatMap(je -> {
>> String name = je.getName();
>> if (name.startsWith(META_INF_VERSIONS)) {
>> // versioned entry
>> if (name.length() > META_INF_VERSIONS.length()) {
>> // potentially real entry
>> String vStr =
>> name.substring(META_INF_VERSIONS.length());
>> int offset = vStr.indexOf('/');
>> int version = 0;
>> if (offset >= 0) try {
>> version = Integer.parseInt(vStr.substring(0,
>> offset));
>> } catch (NumberFormatException e) { /* ignore */ }
>> if (version <= 0) {
>> throw new RuntimeException(
>> "Can't obtain version from multi-release
>> jar entry: "
>> + name);
>> }
>> return (version <= versionMajor && vStr.length() >
>> offset + 1)
>> // normalized name of real entry
>> ? Stream.of(vStr.substring(offset + 1))
>> // version > versionMajor or
>> // META-INF/versions/<version>/ dir -
>> ignored
>> : Stream.empty();
>> } else {
>> // META-INF/versions/ dir - ignored
>> return Stream.empty();
>> }
>> } else { // base entry
>> return Stream.of(name);
>> }
>> })
>> .distinct()
>> .map(this::getJarEntry);
>> }
>>
>>
>> What do you think?
>
> It’s certainly more sophisticated than the way I did it. If we could
> constrain the getJarEntry only to base names we might be able to handle the
> package-private classes too. I need to think about it a bit.
>
> Steve
>
>>
>> Regards, Peter
>>
>>
>> On 08/26/2016 07:16 AM, Tagir Valeev wrote:
>>> Hello!
>>>
>>> Small nitpick:
>>>
>>> versionsMap.keySet().forEach(v -> {
>>> Stream<String> names = versionsMap.get(v).stream().map(nm -> nm.name);
>>> if (v == versionMajor) {
>>> // add all entries of the version we are interested in
>>> finalNames.addAll(names.collect(Collectors.toSet()));
>>> } else {
>>> // add resource entries found in lower versioned directories
>>> finalNames.addAll(names.filter(nm -> nm.endsWith(".class") ? false
>>> : true)
>>> .collect(Collectors.toSet()));
>>> }
>>> });
>>>
>>> I suggest to replace with
>>>
>>> versionsMap.forEach((v, list) -> {
>>> Stream<String> names = list.stream().map(nm -> nm.name);
>>> if (v != versionMajor) {
>>> names = names.filter(nm -> !nm.endsWith(".class"));
>>> }
>>> names.forEach(finalNames::add);
>>> });
>>>
>>> This is cleaner and somewhat faster to use Map.forEach as you don't need to
>>> lookup every map entry.
>>>
>>> Also I don't see why "nm -> nm.endsWith(".class") ? false : true" could be
>>> better than
>>> "nm -> !nm.endsWith(".class")". Probably a matter of style though.
>>>
>>> Finally there's no need to collect into intermediate set just to add this
>>> set into finalNames.
>>> Instead you can drain the stream directly to finalNames via forEach.
>>>
>>> Probably it should be explicitly noted in spec that the resulting stream is
>>> unordered (or at least may
>>> be unordered) as it's created from the Set.
>>>
>>> With best regards,
>>> Tagir Valeev
>>>
>>> On Fri, Aug 26, 2016 at 2:30 AM, Steve Drach <[email protected]> wrote:
>>>
>>>> Hi,
>>>>
>>>> Please review this changeset that adds a versionedStream method to JarFile.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.00/ <
>>>> http://cr.openjdk.java.net/~sdrach/8163798/webrev.00/>
>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8163798 <
>>>> https://bugs.openjdk.java.net/browse/JDK-8163798>
>>>>
>>>> Thanks
>>>> Steve
>>>>
>>>>
>>>>
>>
>
