On Sun, 5 Sep 2021 12:38:20 GMT, Jaikiran Pai <jai.forums2013@gmail.com> wrote:

Do you mean that converting the keySet() of an existing Map into an array and then sorting that array and then using that sorted array to iterate and using these keys to do an additional lookup for value against the original Map would be more efficient in this case?

You can convert entrySet() to array. Not a keySet. In this case there is no need to lookup for values. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Sat, 4 Sep 2021 18:30:06 GMT, Andrey Turbanov <github.com+741251+turbanoff@openjdk.org> wrote:

...

Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:

use @implNote to explain the use of the environment variable

src/java.base/share/classes/java/util/Properties.java line 924:

...

922: writeDateComment(bw); 923: synchronized (this) { 924: for (Map.Entry<Object, Object> e : new TreeMap<>(map).entrySet()) {

Is this sorting intentionally added? It's not clear from issue description or PR description that order of properties should be changed too. Anyway I think copying `entrySet()` to array and then sorting should be faster, than creating a TreeMap

In case of reproducibility it should be at least ordered, i.e. keep original input order. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On 05/09/21 6:01 pm, Jaikiran Pai wrote:

Hello Alan,

On 05/09/21 1:46 pm, Alan Bateman wrote:

...

On 04/09/2021 16:50, Jaikiran Pai 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. In the discussion on this option then I think we were talking about changing the specification of the store methods to allow for an implementation specific means to override the date and put the discussion on SOURCE_DATE_EPOCH in an @implNote.

I will move the updated javadoc to an @implNote then. I guess, the existing part where it explains how the current date comment is written out, should stay where it is currently?

I've now updated the PR to move the new text of this javadoc into a @implNote. -Jaikiran

On 07/09/21 8:35 pm, Roger Riggs wrote:

Hi,

The value of SOURCE_DATE_EPOCH is not so sensitive that it needs the protections you are applying. The doPriv only exposes the value of that specific environment variable and in the usual case, it is undefined.

The complexity in the specification and implementation seem unnecessary in this case.

Given the inputs so far, the doPriveleged approach appears to be acceptable. So I'll go ahead and update the PR shortly to change this part.

Though java.util.Date is used in the current implementation, its use is discouraged in new code in favor of java.time.ZonedDateTime. Consider if java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME can be used to parse and format the time.

Noted. I'll take a look at these and update the PR as necessary. Thank you all for the inputs so far. -Jaikiran

Hello Stuart, On 08/09/21 6:49 am, Stuart Marks wrote:

On 9/7/21 8:27 AM, Jaikiran Pai wrote:

...

On 07/09/21 8:35 pm, Roger Riggs wrote:

...

Though java.util.Date is used in the current implementation, its use is discouraged in new code in favor of java.time.ZonedDateTime. Consider if java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME can be used to parse and format the time.

Noted. I'll take a look at these and update the PR as necessary.

Unless there's an overriding reason, it might be nice to have the output format match the format used in the Debian patch that adds SOURCE_DATE_EPOCH:

https://salsa.debian.org/openjdk-team/openjdk/-/blob/master/debian/patches/r...

So the current patch implementation uses the format "d MMM yyyy HH:mm:ss 'GMT'", with a Locale.ROOT (for locale neutral formatting). I chose this format since that was the one that the (deprecated) java.util.Date#toGMTString() was using. Roger's suggestion is to use DateTimeFormatter#RFC_1123_DATE_TIME date format which is "dow, d MMM yyyy HH:mm:ss GMT" (where dow == day of week) IMO, either of these formats are "well known", since they are/were used within the JDK, especially the DateTimeFormatter#RFC_1123_DATE_TIME which Roger suggested, since that's even a public spec. The one in the debian patch is "yyyy-MM-dd HH:mm:ss z" which although is fine to use, it however feels a bit "less known". I was leaning towards Roger's suggestion to use the RFC_1123_DATE_TIME in my upcoming patch update. Is there a reason why the one in debian's patch is preferable compared to a spec backed format? -Jaikiran

Hello Stuart, On 09/09/21 12:35 am, Stuart Marks wrote:

...

...

Unless there's an overriding reason, it might be nice to have the output format match the format used in the Debian patch that adds SOURCE_DATE_EPOCH:

https://salsa.debian.org/openjdk-team/openjdk/-/blob/master/debian/patches/r...

So the current patch implementation uses the format "d MMM yyyy HH:mm:ss 'GMT'", with a Locale.ROOT (for locale neutral formatting). I chose this format since that was the one that the (deprecated) java.util.Date#toGMTString() was using.

Roger's suggestion is to use DateTimeFormatter#RFC_1123_DATE_TIME date format which is "dow, d MMM yyyy HH:mm:ss GMT" (where dow == day of week)

IMO, either of these formats are "well known", since they are/were used within the JDK, especially the DateTimeFormatter#RFC_1123_DATE_TIME which Roger suggested, since that's even a public spec.

The one in the debian patch is "yyyy-MM-dd HH:mm:ss z" which although is fine to use, it however feels a bit "less known".

I was leaning towards Roger's suggestion to use the RFC_1123_DATE_TIME in my upcoming patch update. Is there a reason why the one in debian's patch is preferable compared to a spec backed format?

My point in bringing this is up is to consider interoperability. I don't have a strong preference over the particular date format. As far as I can see, there are currently two behaviors "in the wild":

1) Baseline OpenJDK 17 behavior:

    dow mon dd hh:mm:ss zzz yyyy

This is the behavior provided by "new Date().toString()" and has likely not changed in many years. Of course, the actual values reflect the current time and locale, which hurts reproducibility, but the format itself hasn't changed.

2) Debian's OpenJDK with SOURCE_DATE_EPOCH set:

    yyyy-MM-dd HH:mm:ss z

The question is, what format should the JDK-8231640 use?

I had said earlier that it might be a good idea to match the Debian format. But thinking about this further, I think sticking with the original JDK format would be preferable. The Debian change is after all an outlier.

So the more specific question is, should we try to continue with the original JDK format or choose a format that's "better" in some sense? It seems to me that if there's something out there that parses the date from a properties file, we'd want to avoid breaking this code if the environment variable is set. So maybe stick with the original format in all cases. But of course for reproducibility use the epoch value from the environment and set the locale and zone offset to known values.

