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

http://cr.openjdk.java.net/~jlaskey/8241602/webrev-02 <http://cr.openjdk.java.net/~jlaskey/8241602/webrev-02>

On May 6, 2020, at 11:05 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:

On 06/05/2020 14:42, Jim Laskey wrote:

...

:

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.

Not in the test but the JDK does use the order-resources plugin with the following pattern:

JLINK_ORDER_RESOURCES += \ /java.base/java/** \ /java.base/jdk/** \ /java.base/sun/** \ /java.base/com/** \ /jdk.localedata/** \ #

Anyway, I looked at webrev-01 and the jlink changes look good. The updated test doesn't use the jimage API so no need to open jdk.internal.jimage. The imports can be removed too, also RUNTIME is no longer used in this version. Have you decided on a test name? The rallying call is "reproducible builds" and I'd prefer to have "Reproducible" in the test name so its consistent with the existing test that checks for reproducible with user modules.

-Alan.