Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

On Tue, 30 Nov 2021 20:00:19 GMT, John Neffenger  wrote:

> > Whichever we use, we have to use e.setTimeLocal(), so can't see what the 
> > difference is?
> 
> Okay, here's a brief command-line example before posting the code example. In 
> my experience, most people trying to set up a common, project-wide build 
> timestamp use the formatted string in UTC:
> 
> ```
> $ date -d @$SOURCE_DATE_EPOCH --iso-8601=seconds --utc
> 2021-11-29T17:36:06+00:00
> ```
> 
> In JavaFX, we're using the more concise Java version of that string: 
> `2021-11-29T17:36:06Z`.
> 
> So here's what happens when you get that exact same instant in time from 
> different sources on different machines:
> 
> ```
> $ TZ=America/Nome date -d @$SOURCE_DATE_EPOCH --iso-8601=seconds
> 2021-11-29T08:36:06-09:00
> $ TZ=Europe/Rome date -d @$SOURCE_DATE_EPOCH --iso-8601=seconds
> 2021-11-29T18:36:06+01:00
> $ git log -1 --pretty=%cI
> 2021-11-29T09:36:06-08:00
> ```
> 
> All of a sudden it makes a big difference when you're discarding the time 
> zone. You end up with differences in the archive files depending on (a) the 
> time zone of the build machine, and (b) the source you use to obtain an ISO 
> 8601 string to represent that singular instant.

AH! got ya, so I was looking at the requirement from a slightly different 
perspective, ie. reproducibility is same input == same output, so to my mind:
--date "2021-11-29T08:36:06-09:00"
is a different input to:
--date "2021-11-29T09:36:06-08:00"

ie. the String is different! but as you say, the use case is actually same 
input === "same INSTANT in TIME", my mind was totally not seeing that, thank 
you!
ok all makes sense now :-)

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[John Neffenger]

On Tue, 30 Nov 2021 19:37:23 GMT, Andrew Leonard  wrote:

> Whichever we use, we have to use e.setTimeLocal(), so can't see what the 
> difference is?

Okay, here's a brief command-line example before posting the code example. In 
my experience, most people trying to set up a common, project-wide build 
timestamp use the formatted string in UTC:


$ date -d @$SOURCE_DATE_EPOCH --iso-8601=seconds --utc
2021-11-29T17:36:06+00:00


In JavaFX, we're using the more concise Java version of that string: 
`2021-11-29T17:36:06Z`.

So here's what happens when you get that exact same instant in time from 
different sources on different machines:


$ TZ=America/Nome date -d @$SOURCE_DATE_EPOCH --iso-8601=seconds
2021-11-29T08:36:06-09:00
$ TZ=Europe/Rome date -d @$SOURCE_DATE_EPOCH --iso-8601=seconds
2021-11-29T18:36:06+01:00
$ git log -1 --pretty=%cI
2021-11-29T09:36:06-08:00


All of a sudden it makes a big difference when you're discarding the time zone. 
You end up with differences in the archive files depending on (a) the time zone 
of the build machine, and (b) the source you use to obtain an ISO 8601 string 
to represent that singular instant.

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[John Neffenger]

On Tue, 30 Nov 2021 18:57:55 GMT, Andrew Leonard  wrote:

> I'm also adding --date validation of 1980->2099

I am only now catching up with you on that issue. 😄  I wrote a very short 
program that illustrates that problem (and all the other ones) very clearly for 
anyone to see. I'll post it shortly.

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

On Tue, 30 Nov 2021 19:31:37 GMT, John Neffenger  wrote:

> > Both basically truncate the timezone.
> 
> There's a difference. The first option saves a DOS date and time that depends 
> on the time zone of the build machine due to the ISO 8601 string returned by 
> default from the `git` and `date` commands, which is what everyone will be 
> using. Those commands return the local time zone offset by default, which is 
> what breaks your current implementation.

