RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v21]

[Lance Andersen]

On Thu, 9 Dec 2021 18:07:58 GMT, Andrew Leonard <aleonard at openjdk.org> wrote:

>> Add a new --source-date <TIMESTAMP> (epoch seconds) option to jar and jmod to allow specification of time to use for created/updated jar/jmod entries. This then allows the ability to make the content deterministic.
>> 
>> Signed-off-by: Andrew Leonard <anleonar at redhat.com>
>
> Andrew Leonard has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8276766: Enable jar and jmod to produce deterministic timestamped content
>   
>   Signed-off-by: Andrew Leonard <anleonar at redhat.com>

Hi Andrew,

Thank you for splitting out the test into its own file and addressing the test issues we found.

Overall looks good.  I have suggested some additional changes so the test is slightly more TestNG friendly and a few places where it would be useful to add comments

test/jdk/tools/jar/ReproducibleJar.java line 30:

> 28:  *          reproducible
> 29:  * @modules jdk.jartool
> 30:  * @run testng ReproducibleJar

Probably use othervm given you are toggling the TZ for extra safety

test/jdk/tools/jar/ReproducibleJar.java line 49:

> 47: import java.time.LocalDateTime;
> 48: import java.time.ZonedDateTime;
> 49: import java.time.ZoneId;

Duplicate import?  Perhaps get rid to the import of line 43

test/jdk/tools/jar/ReproducibleJar.java line 74:

> 72:     private static final File jarFileSourceDate1 = new File("JarEntryTimeSourceDate1.jar");
> 73:     private static final File jarFileSourceDate2 = new File("JarEntryTimeSourceDate2.jar");
> 74: 

Please consider using caps for your static final variable names

test/jdk/tools/jar/ReproducibleJar.java line 87:

> 85:                                 "2098-02-18T00:00:00-08:00",
> 86:                                 "2099-12-31T23:59:59+00:00"};
> 87: 

I would suggest converting to a DataProvider for the test.

test/jdk/tools/jar/ReproducibleJar.java line 95:

> 93:                                  "2006-04-06T12:38:00",
> 94:                                  "2012-08-24T16"};
> 95: 

Another good use of a DataProvider

test/jdk/tools/jar/ReproducibleJar.java line 102:

> 100:         jarFileSourceDate1.delete();
> 101:         jarFileSourceDate2.delete();
> 102:         TimeZone.setDefault(TZ);

I believe you could just call runAfter() from within this method

test/jdk/tools/jar/ReproducibleJar.java line 114:

> 112:     }
> 113: 
> 114:     @Test

Please add a comment introducing the intent of the test.  As mentioned above, please consider using a DataProvider vs. an array with the test.

test/jdk/tools/jar/ReproducibleJar.java line 120:

> 118:         // Test --date source date
> 119:         for (String sourceDate : sourceDates) {
> 120:             createOuterInnerDirs(dirOuter, dirInner);

If you use a DataProvider,  you could call this method from your runBeforeMethod

test/jdk/tools/jar/ReproducibleJar.java line 147:

> 145:             cleanup(dirInner);
> 146:             cleanup(dirOuter);
> 147:             jarFileSourceDate1.delete();

Is the above needed given your  `@AfterMethod`

test/jdk/tools/jar/ReproducibleJar.java line 152:

> 150: 
> 151:     @Test
> 152:     public void testInvalidSourceDate() throws Throwable {

Please add a basic comment of the intent of the method and consider using a DataProvider

test/jdk/tools/jar/ReproducibleJar.java line 165:

> 163: 
> 164:     @Test
> 165:     public void testJarsReproducible() throws Throwable {

Please add a basic comment of the intent of the method and consider using a DataProvider

test/jdk/tools/jar/ReproducibleJar.java line 193:

> 191: 
> 192:             // Check jars are identical
> 193:             checkSameContent(jarFileSourceDate1, jarFileSourceDate2);

You could if you choose use:

 Assert.assertEquals(Files.readAllBytes(jarFileSourceDate1.toPath(), Files.readAllBytes(jarFileSourceDate2.toPath()));

test/jdk/tools/jar/ReproducibleJar.java line 199:

> 197:             jarFileSourceDate1.delete();
> 198:             jarFileSourceDate2.delete();
> 199:         }

I believe your` @AfterMethod` will address this so the above is not needed

test/jdk/tools/jar/ReproducibleJar.java line 202:

> 200:     }
> 201: 
> 202:     static void createOuterInnerDirs(File dirOuter, File dirInner) throws Throwable {

I believe you are always passing in the same parameter values, so perhaps no need for the parameters?

test/jdk/tools/jar/ReproducibleJar.java line 208:

> 206:          *         foo.txt
> 207:          */
> 208:         Assert.assertTrue(dirOuter.mkdir());

Please move the  comment above the method

test/jdk/tools/jar/ReproducibleJar.java line 219:

> 217:     }
> 218: 
> 219:     static void checkFileTime(long now, long original) throws Throwable {

Please add a basic comment of the intent of the method

test/jdk/tools/jar/ReproducibleJar.java line 241:

> 239:     }
> 240: 
> 241:     static void checkSameContent(File f1, File f2) throws Throwable {

See comment above for consideration  regarding the use of Assert.assertEquals

test/jdk/tools/jar/ReproducibleJar.java line 249:

> 247:     }
> 248: 
> 249:     private static boolean isTimeSettingChanged() {

Please add a basic comment describing the intent of the method

test/jdk/tools/jar/ReproducibleJar.java line 260:

> 258:     }
> 259: 
> 260:     private static boolean isInTransition() {

Please add a basic comment of the intent of the method

test/jdk/tools/jar/ReproducibleJar.java line 278:

> 276:         File[] x = dir.listFiles();
> 277:         if (x != null) {
> 278:             for (int i = 0; i < x.length; i++) {

Could use enhanced for loop here if you desire

test/jdk/tools/jar/ReproducibleJar.java line 286:

> 284: 
> 285:     static void extractJar(File jarFile) throws Throwable {
> 286:         String javahome = System.getProperty("java.home");

Please add a basic comment of the intent of the method.

Any reason you chose not to use JAR_TOOL here?

-------------

Marked as reviewed by lancea (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6481