Hello Andrew,
On 30/11/21 7:32 pm, Andrew Leonard wrote:
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.
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/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/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 {@code date and time fields} of the zip file
* entry and encoded in standard {@code MS-DOS date and time format}.
* The {@link 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/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