@jgneff I think you've lost me on this one so example may help me please..
Whichever we use, we have to use e.setTimeLocal(), so can't see what the 
difference is?

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[John Neffenger]

On Tue, 30 Nov 2021 18:52:56 GMT, Andrew Leonard  wrote:

> Both basically truncate the timezone.

There's a difference. The first option saves a DOS date and time that depends 
on the time zone of the build machine due to the ISO 8601 string returned by 
default from the `git` and `date` commands, which is what everyone will be 
using. Those commands return the local time zone offset by default, which is 
what breaks your current implementation.

The second option converts to a DOS date and time that is independent of the 
time zone of the build machine, as long as you stay withing the range of its 
values, as you discovered.

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

On Mon, 29 Nov 2021 16:22:30 GMT, Andrew Leonard  wrote:

>> Add a new --source-date  (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 
>
> 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 
>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>
>Signed-off-by: Andrew Leonard 

I'm also adding --date validation of 1980->2099

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

On Tue, 30 Nov 2021 16:26:28 GMT, John Neffenger  wrote:

> > So what you suggest sounds reasonable, although it would be a "behaviour 
> > change" in that whereas now if the date is between 1980->2106 only a 
> > xdostime is stored, we would now always be storing the additional "mtime" 
> > field ...
> 
> No, sorry, that is not at all what I suggested.
> 
> I am suggesting to immediately convert the value of the `--date` command-line 
> option to an `Instant`. Once you have an `Instant` object, it's just like 
> before when you had the value of `SOURCE_DATE_EPOCH` (which is in fact an 
> instant on the time line), so you can revert to using your prior revision 
> that handled that situation just perfectly!
> 
> In fact, you always want to avoid storing the addition "extended timestamp" 
> field when possible because it causes a rather surprising increase in the 
> size of the archive.
> 
> I wanted to send this right away, because you're such a fast coder, and in a 
> different time zone, but please give me some time to look over your and 
> Jaikiran's comments in more detail this morning. Thanks! 😄
@jgneff John, sorry got confused, but I see what you're saying now. So what 
you're saying is take the --date , work out a LocalDateTime for that 
"instant" but in UTC, then call e.setTimeLocal(localDateTime).
So I think i'm in fact just putting together a new commit for option (1) using 
setTimeLocal() similar to this, just very slightly different but efftecively 
doing the same thing, i'm converting the user input --date string to a 
ZonedDateTime, from which I get the LocalDateTime for calling 
setTimeLocal(zonedDateTime.toLocalDateTime()). That way when you unzip it the 
files have the same local date time.
eg.
--date="2022-03-15T14:36:00+06:00"

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[John Neffenger]

On Tue, 30 Nov 2021 12:00:59 GMT, Andrew Leonard  wrote:

> So what you suggest sounds reasonable, although it would be a "behaviour 
> change" in that whereas now if the date is between 1980->2106 only a xdostime 
> is stored, we would now always be storing the additional "mtime" field ...

No, sorry, that is not at all what I suggested.

I am suggesting to immediately convert the value of the `--date` command-line 
option to an `Instant`. Once you have an `Instant` object, it's just like 
before when you had the value of `SOURCE_DATE_EPOCH` (which is in fact an 
instant on the time line), so you can revert to using your prior revision that 
handled that situation just perfectly!

In fact, you always want to avoid storing the addition "extended timestamp" 
field when possible because it causes a rather surprising increase in the size 
of the archive.

I wanted to send this right away, because you're such a fast coder, and in a 
different time zone, but please give me some time to look over your and 
Jaikiran's comments in more detail this morning. Thanks! 😄

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

On Mon, 29 Nov 2021 16:22:30 GMT, Andrew Leonard  wrote:

>> Add a new --source-date  (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 
>
> 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 
>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>
>Signed-off-by: Andrew Leonard 

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 
> wrote:
>
> Add a new --source-date  (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 /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 
> 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.
>  *
>  *  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 ,
> 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

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

On Tue, 30 Nov 2021 08:53:03 GMT, John Neffenger  wrote:

>> @andrew-m-leonard Thank you, Andrew, for your bold solution to Mark's 
>> request -- one that even solves the problem with the current `ZipEntry` API!
>> 
>> Would you be willing to consider a minor change to your implementation?
>> 
>> As I mentioned earlier, if we are going to allow an ISO 8601 zoned date and 
>> time, we need either to **truncate, restrict, or convert** its value. The 
>> current implementation **truncates,** discarding the time zone information. 
>> The better option might be to **convert.** That is, parse the ISO 8601 
>> string with `Instant.parse`, get the seconds since the epoch with 
>> `getEpochSecond`, and then proceed as before. Treat the value of `--date` as 
>> an instant on the time line, just like `SOURCE_DATE_EPOCH`, and always store 
>> its value in UTC.
>> 
>> This also solves the problem of storing dates before 1980 as a local date 
>> and time in UTC while storing those after 1980 as a local date and time with 
>> a discarded time zone. By converting instead of truncating, the value is 
>> always stored as the local date and time in UTC regardless of whether or not 
>> it's within the range of the "MS-DOS date and time" field.
>> 
>> Those of us using the timestamp of the last commit can still get the value 
>> directly from `git`.
>> 
>> For `SOURCE_DATE_EPOCH`, run:
>> 
>> 
>> $ git log -1 --pretty=%ct
>> 1638207366
>> 
>> 
>> For the `--date` option of the `jar` and `jmod` tools, run:
>> 
>> 
>> $ git log -1 --pretty=%cI
>> 2021-11-29T09:36:06-08:00
>> 
>> 
>> Specifically, that `git` ISO 8601 string, and even the sample `date` command 
>> that Mark shows in his comment, are what break the current implementation by 
>> creating archives that depend on the time zone of the build machine again.
>> 
>> By the way, the `jar` utility displays a time zone in the output of its 
>> `--list` and `--verbose` options even when there is no time zone information 
>> in the file. (In other words, it lies, or at least embellishes.) Use the 
>> verbose output of `zipinfo` to see the true values of the timestamps in an 
>> archive.
>
>> Would you be willing to consider a minor change to your implementation?
> 
> Just to be clear, this change should let us postpone any additions to the 
> `ZipEntry` API for another day, if at all, and maybe even get this pull 
> request integrated for JDK 18.

@jgneff John, i'm going to update this PR to use the LocalDateTime setting 
option and restrict --date to 1980->2099, thus ensuring xdostime is always only 
used. This I believe we should be able to get into jdk-18.

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Jaikiran Pai]

Hello Andrew,

On 30/11/21 7:32 pm, Andrew Leonard wrote:

On Mon, 29 Nov 2021 16:22:30 GMT, Andrew Leonard  wrote:


Add a new --source-date  (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.
 *
 *  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

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

On Mon, 29 Nov 2021 16:22:30 GMT, Andrew Leonard  wrote:

>> Add a new --source-date  (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 
>
> 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 
>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>
>Signed-off-by: Andrew Leonard 

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

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

On Mon, 29 Nov 2021 16:22:30 GMT, Andrew Leonard  wrote:

>> Add a new --source-date  (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 
>
> 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 
>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>
>Signed-off-by: Andrew Leonard 

I see maybe 2 ways forward:
1) We either prove always setting an mtime(FileTime) has no impact in JVM 
startup, and is an acceptable behavior change
2) We implement the original fix in 2 stages:
a) jdk-18: setTimeLocal() change, which provides deterministic behavior over 
years 1980-2106 (which is in keeping with the theme of JDK-8073497 !)
b) jdk-19+: provide ZipEntry Zoned time setting support

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

On Tue, 30 Nov 2021 08:53:03 GMT, John Neffenger  wrote:

>> @andrew-m-leonard Thank you, Andrew, for your bold solution to Mark's 
>> request -- one that even solves the problem with the current `ZipEntry` API!
>> 
>> Would you be willing to consider a minor change to your implementation?
>> 
>> As I mentioned earlier, if we are going to allow an ISO 8601 zoned date and 
>> time, we need either to **truncate, restrict, or convert** its value. The 
>> current implementation **truncates,** discarding the time zone information. 
>> The better option might be to **convert.** That is, parse the ISO 8601 
>> string with `Instant.parse`, get the seconds since the epoch with 
>> `getEpochSecond`, and then proceed as before. Treat the value of `--date` as 
>> an instant on the time line, just like `SOURCE_DATE_EPOCH`, and always store 
>> its value in UTC.
>> 
>> This also solves the problem of storing dates before 1980 as a local date 
>> and time in UTC while storing those after 1980 as a local date and time with 
>> a discarded time zone. By converting instead of truncating, the value is 
>> always stored as the local date and time in UTC regardless of whether or not 
>> it's within the range of the "MS-DOS date and time" field.
>> 
>> Those of us using the timestamp of the last commit can still get the value 
>> directly from `git`.
>> 
>> For `SOURCE_DATE_EPOCH`, run:
>> 
>> 
>> $ git log -1 --pretty=%ct
>> 1638207366
>> 
>> 
>> For the `--date` option of the `jar` and `jmod` tools, run:
>> 
>> 
>> $ git log -1 --pretty=%cI
>> 2021-11-29T09:36:06-08:00
>> 
>> 
>> Specifically, that `git` ISO 8601 string, and even the sample `date` command 
>> that Mark shows in his comment, are what break the current implementation by 
>> creating archives that depend on the time zone of the build machine again.
>> 
>> By the way, the `jar` utility displays a time zone in the output of its 
>> `--list` and `--verbose` options even when there is no time zone information 
>> in the file. (In other words, it lies, or at least embellishes.) Use the 
>> verbose output of `zipinfo` to see the true values of the timestamps in an 
>> archive.
>
>> Would you be willing to consider a minor change to your implementation?
> 
> Just to be clear, this change should let us postpone any additions to the 
> `ZipEntry` API for another day, if at all, and maybe even get this pull 
> request integrated for JDK 18.

@jgneff Hi John, many thanks for the suggestions

So what you suggest sounds reasonable, although it would be a "behaviour 
change" in that whereas now if the date is between 1980->2106 only a xdostime 
is stored, we would now always be storing the additional "mtime" field which is 
a FileTime persisted as an int("seconds from epoch") in the zip file. I was 
trying to not change the binary semantics, but I actually think your suggestion 
makes sense.

I've done a bit of digging as to why the extended mtime is only stored outside 
the xdos date range, and found this bug: 
https://bugs.openjdk.java.net/browse/JDK-8073497
Which basically I think changed ZipEntry so it did not construct Date() objects 
from ZipEntries loaded during JVM startup, which it "implies" (although I can't 
see the evidence) improves startup performance.
There is an interesting comment here: 
https://github.com/openjdk/jdk/blame/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/java/util/zip/ZipEntry.java#L83

 * This establish an approximate high-bound value for DOS times in
 * milliseconds since epoch, used to enable an efficient but
 * sufficient bounds check to avoid generating extended last modified
 * time entries.
 *
 * Calculating the exact number is locale dependent, would require loading
 * TimeZone data eagerly, and would make little practical sense. Since DOS
 * times theoretically go to 2107 - with compatibility not guaranteed
 * after 2099 - setting this to a time that is before but near 2099
 * should be sufficient.

So if we were to always set the extended mtime field, then FileTime.from(long) 
would potentially cause the impact JDK-8073497 was trying to avoid...?
However, with this bug we seem to have not thought about deterministic 
behavior..."Calculating the exact number is locale dependent, would require 
loading TimeZone data eagerly, and would make little practical sense"

Thoughts?

@AlanBateman @LanceAndersen fyi

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[John Neffenger]

On Tue, 30 Nov 2021 08:31:54 GMT, John Neffenger  wrote:

> Would you be willing to consider a minor change to your implementation?

Just to be clear, this change should let us postpone any additions to the 
`ZipEntry` API for another day, if at all, and maybe even get this pull request 
integrated for JDK 18.

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[John Neffenger]

On Mon, 29 Nov 2021 19:27:57 GMT, Andrew Leonard  wrote:

>>> @AlanBateman yes, see above comment, thanks
>> 
>> This is a significant change to the ZipEntry API that will require 
>> discussion and agreement. Can you start a discussion on core-libs-dev about 
>> the topic? You could start with a summary of the problem and the API and 
>> non-API options that have been explored.
>
>> > > @AlanBateman yes, see above comment, thanks
>> > 
>> > 
>> > This is a significant change to the ZipEntry API that will require 
>> > discussion and agreement. Can you start a discussion on core-libs-dev 
>> > about the topic? You could start with a summary of the problem and the API 
>> > and non-API options that have been explored.
>> 
>> I agree with Alan. We are too close to RDP 1 to consider this type of change 
>> for JDK 18 as we need more time to discuss, update the CSR, test (and we 
>> will need additional tests for this), and give it time to bake. IMHO this 
>> will need to go into JDK 19 assuming we move forward with changes similar to 
>> the latest commit
> 
> Thanks @LanceAndersen , @AlanBateman, i've just posted a discussion thread 
> here: 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/084150.html

@andrew-m-leonard Thank you, Andrew, for your bold solution to Mark's request 
-- one that even solves the problem with the current `ZipEntry` API!

Would you be willing to consider a minor change to your implementation?

As I mentioned earlier, if we are going to allow an ISO 8601 zoned date and 
time, we need either to **truncate, restrict, or convert** its value. The 
current implementation **truncates,** discarding the time zone information. The 
better option might be to **convert.** That is, parse the ISO 8601 string with 
`Instant.parse`, get the seconds since the epoch with `getEpochSecond`, and 
then proceed as before. Treat the value of `--date` as an instant on the time 
line, just like `SOURCE_DATE_EPOCH`, and always store its value in UTC.

This also solves the problem of storing dates before 1980 as a local date and 
time in UTC while storing those after 1980 as a local date and time with a 
discarded time zone. By converting instead of truncating, the value is always 
stored as the local date and time in UTC regardless of whether or not it's 
within the range of the "MS-DOS date and time" field.

Those of us using the timestamp of the last commit can still get the value 
directly from `git`.

For `SOURCE_DATE_EPOCH`, run:


$ git log -1 --pretty=%ct
1638207366


For the `--date` option of the `jar` and `jmod` tools, run:


$ git log -1 --pretty=%cI
2021-11-29T09:36:06-08:00


Specifically, that `git` ISO 8601 string, and even the sample `date` command 
that Mark shows in his comment, are what break the current implementation by 
creating archives that depend on the time zone of the build machine again.

By the way, the `jar` utility displays a time zone in the output of its 
`--list` and `--verbose` options even when there is no time zone information in 
the file. (In other words, it lies, or at least embellishes.) Use the verbose 
output of `zipinfo` to see the true values of the timestamps in an archive.

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

On Mon, 29 Nov 2021 17:07:52 GMT, Alan Bateman  wrote:

>> @andrew-m-leonard I see you've add several APIs to ZipEntry in this PR. I 
>> think this part will require discussion as it's not immediately clear that 
>> we should over burden the ZipEntry API as proposed.
>
>> @AlanBateman yes, see above comment, thanks
> 
> This is a significant change to the ZipEntry API that will require discussion 
> and agreement. Can you start a discussion on core-libs-dev about the topic? 
> You could start with a summary of the problem and the API and non-API options 
> that have been explored.

> > > @AlanBateman yes, see above comment, thanks
> > 
> > 
> > This is a significant change to the ZipEntry API that will require 
> > discussion and agreement. Can you start a discussion on core-libs-dev about 
> > the topic? You could start with a summary of the problem and the API and 
> > non-API options that have been explored.
> 
> I agree with Alan. We are too close to RDP 1 to consider this type of change 
> for JDK 18 as we need more time to discuss, update the CSR, test (and we will 
> need additional tests for this), and give it time to bake. IMHO this will 
> need to go into JDK 19 assuming we move forward with changes similar to the 
> latest commit

Thanks @LanceAndersen , @AlanBateman, i've just posted a discussion thread 
here: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/084150.html

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Lance Andersen]

