On Fri, 10 Sep 2021 15:51:30 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> The commit in this PR implements the proposal for enhancement that was >> discussed in the core-libs-dev mailing list recently[1], for >> https://bugs.openjdk.java.net/browse/JDK-8231640 >> >> At a high level - the `store()` APIs in `Properties` have been modified to >> now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env >> variable is set, then instead of writing out the current date time as a date >> comment, the `store()` APIs instead will use the value set for this env >> variable to parse it to a `Date` and write out the string form of such a >> date. The implementation here uses the `d MMM yyyy HH:mm:ss 'GMT'` date >> format and `Locale.ROOT` to format and write out such a date. This should >> provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. >> Furthermore, intentionally, no changes in the date format of the "current >> date" have been done. >> >> These modified `store()` APIs work in the presence of the `SecurityManager` >> too. The caller is expected to have a read permission on the >> `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that >> permission, then the implementation of these `store()` APIs will write out >> the "current date" and will ignore any value that has been set for the >> `SOURCE_DATE_EPOCH` env variable. This should allow for backward >> compatibility of existing applications, where, when they run under a >> `SecurityManager` and perhaps with an existing restrictive policy file, the >> presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the >> `store()` APIs. >> >> The modified `store()` APIs will also ignore any value for >> `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such >> cases, the `store()` APIs will write out the "current date" and ignore the >> value set for this environment variable. No exceptions will be thrown for >> such invalid values. This is an additional backward compatibility precaution >> to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking >> applications. >> >> An additional change in the implementation of these `store()` APIs and >> unrelated to the date comment, is that these APIs will now write out the >> property keys in a deterministic order. The keys will be written out in the >> natural ordering as specified by `java.lang.String#compareTo()` API. >> >> The combination of the ordering of the property keys when written out and >> the usage of `SOURCE_DATE_EPOCH` environment value to determine the date >> comment should together allow for reproducibility of the output generated by >> these `store()` APIs. >> >> New jtreg test classes have been introduced to verify these changes. The >> primary focus of `PropertiesStoreTest` is the ordering aspects of the >> property keys that are written out. On the other hand >> `StoreReproducibilityTest` focuses on the reproducibility of the output >> generated by these APIs. The `StoreReproducibilityTest` runs these tests >> both in the presence and absence of `SecurityManager`. Plus, in the presence >> of SecurityManager, it tests both the scenarios where the caller is granted >> the requisite permission and in other case not granted that permission. >> >> These new tests and existing tests under `test/jdk/java/util/Properties/` >> pass with these changes. >> >> [1] >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html >> [2] https://reproducible-builds.org/specs/source-date-epoch/ > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > allow free-form text for java.util.Properties.storeDate system property src/java.base/share/classes/java/util/Properties.java line 832: > 830: * {@code #} character, the current date and time (formatted using > the > 831: * {@code EEE MMM dd HH:mm:ss zzz yyyy} {@link DateTimeFormatter > date format}), > 832: * and a line separator as generated by the {@code Writer}. Since this behavior is affected by a system property, and that property name is in the standard `java.*` namespace, that should be documented here. In addition, `Writer` has no notion of a line separator; it just comes from `System.lineSeparator`. I'd suggest something like this, replacing the paragraph starting with "Next": If the {@systemProperty java.util.Properties.storeDate} is set and is non-blank (as determined by {@link String#isBlank String.isBlank}, a comment line is written as follows. First, a {@code #} character is written, followed by the contents of the property, followed by a line separator. If the system property is not set or is blank, a comment line is written as follows. First, a {@code #} character is written, followed by the current date and time formatted as if by {@link DateTimeFormatter} with the format {@code "EEE MMM dd HH:mm:ss zzz yyyy"}, followed by a line separator. This was discussed elsewhere, but after writing that, I'm now thinking that we **should** honor the property even if it is blank. It would be useful to disable the timestamp simply by setting the property to the empty string; this will simply result in an empty comment line. This would simplify the code (and the spec) by removing conditions related to String::isBlank. Side question: do we want to do any escaping or vetting of the property value? If for example it contains embedded newlines, this could result in a malformed property file. Or, if carefully constructed, it could contain additional property values. I think this is an unintended consequence of changing the property value from a numeric time value to a free-form string. We may want to reconsider this. src/java.base/share/classes/java/util/Properties.java line 855: > 853: * the value of this system property represents a formatted > 854: * date time value that can be parsed back into a {@link Date} using > an appropriate > 855: * {@link DateTimeFormatter} With the property behavior added to normative text above, I don't think we need this note anymore. We might want to leave something here about the convention of putting a timestamp here, and an implicit(?) requirement that it be formatted properly. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372
