RFR: 8158295 Add a multi-release jar validation mechanism to jar tool

[Steve Drach]

>> Please review the following:
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/ <http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/>
>> issue: https://bugs.openjdk.java.net/browse/JDK-8158295 <https://bugs.openjdk.java.net/browse/JDK-8158295>
>> 
>> This changeset adds a multi-release jar validator to jar tool.  After the jar tool builds a multi-release jar, the potential resultant jar file is passed to the validator to assure that the jar file meets the minimal standards of a multi-release jar, in particular that versioned classes have the same api as base classes they override.  There are other checks, for example warning if two classes are identical.  If the jar file is invalid, it is not kept, so —create will not produce a jar file and —update will keep the input jar file.

I’ve updated the webrev to address the issues raised — http://cr.openjdk.java.net/~sdrach/8158295/webrev.02/index.html <http://cr.openjdk.java.net/~sdrach/8158295/webrev.02/index.html>

> 
> jar/Main.java
> 
> 284             // This code would be repeated in the create and update sections.
> 285             // In order not to do that, it's here as a consumer.
> 286             Consumer<File> validateAndClose = tmpfile -> {
> Why does it need to be Consumer rather than just a method?

I changed it to a method.

> 
> Then i think you don’t need to rethow, but you can still keep the try block and use finally for File.deleteIfExist since it will not complain for the case where you move.

Done.

> 
> 
> 558             int i1 = s1.indexOf('/', n);
> 559             int i2 = s1.indexOf('/', n);
> 560             if (i1 == -1) throw new NumberFormatException(s1);
> 561             if (i2 == -2) throw new NumberFormatException(s2);
> 
> I think you are trying to reject entries directly under the versioned area so it’s not about numbers (same in the validator, so there is some redundancy?).

A couple things here.  The most obvious is it should be “if (i2 == -1)”.  I replaced NumberFormatException with a new InvalidJarException.  I added a comment that
this code is used to sort the version numbers as string representations of numbers, therefore “9” goes before “10”, not usual for string sorts.

> 
> 
> 588             AtomicBoolean validHolder = new AtomicBoolean();
> 589             validHolder.set(valid);
> 
> Pass valid to the constructor

Done.

> 
> 
> Validator/FingerPrint.java
> 
> It would be useful to have some comments on what is checked in terms of API compatibility. e.g. describe what FingerPrint collects and how Validator uses it.

Added some commentary to FingerPrint.

> 
> 
> FingerPrint.java
> 
> 205         private final Map<String,Field> fields = new HashMap<>();
> 206         private final Map<String,Method> methods = new HashMap<>();
> 
> Not sure you need to use a Map, why not use a Set?

Not sure why I did it with Maps instead of Sets, but I wanted to keep the method and field names and maybe that made sense at one time.  It doesn’t now, so Sets they are.

> 
> Change Method to be literally a representation of a single method rather than a consolidation that is lossy for the set of exceptions.

Done.