Thank you. Notes below.
updated webrev: http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00 <http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00> > On May 6, 2020, at 4:14 AM, Alan Bateman <alan.bate...@oracle.com> wrote: > > On 05/05/2020 20:56, Jim Laskey wrote: >> This fix addresses the inconsistent ordering by jimage content by jlink from >> run to run. Bottom line, the implementer was using HashSet without defining >> hashcode/equals for the Set entry classes. >> >> webrev: http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00 >> <http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00> >> jbs: https://bugs.openjdk.java.net/browse/JDK-8241602 >> <https://bugs.openjdk.java.net/browse/JDK-8241602> >> > DirArchive and JmodArchive look okay. They could use Objects.hash/equals but > what you have is okay too. Can you re-check JarArchive as it is created with > a runtime version so equals should be checking 3 fields. Opted to use Objects. Added version for JarArchive (not convinced needed but should be benign.) > > The existing test for reproducible builds is JLinkReproducibleTest. Adding a > new test is okay too but we should at least try to keep the names consistent. > A couple of suggestions for the test in the webrev: > - the @modules tag needs to include java.desktop as it is needed by the test. > Better still would be to change to java.se so that there are more modules in > the images. Switched to java.se, only a second slower than java.desktop . Overall better test than JLinkReproducibleTest for increased likelihood of variance. > - Did you mean to open jdk.tools.jlink.internal? Maybe its a leftover from a > previous iteration of the test? Nope, Yep. > - You can use Files.mismatch to compare the lib/modules files are identical > (like JLinkReproducibleTest). It's okay to use the jimage ImageReaderFactory > to check the names tables too but I think it's more important to check that > the lib/modules files are identical before probing further. I had it in my mind there was a timestamp in the file, but then I remembered there wasn't. Switched to Files.mismatch. Everything else is redundant. Aside: The order in the file is still somewhat scattered, based on archive and archive content. We have the a pattern based sorting plugin which we don't use and we never did any locality vs performance testing. > > -Alan