All this makes sense. So I've updated the PR to continue to use the same date format that java.util.Date.toString() was using, in both the cases - while writing out the current date (in absence of SOURCE_DATE_EPOCH) and while writing out the SOURCE_DATE_EPOCH. While writing out the SOURCE_DATE_EPOCH, we will however fix the timezone to UTC and locale to ROOT for reproducibility. -Jaikiran

On 07/09/21 9:02 pm, Alan Bateman wrote:

On 07/09/2021 16:05, Roger Riggs wrote:

...

Hi,

The value of SOURCE_DATE_EPOCH is not so sensitive that it needs the protections you are applying. The doPriv only exposes the value of that specific environment variable and in the usual case, it is undefined.

The complexity in the specification and implementation seem unnecessary in this case.

I agree.  Given the complexity then it makes your suggestion/option to just drop the date from the comment somewhat tempting.

-Alan

I've now updated the PR to take into account the inputs that were provided so far. More specifically, the PR has been updated to:  - remove the complexity around SecurityManager usage and now just uses a doPriveleged block to get the SOURCE_EPOCH_DATE.  - use Arrays.sort(...) instead of a TreeMap to write out the sorted properties. This was a suggestion from Andrey and based on my JMH testing (which I will post separately), the Arrays.sort(...) did indeed perform better.  - use DateTimeFormatter.RFC_1123_DATE_TIME while formatting and writing the reproducible SOURCE_DATE_EPOCH value. There isn't a general agreement yet on what format should be used. Stuart has suggested using a different format (the one in the debian patch). So this part of the change could still undergo further change.  - use ZonedDateTime along with a DateTimeFormatter which matches the format used by java.util.Date.toString(), instead of using a java.util.Date() instance when writing out the current date. The new tests that have been introduced in this PR have been adjusted to verify these new expectations. The existing and these new tests continue to pass with these changes. -Jaikiran

On Sun, 5 Sep 2021 12:56:46 GMT, Jaikiran Pai <jpai@openjdk.org> wrote:

The failure is consistently in a single test InfoOptsTest and it fails with the following error when running the testUniqueInfoOpts test case. Looks like a recent issue in some commit. I'll sort that out separately.

Created https://bugs.openjdk.java.net/browse/JDK-8273361 ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Wed, 8 Sep 2021 09:32:55 GMT, Jaikiran Pai <jpai@openjdk.org> wrote:

...

Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:

- adjust testcases to verify the new semantics - implement review suggestions: - Use doPriveleged instead of explicit permission checks, to reduce complexity - Use DateTimeFormatter and ZonedDateTime instead of Date.toString() - Use DateTimeFormatter.RFC_1123_DATE_TIME for formatting SOURCE_DATE_EPOCH dates - Use Arrays.sort instead of a TreeMap for ordering property keys

src/java.base/share/classes/java/util/Properties.java line 963:

...