On Mon, 29 Nov 2021 17:07:52 GMT, Alan Bateman  wrote:

> > @AlanBateman yes, see above comment, thanks
> 
> This is a significant change to the ZipEntry API that will require discussion 
> and agreement. Can you start a discussion on core-libs-dev about the topic? 
> You could start with a summary of the problem and the API and non-API options 
> that have been explored.

I agree with Alan.  We are too close to RDP 1 to consider this type of change 
for JDK 18 as we need more time to discuss, update the CSR, test (and we will 
need additional tests for this), and give it time to bake.  IMHO this will need 
to go into JDK 19 assuming we move forward with changes similar to the latest 
commit

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[John Neffenger]

On Mon, 29 Nov 2021 16:22:30 GMT, Andrew Leonard  wrote:

>> Add a new --source-date  (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 
>
> 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 
>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>
>Signed-off-by: Andrew Leonard 

This is just a quick comment on the epoch date and time string. I'm testing 
your updates on the JavaFX builds now and will follow up with the results and 
more comments later today. Thanks, Andrew.

src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 88:

> 86: date {0} is not a valid ISO 8601 date and time
> 87: error.date.before.epoch=\
> 88: date {0} is before Epoch 1970-01-01T00:00:00

The epoch in Java is a [zoned date and time][1] (1970-01-01T00:00:00Z).

[1]: 
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Instant.html#EPOCH

-

Changes requested by jgneff (no project role).

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Alan Bateman]

