Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generated JAR file depends on default time zone #222

Closed
ppkarwasz opened this issue Dec 16, 2023 · 10 comments · Fixed by #230
Closed

Generated JAR file depends on default time zone #222

ppkarwasz opened this issue Dec 16, 2023 · 10 comments · Fixed by #230
Labels
bug help wanted released Issue has been released

Comments

@ppkarwasz
Copy link

When Moditect adds elements to an existing JAR file, it uses ZipFileSystem instead of the older ZipOutputStream. Unfortunately this has a big drawback:

  • JARs generated on systems that only differ by the setting of the default time zone are not identical.

The reason behind this is due to JDK internals:

Reproduction

To reproduce this problem use any recent project that uses Moditect, e.g. apache/commons-logging and build it using two different time zones.

On my Debian system with JDK 17 I obtain:

piotr@bialykiel:~/workspace/commons-logging$ git checkout rel/commons-logging-1.3.0
...
piotr@bialykiel:~/workspace/commons-logging$ export TZ=Europe/Warsaw
piotr@bialykiel:~/workspace/commons-logging$ mvn clean package
...
piotr@bialykiel:~/workspace/commons-logging$ sha256sum target/commons-logging-1.3.0.jar 
8590cef7d2810aef40334472696e11e3c0c97d6230a3eacaf685e74dc982d5ae  target/commons-logging-1.3.0.jar
piotr@bialykiel:~/workspace/commons-logging$ export TZ=America/Chicago
piotr@bialykiel:~/workspace/commons-logging$ mvn clean package
...
piotr@bialykiel:~/workspace/commons-logging$ sha256sum target/commons-logging-1.3.0.jar 
63ed3b7e92347ffe895d58ab72484af700c132e26206d6a4811d48d9f5c9462d  target/commons-logging-1.3.0.jar

The only difference is in the DOS time stamp of the entries modified by Moditect.

@garydgregory
Copy link

TY @ppkarwasz
As a maintainer of Apache Commons, I look forward to a fix and new release!

ppkarwasz added a commit to ppkarwasz/reproducible-central that referenced this issue Dec 18, 2023
The `commons-logging` artifacts version 1.3.0 are fully reproducible
except:

 * the SPDX descriptor.

Due to moditect/moditect#222 the timezone is one of the variables to
take into consideration. In this case it is `America/New_York`.

**Remark**: all the artifacts that use `commons-parent` version 65 (the
first one with Moditect version 1.1.0) should be reproducible too.
hboutemy pushed a commit to jvm-repo-rebuild/reproducible-central that referenced this issue Dec 19, 2023
The `commons-logging` artifacts version 1.3.0 are fully reproducible
except:

 * the SPDX descriptor.

Due to moditect/moditect#222 the timezone is one of the variables to
take into consideration. In this case it is `America/New_York`.

**Remark**: all the artifacts that use `commons-parent` version 65 (the
first one with Moditect version 1.1.0) should be reproducible too.
@hboutemy
Copy link
Contributor

@ppkarwasz I suppose this is an issue that should be fixed by JDK: do you know if it was reported there?

@ppkarwasz
Copy link
Author

@hboutemy, it looks like a regression from JDK-7012868, but I always found submitting bugs against the JDK very frustrating (no way to answer JDK dev comments).

Remark that there is another subtle difference between the original JAR entries and those generated by Moditect: the new entries have an extended timestamp extension and looking at the JDK code, there is no workaround for that either.

@hboutemy
Copy link
Contributor

workaround found for equivalent issue in maven-shade-plugin apache/maven-shade-plugin#179

in the case of moditect, it would be nice to disable extra fields (created/accessed/modified time) usage instead of trying to update f3e629b
notice that applying the TZ (like shade) to the update timestamp should do the job also

@aalmiray
Copy link
Contributor

aalmiray commented Feb 8, 2024

@hboutemy what would be the fix here? MSHADE-420 reads the time from an existing entry. What we need is to write back a given time.

@ppkarwasz
Copy link
Author

ppkarwasz commented Feb 8, 2024

@aalmiray,

This is a hard one. Unless I am mistaken, as long you use Java's Zip File System:

  • the entries of files you modify will either have an NTFS or ExtendedTimestamp extension,
  • either way each entry will have a modification timestamp in UTC and a legacy DOS modification time in your local timezone.

Since even in Java 21 the Zip File System does not have any property to change the timezone just for this file system (cf. documentation) and setting the JVM's default timezone in the plugin is not an acceptable solution, IMHO the add-module-info goal must be rewritten to use ZipFile instead. Yes, that is a big patch.

Until then either users must run Maven with an UTC timezone (which is an acceptable option for reproducibility) or they must use the generate-module-info goal.

@aalmiray
Copy link
Contributor

aalmiray commented Feb 8, 2024

Gotcha. I suppose rewriting to use ZipFile would be the way to go.

aalmiray added a commit to aalmiray/moditect that referenced this issue Feb 8, 2024
@garydgregory
Copy link

There is also Conmons Compress.

@aalmiray
Copy link
Contributor

aalmiray commented Feb 8, 2024

@garydgregory Indeed, but we'd prefer to keep dependencies at a minimum, which means rolling our sleeves and use JSL APIs, even if it means writing more code 😓

aalmiray added a commit to aalmiray/moditect that referenced this issue Feb 9, 2024
aalmiray added a commit to aalmiray/moditect that referenced this issue Feb 9, 2024
aalmiray added a commit that referenced this issue Feb 11, 2024
@github-actions github-actions bot added the released Issue has been released label Mar 13, 2024
Copy link

🎉 This issue has been resolved in 1.2.0.Final (Release Notes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help wanted released Issue has been released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants