On Mon, 29 Nov 2021 16:22:30 GMT, Andrew Leonard <aleon...@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 <anleo...@redhat.com> > > Andrew Leonard has updated the pull request incrementally with two additional > commits since the last revision: > > - 8276766: Enable jar and jmod to produce deterministic timestamped content > > Signed-off-by: Andrew Leonard <anleo...@redhat.com> > - 8276766: Enable jar and jmod to produce deterministic timestamped content > > Signed-off-by: Andrew Leonard <anleo...@redhat.com> Thanks for taking a look Jaikiran, I concur with the above. I'm currently looking to make the rest of jar/jmod deterministic as there are several currentTimeMillis() used... i'll probably update this to be at least consistent with the others. Thanks Andrew On Tue, Nov 30, 2021 at 3:02 PM mlbridge[bot] ***@***.***> wrote: > *Mailing list message from Jaikiran Pai ***@***.***> on > core-libs-dev ***@***.***>:* > > Hello Andrew, > > On 30/11/21 7:32 pm, Andrew Leonard wrote: > > On Mon, 29 Nov 2021 16:22:30 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. > > However, it looks like this behavior to not set extended mtime within the > xdostime range has just been changed by a recent PR: https://github.com/ > /pull/5486 <https://github.com/openjdk/jdk/pull/5486>/files which has > changed jartool.Main using zipEntry.setTime(time) to use > zipEntry.setLastModifiedTime(time), the later sets mtime always regardless > of xdostime. > > When I changed the implementation in sun.tools.jar.Main to use > setLastModifiedTime(FileTime) instead of the previous setTime(long), I > hadn't paid close attention to the implementation of these methods in > java.util.zip.ZipEntry. Now that you brought this up, I had a more > closer look at their implementation. > > So it looks like the implementation of setTime(long) conditionally sets > the extended timestamp fields in optional extra data of the zip entry. > That condition checks to see if the time value being set is before 1980 > or after 2099 and if it passes this condition, it goes and sets the > extended timestamp fields. So in practice, for jar creation/updation in > sun.tools.jar.Main, the previous code using setTime(long) wouldn't have > set the extended timestamp fields, because the current year(s) aren't > before 1980 or after 2099. The implementation of > java.util.zip.ZipEntry.setLastModifiedTime(FileTime) on the other hand > has no such conditional checks and (as noted in the javadoc of that > method) goes ahead and sets the time value in the extended timestamp > fields of that zip entry. > > So yes, the change that went in to > https://github.com//pull/5486 <https://github.com/openjdk/jdk/pull/5486> > did introduce this subtle > change in the generated zip/jar entries. Having said that, the > setTime(long) on java.util.zip.ZipEntry makes no mention of these > conditional checks. It currently states: > > ???? * Sets the last modification time of the entry. > ???? * > ???? * <p> If the entry is output to a ZIP file or ZIP file formatted > ???? * output stream the last modification time set by this method will > ???? * be stored into the ***@***.*** date and time fields} of the zip file > ???? * entry and encoded in standard ***@***.*** MS-DOS date and time format}. > ???? * The ***@***.*** java.util.TimeZone#getDefault() default TimeZone} is > ???? * used to convert the epoch time to the MS-DOS data and time. > > So it doesn't even make a mention of setting extended timestamp fields, > let along setting them on a particular condition. So I'm unsure whether > switching to setLastModifiedTime(FileTime) instead of setTime(long) was > a bad thing. > > The implications of https://bugs.openjdk.java.net/browse/JDK-8073497 > might also apply to FileTime unnecessary initialization slowing VM startup, > however if FileTime is already regularly referenced during startup, then it > wont.. If this is the case then way forward (1) would be ok... > @AlanBateman was that an intentional change? @jaikiran > > I had a look at that JBS issue now. From what I understand in its > description and goal, the idea was to prevent initialization of > java.util.Date utilities very early on during the startup. I had a quick > look at the code in ZipEntry and how it behaves when it constructs a > ZipEntry instance out of the zip file data. With the change in > https://github.com//pull/5486 <https://github.com/openjdk/jdk/pull/5486>, > the "mtime" (which represents > extended timestamp field) will be set in the zip file entry, so there > would be an attempt to create a FileTime out of it. The call that does > that appears to be "unixTimeToFileTime" in ZipEntry and as far as I can > see it doesn't end up using any java.util.Date classes. Of course, I > haven't yet looked or experimented to verify and be absolutely sure > about it, but from a cursory check it doesn't look like this would > contribute to pulling in java.util.Date utilities. > > -Jaikiran > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <https://github.com/openjdk/jdk/pull/6481#issuecomment-982719481>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AHQDDN7UQMX4PV46ZZCALE3UOTRRJANCNFSM5IMSJY4Q> > . > Triage notifications on the go with GitHub Mobile for iOS > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> > or Android > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. > > ------------- PR: https://git.openjdk.java.net/jdk/pull/6481
