On Fri, 21 Jan 2022 16:23:25 GMT, Jonathan Gibbons <j...@openjdk.org> wrote:
>> Please review a javadoc update to support a new `--date` option to support >> reproducible builds. >> >> This pull request supersedes https://github.com/openjdk/jdk/pull/6905. In >> that PR, the `SOURCE_DATE_EPOCH` environment variable was used to provide >> the time stamp, but review feedback suggested the use of a new command-line >> option, `--date`. The format of the argument of the `--date` option is that >> same as that of similar options for the _jar_ and _jmod_ tools, and the code >> to handle the value is based on code in the _jar_ tool. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > change allowable date range to within 10 years of the time of use Generally looks good, but I have a few questions and found a lone typo. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlConfiguration.java line 171: > 169: > 170: /** > 171: * TRhe build date, to be recorded in generated files. Typo: TRhe src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/standard.properties line 433: > 431: doclet.usage.date.description=\ > 432: Specifies the value to be used to timestamp the generated\n\ > 433: pages, in ISO_ZONED_DATE_TIME format I guess it is technically correct to use a `DateTimeFormatter` constant name here, but wouldn't it be better form to use a generic term like "ISO-8601 format"? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java line 359: > 357: private static final ZonedDateTime now = > ZonedDateTime.now(); > 358: static final ZonedDateTime DATE_MIN = > now.minusYears(10); > 359: static final ZonedDateTime DATE_MAX = > now.plusYears(10); What's the purpose of limiting valid dates this way? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java line 370: > 368: try { > 369: date = ZonedDateTime.parse(arg, > DateTimeFormatter.ISO_ZONED_DATE_TIME) > 370: .withZoneSameInstant(ZoneOffset.UTC); I notice that the new option takes a date-time with timezone, but the timezone is discarded here (and the date-time is later converted to a java.util.Date). Since the purpose of this change is to enable reproducible builds, I guess it should be possible to define the timezone used in the "Generated by" comment (which as far as I can tell is the timezone of the local host). ------------- PR: https://git.openjdk.java.net/jdk/pull/7171
