This is a proposed fix for the reproducibility problem described here https://github.com/spring-projects/spring-boot/issues/34424
Comment From: pivotal-cla
@nielsbasjes Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Comment From: pivotal-cla
@nielsbasjes Thank you for signing the Contributor License Agreement!
Comment From: nielsbasjes
I installed this version of the plugin locally and then when I use that version in my reproduction project I actually get the right timestamps and all files (using all timezones in docker) are binary identical and the timestamp looks exact like what was configured: 2011-11-11 22:22 .
https://github.com/nielsbasjes/BugreportSpringPluginTimezone
Comment From: scottfrederick
@nielsbasjes I don't think manipulating the time zone like this is the right way to approach the fix.
Looking at the .jar.original files in your reproducing sample, the same timestamps are applied to both jars, independent of the local timezone. So Maven is doing the right thing. The code that Maven uses to parse the project.build.outputTimeStamp is here: https://github.com/apache/maven-archiver/blob/maven-archiver-3.6.0/src/main/java/org/apache/maven/archiver/MavenArchiver.java#L886-L887.
Unlike Maven, Spring Boot does not apply the withOffsetSameInstant(ZoneOffset.UTC) modifier after OffsetDateTime.parse(outputTimeStamp) when parsing the date here: https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot-tools/spring-boot-maven-plugin/src/main/java/org/springframework/boot/maven/RepackageMojo.java#L238. If applying this modifier fixes the problem, it looks like a cleaner solution to me.
Would you like to try this change instead?
Comment From: nielsbasjes
The problem is not in the parsing of the timestamp. That is done correctly. The timezone is part of the provided timestamp (in my example 'Z') and is correctly handled.
The parsed timestamp is then persisted in an instance of java.nio.file.attribute.FileTime which stores the timestamp essentially as epoch (i.e. without any timezone).
The proposed withOffsetSameInstant(ZoneOffset.UTC) does not fix this problem because this does not change the underlying epoch value which is copied into the FileTime instance.
It is then the Zip creation code (from commons-compress) that converts the epoch into something else using an instance of Calendar which is based on the system default Timezone.
As far as I can tell all the code in the Spring project correctly handles the timestamps.
Comment From: nielsbasjes
Also: It is possible to provide the desired timestamp as epoch which cannot have a timestamp and thus a timezone must be chosen. The current choice is the local system timezone, my proposal is to force (or make configurable?) this to be the UTC timezone.
Comment From: philwebb
I wonder if we can change this logic in JarWriter.writeToArchive(...) so that jarEntry.setLastModifiedTime changes this.lastModifiedTime based on Calendar.getInstance(). That way we write an offset time with the knowledge that ZipUtil.toDosTime will change it back again.
Comment From: nielsbasjes
@philwebb So you shift the epoch timestamp using the current timezone, because you expect the underlying code to shift it back again? I would vote against such code.
Comment From: philwebb
Yeah. The reason I prefer it is because TimeZone.setDefault(...) with change the default timezone for the entire JVM. If Maven is doing something else in another thread (now or in the future) then the default timezone could shift under its feet.
Comment From: scottfrederick
TimeZone.setDefault(...)with change the default timezone for the entire JVM
That was my concern too, that this could have unpredictable side-effects that are difficult to diagnose.
Comment From: nielsbasjes
Yes that is true. Perhaps marking this plugin as not threadsafe?
Comment From: nielsbasjes
I'll have a look tomorrow if I can make this nasty time shifting work reliable. Speaking of timezones .. it is already tomorrow over here ...
Comment From: philwebb
I discovered the javadoc for setTime is much better than setLastModifiedTime and actually describes that the default TimeZone is used when writing values. I've switched to this method and gone with the shift approach. The fix is in commit 998d59b7ac1a75b26634e4fd2843a7833e554840.
Thanks for raising the issue and providing the analysis.