On Mon, 29 Nov 2021 16:18:44 GMT, Alan Bateman  wrote:

>> 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 
>>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>>
>>Signed-off-by: Andrew Leonard 
>
> @andrew-m-leonard I see you've add several APIs to ZipEntry in this PR. I 
> think this part will require discussion as it's not immediately clear that we 
> should over burden the ZipEntry API as proposed.

> @AlanBateman yes, see above comment, thanks

This is a significant change to the ZipEntry API that will require discussion 
and agreement. Can you start a discussion on core-libs-dev about the topic? You 
could start with a summary of the problem and the API and non-API options that 
have been explored.

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

On Mon, 29 Nov 2021 16:18:44 GMT, Alan Bateman  wrote:

>> 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 
>>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>>
>>Signed-off-by: Andrew Leonard 
>
> @andrew-m-leonard I see you've add several APIs to ZipEntry in this PR. I 
> think this part will require discussion as it's not immediately clear that we 
> should over burden the ZipEntry API as proposed.

@AlanBateman yes, see above comment, thanks

-

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Alan Bateman]

On Mon, 29 Nov 2021 16:19:01 GMT, Andrew Leonard  wrote:

>> Add a new --source-date  (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 
>
> 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 
>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>
>Signed-off-by: Andrew Leonard 

@andrew-m-leonard I see you've add several APIs to ZipEntry in this PR. I think 
this part will require discussion as it's not immediately clear that we should 
over burden the ZipEntry API as proposed.

-

Changes requested by alanb (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6481

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]

[Andrew Leonard]

> Add a new --source-date  (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 

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 
 - 8276766: Enable jar and jmod to produce deterministic timestamped content
   
   Signed-off-by: Andrew Leonard 

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6481/files
  - new: https://git.openjdk.java.net/jdk/pull/6481/files/c7928bfe..6206c1ee

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6481&range=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6481&range=07-08

  Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6481.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6481/head:pull/6481

PR: https://git.openjdk.java.net/jdk/pull/6481