>> 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.
