Fix plugin timezone dependency

[nielsbasjes]

This is a proposed fix for the reproducibility problem described here
#34424

[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.

[pivotal-cla]

@nielsbasjes Thank you for signing the Contributor License Agreement!

[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

[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?

[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.

[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.

[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.

[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.

[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.

[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.

[nielsbasjes]

Yes that is true.
Perhaps marking this plugin as not threadsafe?

[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 ...

[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 998d59b.

Thanks for raising the issue and providing the analysis.