On 10/09/21 7:59 pm, Roger Riggs wrote:
On Fri, 10 Sep 2021 10:15:45 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:
jcheck fix - trailing whitespace in test
I'm inclined to agree with Magnus, supplying the property as a string, avoids
hardcoding details that are not really in the domain of Properties. It opens
up the option to omit the date and simplifies the code and spec.
Since the tools exist at build time or in the setting of the property to format
it as needed, Properties does not need to.
If non-null and non-empty it would be inserted as a comment. The string would not
contain the "# " prefix.
However, that goes against one of the goals this PR is trying to achieve
of backward compatibility of these existing store() APIs. Allowing a
free form text will mean that someone can feed a "foo bar" and we will
end up writing that as a comment, which essentially means that the
stored properties file no longer has any date comment. Another example
is, someone feeds a valid formatted text date to this system property
but that format doesn't match the format that is advertized till date
(the one which Date.toString() uses).
Are we willing to allow that?
-Jaikiran