961: synchronized (this) { 962: var entries = map.entrySet().toArray(new Map.Entry<?, ?>[0]); 963: Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() {

This part here, intentionally doesn't use a lambda, since from what I remember seeing in some mail discussion, it was suggested that using lambda in core parts which get used very early during JVM boostrap, should be avoided. If that's not a concern here, do let me know and I can change it to a lambda.

This is a fair concern, but writing out a properties file is a pretty high-level operation that doesn't seem likely to be used during bootstrap. Also, I observe that the `doPrivileged` block above uses a lambda. So I think we're probably ok to use a lambda here. But if you get an inexplicable error at build time or at startup time, this would be the reason why. Assuming we're ok with lambdas, it might be easier to use collections instead of arrays in order to preserve generic types. Unfortunately the map is `Map<Object, Object>` so we have to do some fancy casting to get the right type. But then we can use `Map.Entry.comparingByKey()` as the comparator. (Note that this uses lambda internally.) Something like this might work: @SuppressWarnings("unchecked") var entries = new ArrayList<>(((Map<String, String>)(Map)map).entrySet()); entries.sort(Map.Entry.comparingByKey()); ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Wed, 8 Sep 2021 09:26:33 GMT, Jaikiran Pai <jpai@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.htm... [2] https://reproducible-builds.org/specs/source-date-epoch/

Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:

- adjust testcases to verify the new semantics - implement review suggestions: - Use doPriveleged instead of explicit permission checks, to reduce complexity - Use DateTimeFormatter and ZonedDateTime instead of Date.toString() - Use DateTimeFormatter.RFC_1123_DATE_TIME for formatting SOURCE_DATE_EPOCH dates - Use Arrays.sort instead of a TreeMap for ordering property keys

src/java.base/share/classes/java/util/Properties.java line 886:

884: * In the presence of a SecurityManager, if the caller doesn't have permission 885: * to read the {@code SOURCE_DATE_EPOCH} environment variable, then the current date 886: * and time will be written. Similarly, if the value set for {@code SOURCE_DATE_EPOCH}

This is no longer true - is it? It seems you changed the code but not the spec :-) ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Wed, 8 Sep 2021 09:54:33 GMT, Jaikiran Pai <jpai@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.htm... [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:

update javadoc @implNote to match latest changes

Am I the only one thinking there should also be a way for developers to explicitly disable timestamps from the API? I think the current iteration looks okay (but the core-libs guys of course has the say in this), but I still think we need a new method (or overload) to allow for a timestamp-free to be generated, always, independent of environment variables. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Thu, 9 Sep 2021 08:31:30 GMT, Jaikiran Pai <jpai@openjdk.org> wrote:

...

Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:

update javadoc to match latest changes

Hello Magnus,

...

Am I the only one thinking there should also be a way for developers to explicitly disable timestamps from the API?

I think the current iteration looks okay (but the core-libs guys of course has the say in this), but I still think we need a new method (or overload) to allow for a timestamp-free to be generated, always, independent of environment variables.

I decided to take this comment to the mailing list[1] where the proposal for this change was discussed, since it's likely more people will be watching that thread than this RFR thread.

[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-September/081305....

@jaikiran Thanks for following this through all the twists and turns. The review thread here is pretty long, but I think we've made a lot of progress and are close to converging. In addition, the code keeps getting simpler, so that's a good thing. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Thu, 9 Sep 2021 09:56:30 GMT, Jaikiran Pai <jpai@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.htm... [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:

dummy commit to trigger GitHub actions job to try and reproduce an unexplained failure with the new tests that happened around 24 hours back, this time yesterday (rule out any time/date/timezone specific issues)

Overall looks good, though a system property would be more in keeping with existing JDK pattern. See comments. src/java.base/share/classes/java/util/Properties.java line 187:

185: ? System.getenv("SOURCE_DATE_EPOCH") 186: : AccessController.doPrivileged((PrivilegedAction<String>) 187: () -> System.getenv("SOURCE_DATE_EPOCH"));

The format of SOURCE_DATE_EPOCH is fine, but referring to an external document as part of the OpenJDK spec is undesirable in the long run. Previous comments gave support to using the environment variable directly but it very unlike similar cases that use system properties when the behavior of an API needs to be overridden or modified. As a system property, specified on the command line, it would be visible when the program is invoked, explicitly intended to change behavior and not having a context sensitive effect. Named perhaps, java.util.Properties.storeDate. It can be documented as part of the spec with the javadoc tag "{@systemProperty ...}" There is a cache of properties in jdk.internal.util.StaticProperty that captures the state when the runtime is initialized. For specific properties, they are cached and available without regard to the setting of the SecurityManager or not. (BTW, there is no need to special case doPriv calls, they are pretty well optimized when the security manager is not set and it keeps the code simpler.) Given the low frequency of calls to store(), even caching the parsed date doesn't save much. And the cost is the cost of the size and loading an extra class. src/java.base/share/classes/java/util/Properties.java line 887:

885: * If the value set for {@code SOURCE_DATE_EPOCH} cannot be parsed to a {@code long}, 886: * then the current date and time will be written. 887: *

Update to refer to the property and add the {@systemProperty ...} javadoc tag so the property is listed in the system property reference. ------------- Changes requested by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5372

Looking at the discussion I see a fear of extending the Properties API, where every solution comes with its own little ugliness. There's one topic that hasn't been mentioned yet: is Properties responsible for writing its own content? While breaking up the JDK into modules, an weird thing was exposed: storeToXML, which would need a dependency on java.xml but java.base does not. Should there be a new class for writing Properties, where we have the control for specifying a new clean API? thanks, Robert ------ Origineel bericht ------ Van: "Jaikiran Pai" <jai.forums2013@gmail.com> Aan: core-libs-dev@openjdk.java.net; "Roger Riggs" <Roger.Riggs@oracle.com> Verzonden: 10-9-2021 03:57:27 Onderwerp: Re: RFR: 8231640: (prop) Canonical property storage [v7]

Hello Roger,

On 10/09/21 12:32 am, Roger Riggs wrote:

...

src/java.base/share/classes/java/util/Properties.java line 187:

...

185: ? System.getenv("SOURCE_DATE_EPOCH") 186: : AccessController.doPrivileged((PrivilegedAction<String>) 187: () -> System.getenv("SOURCE_DATE_EPOCH")); The format of SOURCE_DATE_EPOCH is fine, but referring to an external document as part of the OpenJDK spec is undesirable in the long run. Previous comments gave support to using the environment variable directly but it very unlike similar cases that use system properties when the behavior of an API needs to be overridden or modified.

As a system property, specified on the command line, it would be visible when the program is invoked, explicitly intended to change behavior and not having a context sensitive effect. Named perhaps, java.util.Properties.storeDate.

Would this system property have a value that represents what SOURCE_DATE_EPOCH environment variable would have contained? i.e. it is the "A UNIX timestamp, defined as the number of seconds, excluding leap seconds, since 01 Jan 1970 00:00:00 UTC."? So users can potentially do -Djava.util.Properties.storeDate=${SOURCE_DATE_EPOCH}?

Or would this system property be just some kind flag, which when having a value of "true" would expect the java.util.Properties code to lookup the SOURCE_DATE_EPOCH environment variable and use that value from the environment variable?

I'm guessing it's the former where the value is the number of epoch seconds, but just wanted to be sure before I do this change.

...

(BTW, there is no need to special case doPriv calls, they are pretty well optimized when the security manager is not set and it keeps the code simpler.)

From what I see in the implementation of AccessController.doPriveleged(), I see that it does a bunch of uncoditional work (like Reflection.getCallerClass() and then Reference.reachabilityFence() calls). Do you mean that additional work is neglible in the absence of a security manager, that this special casing of the doPriv call can be removed?

...

Given the low frequency of calls to store(), even caching the parsed date doesn't save much. And the cost is the cost of the size and loading an extra class.

Sounds fine. I'll remove the caching part in my subsequent update.

-Jaikiran

On 2021-09-10 03:57, Jaikiran Pai wrote:

Would this system property have a value that represents what SOURCE_DATE_EPOCH environment variable would have contained? i.e. it is the "A UNIX timestamp, defined as the number of seconds, excluding leap seconds, since 01 Jan 1970 00:00:00 UTC."? So users can potentially do -Djava.util.Properties.storeDate=${SOURCE_DATE_EPOCH}?

Or would this system property be just some kind flag, which when having a value of "true" would expect the java.util.Properties code to lookup the SOURCE_DATE_EPOCH environment variable and use that value from the environment variable?

I'm guessing it's the former where the value is the number of epoch seconds, but just wanted to be sure before I do this change.

Actually, why don't we define it as a free-form string instead? That way the onus is on the user setting the property to get it formatted the way they want. If they want a backwards-compatibly formatted string for SOURCE_DATE_EPOCH, they'd have to call Java with an argument something along the lines of: -Djava.util.Properties.storeDate="$(date --date=@${SOURCE_DATE_EPOCH} +"%a %b %d %H:%M:%S %Z %Y")" which is not much more terrible than -Djava.util.Properties.storeDate=${SOURCE_DATE_EPOCH} given that we in any case won't be reading SOURCE_DATE_EPOCH directly. This also allows for the user to just skip the date completely: -Djava.util.Properties.storeDate="" By changing this property from a epoch based long to a string, all formatting and verification gets removed from your patch, and it is greatly simplified. /Magnus

On Fri, 10 Sep 2021 10:15:45 GMT, Jaikiran Pai <jpai@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.htm... [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. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

Hi, It does move the responsibility for that specific compatibility to the person defining the system property. I would document it as conventionally having the date in the format expected. In the context of a reproducible build, where the property is expected to be used, the builder would be able to track down tools that were expecting a specific format and correct the input/property setting. I think the incorrectly formatted string would be more use for tracking down a problem than having the date be in the correct format but quietly different from what was expected. (As is, an unparseable property value is replaced by the current date). Roger On 9/10/21 10:40 AM, Jaikiran Pai wrote:

On 10/09/21 7:59 pm, Roger Riggs wrote:

...

On Fri, 10 Sep 2021 10:15:45 GMT, Jaikiran Pai <jpai@openjdk.org> wrote: 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

On Fri, 10 Sep 2021 10:15:45 GMT, Jaikiran Pai <jpai@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.htm... [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

Hi Jaikiran, On 9/9/21 9:59 PM, mlbridge[bot] wrote:

/Mailing list message from Jaikiran Pai ***@***.***> on core-libs-dev ***@***.***>:/

Hello Roger,

On 10/09/21 12:32 am, Roger Riggs wrote:

src/java.base/share/classes/java/util/Properties.java line 187:

185: ? System.getenv("SOURCE_DATE_EPOCH") 186: : AccessController.doPrivileged((PrivilegedAction<String>) 187: () -> System.getenv("SOURCE_DATE_EPOCH")); The format of SOURCE_DATE_EPOCH is fine, but referring to an external document as part of the OpenJDK spec is undesirable in the long run. Previous comments gave support to using the environment variable directly but it very unlike similar cases that use system properties when the behavior of an API needs to be overridden or modified.

As a system property, specified on the command line, it would be visible when the program is invoked, explicitly intended to change behavior and not having a context sensitive effect. Named perhaps, java.util.Properties.storeDate.

Would this system property have a value that represents what SOURCE_DATE_EPOCH environment variable would have contained? i.e. it is the "A UNIX timestamp, defined as the number of seconds, excluding leap seconds, since 01 Jan 1970 00:00:00 UTC."? So users can potentially do -Djava.util.Properties.storeDate=${SOURCE_DATE_EPOCH}?

Or would this system property be just some kind flag, which when having a value of "true" would expect the java.util.Properties code to lookup the SOURCE_DATE_EPOCH environment variable and use that value from the environment variable?

I'm guessing it's the former where the value is the number of epoch seconds, but just wanted to be sure before I do this change.

I agree with Magnus' suggestion to make it just a comment string to be included if it is non-null and non-empty.

(BTW, there is no need to special case doPriv calls, they are pretty well optimized when the security manager is not set and it keeps the code simpler.)

From what I see in the implementation of AccessController.doPriveleged(), I see that it does a bunch of uncoditional work (like Reflection.getCallerClass() and then Reference.reachabilityFence() calls). Do you mean that additional work is neglible in the absence of a security manager, that this special casing of the doPriv call can be removed?

(The code is gone now) I meant that the testing for SM == null to avoid the doPriv won't improve performance much and performance isn't critical here. The context will be null, getCallerClass is optimized(an Intrinsic), and the reachabilityFence() is effectively a no-op, to keep gc from reclaiming objects too soon. So all those a lightweight or optimized away.

Given the low frequency of calls to store(), even caching the parsed date doesn't save much. And the cost is the cost of the size and loading an extra class.

Sounds fine. I'll remove the caching part in my subsequent update.

Thanks, Roger

-Jaikiran

— You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/5372*issuecomment-916571385__;Iw!!ACWV5N9M2RV99hQ!Yuv9mklDfoz0-neVqyTkIVkT2pStt3pp56LC_GtNhZ1XaO6Iq34i47TUsILYLySc$>, or unsubscribe <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AAIRSVEFZBJXHRXMASLW4MDUBFRA7ANCNFSM5DNMPLNA__;!!ACWV5N9M2RV99hQ!Yuv9mklDfoz0-neVqyTkIVkT2pStt3pp56LC_GtNhZ1XaO6Iq34i47TUsHmTJQki$>. Triage notifications on the go with GitHub Mobile for iOS <https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!ACWV5N9M2RV99hQ!Yuv9mklDfoz0-neVqyTkIVkT2pStt3pp56LC_GtNhZ1XaO6Iq34i47TUsIOavqZC$> or Android <https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!ACWV5N9M2RV99hQ!Yuv9mklDfoz0-neVqyTkIVkT2pStt3pp56LC_GtNhZ1XaO6Iq34i47TUsHjk8HBC$>.

------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Fri, 10 Sep 2021 15:51:30 GMT, Jaikiran Pai <jpai@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.htm... [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

Hi Jaikiran, "Editing" the value of the comment property, to remove or ignore blanks for other special characters makes the code more complex and adds a bunch of conditions. Dropping the comment if it doesn't have the allowed format is hard to track down, because there's no way to report it was dropped or why. I would write the value of the comment property using the writeComments method so it is encoded the same as the other comment passed as an store argument. That will handle all special characters and multi-line comments.  It is simpler to specify and has the same predictable output as other comments. if the property is non-empty On 9/12/21 10:29 AM, Jaikiran Pai wrote:

...

...

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.

OK. I don't see any obvious issues with interpreting empty/whitespace-only value for the system property to translate to an empty comment line. To be clear, an empty comment line that gets written out in such cases is *always* going to be the "#" character followed by a line separator right? Irrespective of what or how many whitespace characters are present in the property value? Or do you want those characters to be carried over into that comment line that gets written out? The reason I ask this is because I think we should always write just the "#" followed by the line separator character in such cases, which effectively means we will still need the String::isBlank check which would then look something like:

if (propVal.isBlank()) {   bw.write("#");   bw.newLine(); }

...

Side question: do we want to do any escaping or vetting of the property value?

One of the reasons why we didn't want to use the date in epoch seconds or a formatted date string was to avoid the complexities that come with managing that property value. So a free-form property value seemed more appropriate and I think a free-form value still seems appropriate, but I think we should keep any vetting to minimal. More details below.

...

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.

The specification on the load(Reader reader) method of the java.util.Properties class has this to say about comment lines (and lines in general).

(snipped relevant content):

   There are two kinds of line, <i>natural lines</i> and <i>logical    lines</i>.    A natural line is defined as a line of characters that is terminated    either    by a set of line terminator characters ({@code \n} or {@code \r} or    {@code \r\n}) or by the end of the stream. A natural line may be    either a blank line, a comment line, or hold all or some of a    key-element pair. A logical    line holds all the data of a key-element pair, which may be spread    out across several adjacent natural lines by escaping    the line terminator sequence with a backslash character    {@code \}.  Note that a comment line cannot be extended    in this manner; every natural line that is a comment must have    its own comment indicator, as described below.

With that knowledge about comment lines, I think what we should do is disallow system property values that contain any form of line terminator characters (\n or \r or \r\n). If the system property value is _not_ blank (as specified by String::isBlank) and contains any of these line terminator characters, we should simply ignore it and write out the current date as the date comment. That, IMO, should prevent any of these sneaky/rogue value that can end up either creating additional property key/values to be written out or causing any malformed properties file. Plus, that would help us keep the vetting to minimal without involving too much complexity.

...

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.

The newly updated PR has updated this @implNote to remove some of the previous text and yet retain some hints on what would be an "ideal" value for the system property value. But I think we should perhaps just get rid of this @implNote since we are now proposing to allow empty comment lines and free form text and this no longer is a "let's use this system property to allow users to specify a fixed date" enhancement. good

-Jaikiran

On 14/09/2021 03:28, Jaikiran Pai wrote:

Hello Roger,

I've now updated the PR to implement these suggested changes. I think at this point all suggestions have been implemented and I don't think there are any open questions.

If the latest PR looks fine, I think the next step will be a CSR creation.

Thanks for hanging in there, I think you've got this to a good place. The system property and resulting behavior look good so you should be able to create the CSR. -Alan

On Sun, 12 Sep 2021 14:03:42 GMT, Jaikiran Pai <jpai@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.htm... [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:

Update javadoc based on Stuart's inputs

Thinking more about this, given that there's now willingless to allow this system property to allow empty comments or even free form text and do away with backward compatibilty concerns that this will no longer be representing a date comment, I would like to propose something else. Let's take a small step back though. I think we all agree that the text of the default date comment isn't what is contentious. Instead, it's the presence of that comment itself. So instead of trying to allow users to feed some different text into this comment, why not just allow them to say whether or not they want this comment? In other words, why not consider the value of java.util.Properties.storeDate be either "true" or "false" and consider it a boolean system property? That should avoid all these complexities. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Tue, 14 Sep 2021 10:47:34 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:

...

Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:

unused imports

src/java.base/share/classes/java/util/Properties.java line 836:

...

834: * <p> 835: * Then every entry in this {@code Properties} table is 836: * written out, one per line. For each entry the key string is

Sorry for coming late to the party: I don't remember if this was already asked for and rejected - but shouldn't there be a non-normative `@implNote` here to state that the default implementation of this method will write the properties sorted by their keys in alphanumeric ordering? Otherwise this looks very good!

Hello Daniel, you are right - I missed discussing whether or not to include a `@implNote` to explain the natural sorted order of the property keys. When we started this whole PR proposal, Alan had hinted that maybe we should "specify" this behaviour. Should this be a `@implNote` or should this be part of the formal spec like we did for the system property handling? ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Tue, 14 Sep 2021 12:56:28 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:

...

Hello Daniel, you are right - I missed discussing whether or not to include a `@implNote` to explain the natural sorted order of the property keys. When we started this whole PR proposal, Alan had hinted that maybe we should "specify" this behaviour. Should this be a `@implNote` or should this be part of the formal spec like we did for the system property handling?

I would leave it as an `@implNote` - or possibly `@implSpec`: depending on whether or not we want all implementations of the spec to behave in this way. However I don't think we would want to prevent subclasses from overriding this behavior and using their own business-logic ordering. So I believe the default behavior should be specified, if only so that subclasses can decide whether or not to override this method, without invalidating what subclasses might currently have implemented - or might wish to implement in the future. The CSR will be a good way to get feedback on whether `@implNote` or `@implSpec` is more appropriate. Also this is a change in behavior that needs to be made visible somewhere - and nobody reads release notes ;-)

Done. I've updated the PR to include a `@implNote` specifying the order of the properties. The CSR has been updated too to reflect this. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Tue, 14 Sep 2021 02:30:01 GMT, Jaikiran Pai <jpai@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.htm... [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:

unused imports

src/java.base/share/classes/java/util/Properties.java line 828:

826: * a comment line is written as follows. 827: * First, a {@code #} character is written, followed by the contents 828: * of the property, followed by a line separator.

Maybe you should refer to how the comment is written out above, since you use the same method. The spec changes as written above does not specify how newlines in the property would be handled (which is possible to get, I believe, even if it means an intricate command line escape dance) ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Tue, 14 Sep 2021 02:30:01 GMT, Jaikiran Pai <jpai@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.htm... [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:

unused imports

I've created a CSR https://bugs.openjdk.java.net/browse/JDK-8273727 and added the details. I'll update that javadoc to include the properties order specification too once we decide whether it makes sense to have it in `@implNote` or as the main text. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Tue, 14 Sep 2021 13:19:25 GMT, Jaikiran Pai <jpai@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.htm... [2] https://reproducible-builds.org/specs/source-date-epoch/

Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:

- Add a @implNote to specify the order in which the properties are written out - update the javadoc to clarify how line terminator characters are handled in the value of the java.util.Properties.storeDate system property

src/java.base/share/classes/java/util/Properties.java line 822:

820: * {@link System#lineSeparator() line separator} and if the next 821: * character in comments is not character {@code #} or character {@code !} then 822: * an ASCII {@code #} is written out after that line separator.

Should something be done for comments ending with \ (backslash) ? It might otherwise suppress the first property assignment that follows. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Tue, 14 Sep 2021 13:24:55 GMT, Jaikiran Pai <jpai@openjdk.org> wrote:

...

src/java.base/share/classes/java/util/Properties.java line 822:

...

820: * {@link System#lineSeparator() line separator} and if the next 821: * character in comments is not character {@code #} or character {@code !} then 822: * an ASCII {@code #} is written out after that line separator.

Should something be done for comments ending with \ (backslash) ? It might otherwise suppress the first property assignment that follows.

There has been no change in how we deal with this aspect. The existing specification (stated in the `load` method) says:

...

* Properties are processed in terms of lines. There are two * kinds of line, <i>natural lines</i> and <i>logical lines</i>. * A natural line is defined as a line of * characters that is terminated either by a set of line terminator * characters ({@code \n} or {@code \r} or {@code \r\n}) * or by the end of the stream. A natural line may be either a blank line, * a comment line, or hold all or some of a key-element pair. A logical * line holds all the data of a key-element pair, which may be spread * out across several adjacent natural lines by escaping * the line terminator sequence with a backslash character * {@code }. **Note that a comment line cannot be extended * in this manner;**

(emphasis on that last sentence). I'll anyway go ahead and add new tests around this to be sure that this works as advertised.

Oh - great - thanks - verifying by a test that it also applies to the comment specified by `java.util.Properties.storeDate` would be good. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Tue, 14 Sep 2021 14:02:10 GMT, Jaikiran Pai <jpai@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.htm... [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:

Introduce a test to make sure backslash character in the system property value doesn't cause unexpected output. Plus minor updates to tests to add additional checks.

Looks good. src/java.base/share/classes/java/util/Properties.java line 955:

953: bw.write("#" + DateTimeFormatter.ofPattern(dateFormatPattern).format(ZonedDateTime.now())); 954: bw.newLine(); 955: }

Since there is no longer a need to format an arbitrary date, I'd suggest going back to the original Date.toString() code. It removes the need to replicate the format using DateTimeBuilder and is known to be the same as before. If you keep the DateTimeFormat version, you would want "uuuu" instead of "yyyy". In java.time. DateTimeFormatterBuilder uses slightly different pattern letter conventions as compared to SimpleDateFormat. test/jdk/java/util/Properties/StoreReproducibilityTest.java line 429:

427: try { 428: parsedDate = new SimpleDateFormat(dateCommentFormat).parse(dateComment); 429: } catch (ParseException pe) {

Its slightly better to use the same date formatting and parsing APIs consistently. DateTimeFormatter.parse could be used here since DateTimeFormatter was used above. (Though the pattern uses "yyyy" instead of "uuuu" for the year.) ------------- Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5372

On Tue, 14 Sep 2021 14:02:10 GMT, Jaikiran Pai <jpai@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.htm... [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:

Introduce a test to make sure backslash character in the system property value doesn't cause unexpected output. Plus minor updates to tests to add additional checks.

src/java.base/share/classes/java/util/Properties.java line 929:

927: @SuppressWarnings("unchecked") 928: var entries = new ArrayList<>(((Map<String, String>) (Map) map).entrySet()); 929: entries.sort(Map.Entry.comparingByKey());

Since Properties can be subclassed and the `entrySet()` method overridden, should the set of entries to be sorted be taken from this.entrySet() instead of bypassing the public API? ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Tue, 14 Sep 2021 20:34:21 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:

...

src/java.base/share/classes/java/util/Properties.java line 929:

...

927: @SuppressWarnings("unchecked") 928: var entries = new ArrayList<>(((Map<String, String>) (Map) map).entrySet()); 929: entries.sort(Map.Entry.comparingByKey());

Since Properties can be subclassed and the `entrySet()` method overridden, should the set of entries to be sorted be taken from this.entrySet() instead of bypassing the public API?

Hmm, so if someone has subclassed `Properties` and overridden `entrySet` for the purpose of ordering entries, that trick will no longer work with the new code. I wonder - should we sort entries only when the `java.util.Properties.storeDate` is also defined and not empty? Or should we simply state in the release notes that such tricks will no longer work?

I think we want the entries to be sorted by default; that is the most useful to the most people. Checking for the overloaded method seems like trying too hard. Changing the entrySet to return them sorted (always) would add overhead in a lot of cases but would allow a subclass to re-sort them as they see fit. A half measure would be to sort only if the properties instance was not subclassed and spec it that way as an implementation detail. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Wed, 15 Sep 2021 06:11:31 GMT, Jaikiran Pai <jpai@openjdk.org> wrote:

...

I think we want the entries to be sorted by default; that is the most useful to the most people. Checking for the overloaded method seems like trying too hard. Changing the entrySet to return them sorted (always) would add overhead in a lot of cases but would allow a subclass to re-sort them as they see fit. A half measure would be to sort only if the properties instance was not subclassed and spec it that way as an implementation detail.

Good catch regarding the possibility of entrySet() being overridden.

...

I think we want the entries to be sorted by default; that is the most useful to the most people.

Agreed

...

Changing the entrySet to return them sorted (always) would add overhead in a lot of cases but would allow a subclass to re-sort them as they see fit.

Agreed - I think changing the implementation of entrySet to return an ordered set is an unnecessary overhead in the context of what's being targeted in this PR.

...

A half measure would be to sort only if the properties instance was not subclassed and spec it that way as an implementation detail.

I think just checking the properties instance type to see if it is subclassed is perhaps too big a penalty for those applications that do subclass java.util.Properties but don't override the entrySet() method. Such applications/subclasses will then never be able to use this new implementation where we write out the properties in the natural order. Perhaps a middle ground would be your other option:

...

Checking for the overloaded method seems like trying too hard.

IMO, I don't think we would be trying too hard to identify this. I have updated this PR to try and articulate this both in terms of the implementation as well as the updated javadoc of this method. Tests have then been introduced to verify this case of Properties subclasses. This https://github.com/openjdk/jdk/pull/5372/commits/79d1052775dd8e98fb7078710ed... is the relevant commit in the updated PR. I understand that we haven't come to an agreement on this aspect yet, but it was easier to show it in terms of code and spec, so I decided to include this commit in the PR. I will of course revert it or change it appropriately depending on how we want to deal with this case.

...

Checking for the overloaded method seems like trying too hard.

IMO, I don't think we would be trying too hard to identify this. I have updated this PR to try and articulate this both in terms of the implementation as well as the updated javadoc of this method. Tests have then been introduced to verify this case of Properties subclasses. This 79d1052 is the relevant commit in the updated PR. I understand that we haven't come to an agreement on this aspect yet, but it was easier to show it in terms of code and spec, so I decided to include this commit in the PR. I will of course revert it or change it appropriately depending on how we want to deal with this case.

Hmm. Interesting idea - it looks like it could work given that properties as a private `EntrySet` class used for its entry set implementation... @RogerRiggs what do you think? ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Wed, 15 Sep 2021 15:31:45 GMT, Roger Riggs <rriggs@openjdk.org> wrote:

...

Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:

- Clarify how overriden Properties#entrySet() method impacts the order of stored properties - Tests to verify subclasses of Properties

src/java.base/share/classes/java/util/Properties.java line 850:

...

848: * the {@code entrySet} method and return a different {@code Set} instance, 849: * then the property list is written out in the iteration order of 850: * that returned {@code Set}

Rewording a bit:

"The keys and elements are written in the natural sort order of the keys in the `Properties.entrySet` unless `entrySet` is overridden by a subclass to return a different instance."

"different instance" is a bit hard to implement given that entrySet() returns a new synchronized set each time. typo: missing final "."

yes - maybe we could work on the wording. Perhaps:

The keys and elements are written in the natural sort order of the keys in the Properties.entrySet unless entrySet is overridden by a subclass to return a different set implementation.

src/java.base/share/classes/java/util/Properties.java line 932:

...

930: if (entries instanceof Collections.SynchronizedSet<?> ss 931: && ss.c instanceof EntrySet) { 932: entries = new ArrayList<>(entries);

From the description referring to a 'different instance', I expected to see == or != in this test. Since Properties.entrySet() always returns a new synchronized set of a new EntrySet, specifying that the underlying map is the same instance is more difficult. Perhaps either the SynchronizedSet or the EntrySet instance could be cached and compared.

(typo: missing space after period... ".If yes")

A custom subclass couldn't create an instance of EntrySet directly. Therefore if it wants to reorder the entries, it would have to return a new TreeSet or LinkedHashSet - or some other custom set implementation. If the result of calling entrySet() is an EntrySet wrapped in an UnmodifiableSet it's therefore legitimate to assume that the set hasn't been reordered, and that we can reorder it. Or am I missing something? ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Wed, 15 Sep 2021 16:13:56 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:

...

src/java.base/share/classes/java/util/Properties.java line 932:

...

930: if (entries instanceof Collections.SynchronizedSet<?> ss 931: && ss.c instanceof EntrySet) { 932: entries = new ArrayList<>(entries);

From the description referring to a 'different instance', I expected to see == or != in this test. Since Properties.entrySet() always returns a new synchronized set of a new EntrySet, specifying that the underlying map is the same instance is more difficult. Perhaps either the SynchronizedSet or the EntrySet instance could be cached and compared.

(typo: missing space after period... ".If yes")

A custom subclass couldn't create an instance of EntrySet directly. Therefore if it wants to reorder the entries, it would have to return a new TreeSet or LinkedHashSet - or some other custom set implementation. If the result of calling entrySet() is an EntrySet wrapped in an UnmodifiableSet it's therefore legitimate to assume that the set hasn't been reordered, and that we can reorder it. Or am I missing something?

I've updated the PR to clarify what this part of the code is doing. I took Daniel's input to reword the javadoc as well as tried to improve the code comment where this check is done. Essentially, we check for the "type" of the returned intstance and if we see that it's a `SynchronizedSet` whose underlying collection is a private class `EntrySet` then we know that it's the one created by Properties.entrySet(). Like Daniel says, subclasses won't be able to create or use this internal private class and if they did override the entrySet() to change the order of the entries they would have to do it by returning us a different "type" from that method. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Wed, 15 Sep 2021 21:46:59 GMT, Stuart Marks <smarks@openjdk.org> wrote:

...

src/java.base/share/classes/java/util/Properties.java line 819:

...

817: * <p> 818: * If the {@systemProperty java.util.Properties.storeDate} is set and 819: * is non-empty (as determined by {@link String#isEmpty() String.isEmpty}),

"is set **on the command line** and non-empty"...

Following from a comment on the CSR, it should be clear that the property value used can only be set on the command line.

This is a clever way to detect whether the `entrySet()` method has been overridden to return something other than the entry-set provided by the Properties class... but I'm wondering if it's too clever. Does anyone who overrides entrySet() care about a specific order, or do they simply sort it in order to get reproducible output? If the latter, then sorting by default hasn't really broken anything.

Also, it was never specified that the properties are written based on what's returned by a self-call to `entrySet()`. So this was never guaranteed, though we do want to avoid gratuitous breakage.

I would also wager that anybody who overrides entrySet() so that they can control the ordering of the entries is probably breaking the contract of Map::entrySet, which says that it's mutable (a mapping can be removed by removing its entry from the entry-set, or the underlying value can be changed by calling setValue on an entry). This is admittedly pretty obscure, but it tells me that trying to customize the output of Properties::store by overriding entrySet() is a pretty fragile hack.

If people really need to control the order of output, they need to iterate the properties themselves instead of overriding entrySet(). I think the only difficulty in doing so is properly escaping the keys and values, as performed by saveConvert(). If this is a use case we want to support, then maybe we should expose a suitable API for escaping properties keys and values. That would be a separate enhancement, though.

Note some history in this Stack Overflow question and answers:

https://stackoverflow.com/questions/10275862/how-to-sort-properties-in-java

Basically they mostly describe overriding `keys()`, but we broke that in Java 9, and now the advice is to override `entrySet()`. But the goal is to sort the properties, which we're doing anyway!

One part of what store() does is annoying to replicate, the encoding that `saveConvert` does is non-trivial. Other hacks based on returning a different entrySet might be to filter the set either keep some entries or ignore them. Both unlikely, but hacks are frequently fragile. And we've been very cautious in this discussion to avoid breaking things, in part, because there is so little info about how it is used. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On 16/09/21 4:05 am, Roger Riggs wrote:

On Wed, 15 Sep 2021 21:46:59 GMT, Stuart Marks <smarks@openjdk.org> wrote:

...

...

src/java.base/share/classes/java/util/Properties.java line 819:

...

817: * <p> 818: * If the {@systemProperty java.util.Properties.storeDate} is set and 819: * is non-empty (as determined by {@link String#isEmpty() String.isEmpty}), "is set **on the command line** and non-empty"...

Following from a comment on the CSR, it should be clear that the property value used can only be set on the command line. This is a clever way to detect whether the `entrySet()` method has been overridden to return something other than the entry-set provided by the Properties class... but I'm wondering if it's too clever. Does anyone who overrides entrySet() care about a specific order, or do they simply sort it in order to get reproducible output? If the latter, then sorting by default hasn't really broken anything.

Also, it was never specified that the properties are written based on what's returned by a self-call to `entrySet()`. So this was never guaranteed, though we do want to avoid gratuitous breakage.

I would also wager that anybody who overrides entrySet() so that they can control the ordering of the entries is probably breaking the contract of Map::entrySet, which says that it's mutable (a mapping can be removed by removing its entry from the entry-set, or the underlying value can be changed by calling setValue on an entry). This is admittedly pretty obscure, but it tells me that trying to customize the output of Properties::store by overriding entrySet() is a pretty fragile hack.

If people really need to control the order of output, they need to iterate the properties themselves instead of overriding entrySet(). I think the only difficulty in doing so is properly escaping the keys and values, as performed by saveConvert(). If this is a use case we want to support, then maybe we should expose a suitable API for escaping properties keys and values. That would be a separate enhancement, though.

Note some history in this Stack Overflow question and answers:

https://stackoverflow.com/questions/10275862/how-to-sort-properties-in-java

Basically they mostly describe overriding `keys()`, but we broke that in Java 9, and now the advice is to override `entrySet()`. But the goal is to sort the properties, which we're doing anyway! One part of what store() does is annoying to replicate, the encoding that `saveConvert` does is non-trivial. Other hacks based on returning a different entrySet might be to filter the set either keep some entries or ignore them. Both unlikely, but hacks are frequently fragile. And we've been very cautious in this discussion to avoid breaking things, in part, because there is so little info about how it is used.

To summarize the options that we have discussed for this entrySet() part: - Do nothing specific for subclasses that override entrySet() method. This would mean that the store() method would write out the properties in the natural sort order, but also has a tiny possibility that it will break backward compatibility if some code out there that was returning a differently ordered set. Given how we have tried to prevent backward compatibility issues in this PR plus the fact that we might have a possible way to prevent this, I think we can rule out this option. - Check the Properties object instance type to see if it has been subclassed. If yes, then don't store the properties in the natural sort order. I personally think we should rule this option out because this option prevents this enhancement from being usable by any subclass of Properties even if those subclasses do nothing related to entrySet() and could merely have been subclassed for completely unrelated things. - Detect that the entrySet() method is overridden by the subclass of Properties and might have done something with the returned instance/type. It's just a co-incidence that the Properties.entrySet() already returns a internal private class from that method. This does allow us to introduce a check to decide whether or not to use the new natural sort order for writing the properties. It relies on an internal knowledge plus the internal impl detail of the Property.entrySet() but, IMO, it might be good enough for us to use to be sure that we don't break backward compatibility and yet at the same time, introduce this natural sorted order by default for most of the subclasses out there. The other aspect to consider here is the future maintainability of such check that is being proposed here. What this check would effectively mean is that the implementation in Property.entrySet() and this store() method will have to be closely "held together" so that if we do change the implementation of Property.entrySet() to return something else at a later point, we would still have to return a type which is private to this Properties class (or some internal marker interface type) so that the store() methods can continue to employ a check like this one. IMO, that shouldn't be too hard to do if/when such a change happens in Properties.entrySet(). So I am in favour of this option to get past this entrySet() overridding issue. -Jaikiran

On Sat, 18 Sep 2021 03:52:17 GMT, Jaikiran Pai <jpai@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.htm... [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:

Roger's suggestion to reword the implSpec text

What would be the next step to move the CSR forward? Should I be changing it's status or do something else? ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

Thank you Joe. That link helped. I have now moved the CSR to the next state. -Jaikiran On 21/09/21 9:28 am, Joseph D. Darcy wrote:

Hello Jaikiran,

The CSR is in Draft state. As discussed in the CSR wiki (https://wiki.openjdk.java.net/display/csr/Main), the request needs to be moved by the assignee to either Finalized or Proposed state to request the CSR review the request.

HTH,

-Joe

On 9/20/2021 8:46 PM, Jaikiran Pai wrote:

...

On Sat, 18 Sep 2021 03:52:17 GMT, Jaikiran Pai <jpai@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.htm... [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:

   Roger's suggestion to reword the implSpec text What would be the next step to move the CSR forward? Should I be changing it's status or do something else?

-------------

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

On Sat, 18 Sep 2021 03:52:17 GMT, Jaikiran Pai <jpai@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.htm... [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:

Roger's suggestion to reword the implSpec text

src/java.base/share/classes/java/lang/System.java line 766:

764: * <tr><th scope="row">{@systemProperty java.properties.date}</th> 765: * <td>Text for the comment that must replace the default date comment 766: * written out by {@code Properties.store()} methods <em>(optional)</em> </td></tr>

To date, the table in getProperties has listed the supported system properties that the runtime makes available to applications. It hasn't historically listed the many other standard properties that can be set on the command line. So I'm sure about adding this one to the table as it opens the door to expanding the table to all the other system properties that are documented elsewhere in the API docs. Note that javadoc creates a table of system properties from usages of `@systemProperty` so there is already a more complete table in the javadoc build. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Wed, 22 Sep 2021 10:27:45 GMT, Jaikiran Pai <jpai@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.htm... [2] https://reproducible-builds.org/specs/source-date-epoch/

Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:

- Revert "Implement Mark's suggestion in CSR to include the java.properties.date in the list of system properties listed in System::getProperties()"

Additional inputs since this specific commit was introduced have leaned towards not listing this property in System::getProperties()

This reverts commit 458c1fd37cf1e8e8bd0a60a171d173ce5da12b7c. - reduce the line length of code comment

The CSR for this has been approved and there are no pending changes in this PR. I am thinking of integrating this PR in around 24 hours from now if this looks good. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372

On Wed, 22 Sep 2021 10:27:45 GMT, Jaikiran Pai <jpai@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.htm... [2] https://reproducible-builds.org/specs/source-date-epoch/

Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:

- Revert "Implement Mark's suggestion in CSR to include the java.properties.date in the list of system properties listed in System::getProperties()"

Additional inputs since this specific commit was introduced have leaned towards not listing this property in System::getProperties()

This reverts commit 458c1fd37cf1e8e8bd0a60a171d173ce5da12b7c. - reduce the line length of code comment

I agree with Roger. It's hard to understand the amount of work you have put into this when you only look at the small, resulting patch. Thank you for pulling this through! ------------- PR: https://git.openjdk.java.net/jdk/pull/5372