RFR: 8231640: (prop) Canonical property storage
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/ ------------- Commit messages: - 8231640: (prop) Canonical property storage Changes: https://git.openjdk.java.net/jdk/pull/5372/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8231640 Stats: 667 lines in 5 files changed: 663 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
On Sat, 4 Sep 2021 15:25:59 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/
I haven't yet created a CSR for this and will do that next week once the initial reviews and changes are sorted out. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372
On Sat, 4 Sep 2021 15:25:59 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/
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 to array and then sorting should be faster, that creating TreeMap ------------- PR: https://git.openjdk.java.net/jdk/pull/5372
Hello Andrey, On 05/09/21 12:02 am, Andrey Turbanov wrote:
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 to array and then sorting should be faster, that creating TreeMap
Yes, the ordering of the properties is intentional as noted in the description of this PR as well as the linked mail discussion thread. As for the usage of TreeMap, I had looked around the JDK code to see if there are more performant ways to order that existing Map, but I couldn't find any. 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? I can experiment with it in a simple JMH benchmark with some decent/regular sized Map and see how it performs. -Jaikiran
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
Hello Andrey, On 07/09/21 7:50 pm, Andrey Turbanov wrote:
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.
I experimented this in a JMH test and the results matched your expectations. So, I've updated the PR to use array sorting instead of creating a TreeMap. For reference, here's the JMH benchmark code and the results: package org.myapp; import org.openjdk.jmh.annotations.Benchmark; import java.util.*; import java.util.concurrent.*; import org.openjdk.jmh.annotations.*; public class MyBenchmark { @State(Scope.Thread) public static class TestData { static final Map<Object, Object> tenItems; static final Map<Object, Object> hundredItems; static final Map<Object, Object> thousandItems; static { tenItems = new ConcurrentHashMap<>(8); hundredItems = new ConcurrentHashMap<>(8); thousandItems = new ConcurrentHashMap<>(8); for (int i = 0; i < 1000; i++) { thousandItems.put("foo" + i, "bar"); if (i < 100) { hundredItems.put("hello" + i, "world"); } if (i < 10) { tenItems.put("good" + i, "morning"); } } System.out.println("Test data created with " + tenItems.size() + ", " + hundredItems.size() + " and " + thousandItems.size() + " Map keys"); } } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testTenItemsTreeMapSorting(TestData testData) { final Map<Object, Object> sorted = new TreeMap(testData.tenItems); } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testHundredItemsTreeMapSorting(TestData testData) { final Map<Object, Object> sorted = new TreeMap(testData.hundredItems); } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testThousandItemsTreeMapSorting(TestData testData) { final Map<Object, Object> sorted = new TreeMap(testData.thousandItems); } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testTenItemsArraySorting(TestData testData) { var entries = testData.tenItems.entrySet().toArray(new Map.Entry<?, ?>[0]); Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() { @Override public int compare(Map.Entry<?, ?> o1, Map.Entry<?, ?> o2) { return ((String) o1.getKey()).compareTo((String) o2.getKey()); } }); } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testHundredItemsArraySorting(TestData testData) { var entries = testData.hundredItems.entrySet().toArray(new Map.Entry<?, ?>[0]); Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() { @Override public int compare(Map.Entry<?, ?> o1, Map.Entry<?, ?> o2) { return ((String) o1.getKey()).compareTo((String) o2.getKey()); } }); } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testThousandItemsArraySorting(TestData testData) { var entries = testData.thousandItems.entrySet().toArray(new Map.Entry<?, ?>[0]); Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() { @Override public int compare(Map.Entry<?, ?> o1, Map.Entry<?, ?> o2) { return ((String) o1.getKey()).compareTo((String) o2.getKey()); } }); } } Results: Benchmark Mode Cnt Score Error Units MyBenchmark.testHundredItemsArraySorting avgt 25 8.330 ± 0.147 us/op MyBenchmark.testHundredItemsTreeMapSorting avgt 25 8.637 ± 0.333 us/op MyBenchmark.testTenItemsArraySorting avgt 25 0.261 ± 0.006 us/op MyBenchmark.testTenItemsTreeMapSorting avgt 25 0.422 ± 0.007 us/op MyBenchmark.testThousandItemsArraySorting avgt 25 151.566 ± 1.660 us/op MyBenchmark.testThousandItemsTreeMapSorting avgt 25 163.767 ± 1.911 us/op -Jaikiran
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
Hello Robert, On 07/09/21 11:24 pm, Robert Scholte wrote:
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.
As discussed in the mailing list, it is agreed upon that these property keys will be oredered when they are written out by the store() APIs. Thus providing reproducibility. However, the order will not be the insertion order, instead it will be the natural order of the property keys and this order will only be applicable/maintained when using the store() APIs. Trying to store them in a original input order will be a much bigger change and won't just be applicable for the store() APIs but the entire internal implementation of the Properties class itself. -Jaikiran
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.
The SM case probably needs discussion as I was expecting to see something like this: PrivilegedAction<String> pa = () -> System.getenv("SOURCE_DATE_EPOCH"); String value = AccessController.doPrivileged(pa); as a caller of the store method (in a library for example) is unlikely to have this permission. I assume your concern is that untrusted code would have a way to read this specific env variable without a permission (by way of creating and storing an empty Properties)? In this case I'm mostly wondering if it's really worth having a behavior difference in this mode.
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 store methods are already specified to throw CCE if the properties has non-String keys so this should be okay. -Alan
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?
The SM case probably needs discussion as I was expecting to see something like this:
PrivilegedAction<String> pa = () -> System.getenv("SOURCE_DATE_EPOCH"); String value = AccessController.doPrivileged(pa);
as a caller of the store method (in a library for example) is unlikely to have this permission. I assume your concern is that untrusted code would have a way to read this specific env variable without a permission (by way of creating and storing an empty Properties)? In this case I'm mostly wondering if it's really worth having a behavior difference in this mode.
I had initially started off with using a doPrivileged code in this change. However, I soon realized that it might be a security hole, like in the example you state. Unlike other parts of the JDK code where the doPrivileged makes sense, since the "action" part of it does things that are internal implementation details to the JDK, this new piece of code that we are introducing, does a System.getenv(...) in its "action" but will then additionally hand out the value (in a formatted manner of course) of that environment variable to the callers, through the OutputStream/Writer instances that the callers pass in to the store() APIs. Effectively, this means that even when getenv.SOURCE_DATE_EPOCH (or getenv.* for that matter) haven't been granted to the callers, they will _always_ be able to get hold of that environment variable's value through this approach. Like you state, they could just call Properties.store() (which by the way, doesn't necessarily have to be empty) to bypass the security checks that are put in place for the direct System.getenv(...) calls from their code. In such a doPrivelged case, the only way the administrator can prevent this security bypass, would then be to _not_ grant this permission to the JDK code itself, which then effectively means that this enhancement for using SOURCE_DATE_EPOCH will not take effect at all and the date comment will always write out the current date. So the doPriveleged case, IMO, will not allow selective granting of permissions to callers. The alternate approach that's used in this PR, on the other hand allows for selective granting of permissions. I'm not too aware of how things stand with the application server and security manager integration these days, but I would guess that in such use cases where you probably would want to control which deployed applications (within the same JVM) are granted what permissions, this alternate approach of not using the doPriveleged would make it feasible to control these permissions. -Jaikiran
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
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. 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. Thanks, Roger On 9/5/21 8:31 AM, 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?
The SM case probably needs discussion as I was expecting to see something like this:
PrivilegedAction<String> pa = () -> System.getenv("SOURCE_DATE_EPOCH"); String value = AccessController.doPrivileged(pa);
as a caller of the store method (in a library for example) is unlikely to have this permission. I assume your concern is that untrusted code would have a way to read this specific env variable without a permission (by way of creating and storing an empty Properties)? In this case I'm mostly wondering if it's really worth having a behavior difference in this mode.
I had initially started off with using a doPrivileged code in this change. However, I soon realized that it might be a security hole, like in the example you state. Unlike other parts of the JDK code where the doPrivileged makes sense, since the "action" part of it does things that are internal implementation details to the JDK, this new piece of code that we are introducing, does a System.getenv(...) in its "action" but will then additionally hand out the value (in a formatted manner of course) of that environment variable to the callers, through the OutputStream/Writer instances that the callers pass in to the store() APIs. Effectively, this means that even when getenv.SOURCE_DATE_EPOCH (or getenv.* for that matter) haven't been granted to the callers, they will _always_ be able to get hold of that environment variable's value through this approach. Like you state, they could just call Properties.store() (which by the way, doesn't necessarily have to be empty) to bypass the security checks that are put in place for the direct System.getenv(...) calls from their code.
In such a doPrivelged case, the only way the administrator can prevent this security bypass, would then be to _not_ grant this permission to the JDK code itself, which then effectively means that this enhancement for using SOURCE_DATE_EPOCH will not take effect at all and the date comment will always write out the current date.
So the doPriveleged case, IMO, will not allow selective granting of permissions to callers. The alternate approach that's used in this PR, on the other hand allows for selective granting of permissions. I'm not too aware of how things stand with the application server and security manager integration these days, but I would guess that in such use cases where you probably would want to control which deployed applications (within the same JVM) are granted what permissions, this alternate approach of not using the doPriveleged would make it feasible to control these permissions.
-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
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... (See JDK-8272157 for the journey that led us here, in particular, lack of doPrivileged when reading the environment. The Debian patch also doesn't provide a reproducible order, which we've already agreed is necessary.) s'marks
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
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. s'marks
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/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
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 Sat, 4 Sep 2021 15:25:59 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/
Off topic - the `tier1` failures that are being reported in GitHub action jobs seem genuine but unrelated to this change. 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. Here's the error from the logs: 2021-09-04T16:37:24.7528210Z Running test testUniqueInfoOpts 2021-09-04T16:37:24.7630470Z Main [--help, --help] [] 2021-09-04T16:37:24.7635480Z rc:0 2021-09-04T16:37:24.7736940Z javac/DIRECT: 2021-09-04T16:37:24.7762440Z Usage: javac <options> <source files> 2021-09-04T16:37:24.7863980Z where possible options include: 2021-09-04T16:37:24.7892480Z @<filename> Read options and filenames from file 2021-09-04T16:37:24.7986700Z -Akey[=value] Options to pass to annotation processors 2021-09-04T16:37:24.8003100Z --add-modules <module>(,<module>)* 2021-09-04T16:37:24.8030720Z Root modules to resolve in addition to the initial modules, or all modules 2021-09-04T16:37:24.8106550Z on the module path if <module> is ALL-MODULE-PATH. 2021-09-04T16:37:24.8108330Z --boot-class-path <path>, -bootclasspath <path> 2021-09-04T16:37:24.8110830Z Override location of bootstrap class files 2021-09-04T16:37:24.8112470Z --class-path <path>, -classpath <path>, -cp <path> 2021-09-04T16:37:24.8113590Z Specify where to find user class files and annotation processors 2021-09-04T16:37:24.8115140Z -d <directory> Specify where to place generated class files 2021-09-04T16:37:24.8116530Z -deprecation 2021-09-04T16:37:24.8117490Z Output source locations where deprecated APIs are used 2021-09-04T16:37:24.8118770Z --enable-preview 2021-09-04T16:37:24.8120330Z Enable preview language features. To be used in conjunction with either -source or --release. 2021-09-04T16:37:24.8129810Z -encoding <encoding> Specify character encoding used by source files 2021-09-04T16:37:24.8136440Z -endorseddirs <dirs> Override location of endorsed standards path 2021-09-04T16:37:24.8239190Z -extdirs <dirs> Override location of installed extensions 2021-09-04T16:37:24.8251400Z -g Generate all debugging info 2021-09-04T16:37:24.8290250Z -g:{lines,vars,source} Generate only some debugging info 2021-09-04T16:37:24.8291910Z -g:none Generate no debugging info 2021-09-04T16:37:24.8293200Z -h <directory> 2021-09-04T16:37:24.8294170Z Specify where to place generated native header files 2021-09-04T16:37:24.8296270Z --help, -help, -? Print this help message 2021-09-04T16:37:24.8297720Z --help-extra, -X Print help on extra options 2021-09-04T16:37:24.8299160Z -implicit:{none,class} 2021-09-04T16:37:24.8300250Z Specify whether or not to generate class files for implicitly referenced files 2021-09-04T16:37:24.8301820Z -J<flag> Pass <flag> directly to the runtime system 2021-09-04T16:37:24.8303230Z --limit-modules <module>(,<module>)* 2021-09-04T16:37:24.8304210Z Limit the universe of observable modules 2021-09-04T16:37:24.8305570Z --module <module>(,<module>)*, -m <module>(,<module>)* 2021-09-04T16:37:24.8306630Z Compile only the specified module(s), check timestamps 2021-09-04T16:37:24.8307980Z --module-path <path>, -p <path> 2021-09-04T16:37:24.8309050Z Specify where to find application modules 2021-09-04T16:37:24.8310500Z --module-source-path <module-source-path> 2021-09-04T16:37:24.8311660Z Specify where to find input source files for multiple modules 2021-09-04T16:37:24.8313050Z --module-version <version> 2021-09-04T16:37:24.8314040Z Specify version of modules that are being compiled 2021-09-04T16:37:24.8315390Z -nowarn Generate no warnings 2021-09-04T16:37:24.8316640Z -parameters 2021-09-04T16:37:24.8317620Z Generate metadata for reflection on method parameters 2021-09-04T16:37:24.8361300Z -proc:{none,only} 2021-09-04T16:37:24.8362510Z Control whether annotation processing and/or compilation is done. 2021-09-04T16:37:24.8364150Z -processor <class1>[,<class2>,<class3>...] 2021-09-04T16:37:24.8365410Z Names of the annotation processors to run; bypasses default discovery process 2021-09-04T16:37:24.8366910Z --processor-module-path <path> 2021-09-04T16:37:24.8367990Z Specify a module path where to find annotation processors 2021-09-04T16:37:24.8369540Z --processor-path <path>, -processorpath <path> 2021-09-04T16:37:24.8370620Z Specify where to find annotation processors 2021-09-04T16:37:24.8371840Z -profile <profile> 2021-09-04T16:37:24.8372800Z Check that API used is available in the specified profile 2021-09-04T16:37:24.8374080Z --release <release> 2021-09-04T16:37:24.8375110Z Compile for the specified Java SE release. Supported releases: 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18 2021-09-04T16:37:24.8377060Z -s <directory> Specify where to place generated source files 2021-09-04T16:37:24.8378940Z --source <release>, -source <release> 2021-09-04T16:37:24.8380150Z Provide source compatibility with the specified Java SE release. Supported releases: 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18 2021-09-04T16:37:24.8381690Z --source-path <path>, -sourcepath <path> 2021-09-04T16:37:24.8382670Z Specify where to find input source files 2021-09-04T16:37:24.8383990Z --system <jdk>|none Override location of system modules 2021-09-04T16:37:24.8385370Z --target <release>, -target <release> 2021-09-04T16:37:24.8386570Z Generate class files suitable for the specified Java SE release. Supported releases: 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18 2021-09-04T16:37:24.8388070Z --upgrade-module-path <path> 2021-09-04T16:37:24.8389040Z Override location of upgradeable modules 2021-09-04T16:37:24.8390480Z -verbose Output messages about what the compiler is doing 2021-09-04T16:37:24.8391930Z --version, -version Version information 2021-09-04T16:37:24.8393380Z -Werror Terminate compilation if warnings occur 2021-09-04T16:37:24.8394150Z 2021-09-04T16:37:24.8395160Z Main [-X, -X] [] 2021-09-04T16:37:24.8395860Z rc:0 2021-09-04T16:37:24.8396520Z javac/DIRECT: 2021-09-04T16:37:24.8397830Z --add-exports <module>/<package>=<other-module>(,<other-module>)* 2021-09-04T16:37:24.8399140Z Specify a package to be considered as exported from its defining module 2021-09-04T16:37:24.8400780Z to additional modules, or to all unnamed modules if <other-module> is ALL-UNNAMED. 2021-09-04T16:37:24.8403050Z --add-reads <module>=<other-module>(,<other-module>)* 2021-09-04T16:37:24.8404200Z Specify additional modules to be considered as required by a given module. 2021-09-04T16:37:24.8405750Z <other-module> may be ALL-UNNAMED to require the unnamed module. 2021-09-04T16:37:24.8407430Z --default-module-for-created-files <module-name> 2021-09-04T16:37:24.8408890Z Fallback target module for files created by annotation processors, if none specified or inferred. 2021-09-04T16:37:24.8410640Z -Djava.endorsed.dirs=<dirs> Override location of endorsed standards path 2021-09-04T16:37:24.8412280Z -Djava.ext.dirs=<dirs> Override location of installed extensions 2021-09-04T16:37:24.8413820Z --help-lint Print the supported keys for -Xlint 2021-09-04T16:37:24.8415200Z --patch-module <module>=<file>(:<file>)* 2021-09-04T16:37:24.8416210Z Override or augment a module with classes and resources 2021-09-04T16:37:24.8417110Z in JAR files or directories 2021-09-04T16:37:24.8418510Z -Xbootclasspath:<path> Override location of bootstrap class files 2021-09-04T16:37:24.8420080Z -Xbootclasspath/a:<path> Append to the bootstrap class path 2021-09-04T16:37:24.8421630Z -Xbootclasspath/p:<path> Prepend to the bootstrap class path 2021-09-04T16:37:24.8423170Z -Xdiags:{compact,verbose} Select a diagnostic mode 2021-09-04T16:37:24.8424430Z -Xdoclint 2021-09-04T16:37:24.8425370Z Enable recommended checks for problems in javadoc comments 2021-09-04T16:37:24.8426730Z -Xdoclint:(all|none|[-]<group>)[/<access>] 2021-09-04T16:37:24.8427770Z Enable or disable specific checks for problems in javadoc comments, 2021-09-04T16:37:24.8428920Z where <group> is one of accessibility, html, missing, reference, or syntax, 2021-09-04T16:37:24.8430170Z and <access> is one of public, protected, package, or private. 2021-09-04T16:37:24.8431760Z -Xdoclint/package:[-]<packages>(,[-]<package>)* 2021-09-04T16:37:24.8432880Z Enable or disable checks in specific packages. Each <package> is either the 2021-09-04T16:37:24.8434310Z qualified name of a package or a package name prefix followed by .*, which 2021-09-04T16:37:24.8435940Z expands to all sub-packages of the given package. Each <package> can be prefixed 2021-09-04T16:37:24.8438060Z with - to disable checks for the specified package or packages. 2021-09-04T16:37:24.8439610Z -Xlint Enable recommended warnings 2021-09-04T16:37:24.8440840Z -Xlint:<key>(,<key>)* 2021-09-04T16:37:24.8441750Z Warnings to enable or disable, separated by comma. 2021-09-04T16:37:24.8443150Z Precede a key by - to disable the specified warning. 2021-09-04T16:37:24.8444570Z Use --help-lint to see the supported keys. 2021-09-04T16:37:24.8446010Z -Xmaxerrs <number> Set the maximum number of errors to print 2021-09-04T16:37:24.8447550Z -Xmaxwarns <number> Set the maximum number of warnings to print 2021-09-04T16:37:24.8550380Z -Xpkginfo:{always,legacy,nonempty} 2021-09-04T16:37:24.8636310Z Specify handling of package-info files 2021-09-04T16:37:24.8639600Z -Xplugin:"name args" 2021-09-04T16:37:24.8663470Z Name and optional arguments for a plug-in to be run 2021-09-04T16:37:24.8665480Z -Xprefer:{source,newer} 2021-09-04T16:37:24.8683700Z Specify which file to read when both a source file and class file are found for an implicitly compiled class 2021-09-04T16:37:24.8728280Z -Xprint 2021-09-04T16:37:24.8729520Z Print out a textual representation of specified types 2021-09-04T16:37:24.8730900Z -XprintProcessorInfo 2021-09-04T16:37:24.8731960Z Print information about which annotations a processor is asked to process 2021-09-04T16:37:24.8733270Z -XprintRounds 2021-09-04T16:37:24.8734200Z Print information about rounds of annotation processing 2021-09-04T16:37:24.8736230Z -Xstdout <filename> Redirect standard output 2021-09-04T16:37:24.8736940Z 2021-09-04T16:37:24.8737760Z These extra options are subject to change without notice. 2021-09-04T16:37:24.8739170Z Main [--help-lint, --help-lint] [] 2021-09-04T16:37:24.8739960Z rc:0 2021-09-04T16:37:24.8740890Z javac/DIRECT: 2021-09-04T16:37:24.8742040Z The supported keys for -Xlint are: 2021-09-04T16:37:24.8742920Z all Enable all warnings 2021-09-04T16:37:24.8743980Z auxiliaryclass Warn about an auxiliary class that is hidden in a source file, and is used from other files. 2021-09-04T16:37:24.8744800Z cast Warn about use of unnecessary casts. 2021-09-04T16:37:24.8745500Z classfile Warn about issues related to classfile contents. 2021-09-04T16:37:24.8746200Z deprecation Warn about use of deprecated items. 2021-09-04T16:37:24.8747550Z dep-ann Warn about items marked as deprecated in JavaDoc but not using the @Deprecated annotation. 2021-09-04T16:37:24.8748410Z divzero Warn about division by constant integer 0. 2021-09-04T16:37:24.8749030Z empty Warn about empty statement after if. 2021-09-04T16:37:24.8749680Z exports Warn about issues regarding module exports. 2021-09-04T16:37:24.8750470Z fallthrough Warn about falling through from one case of a switch statement to the next. 2021-09-04T16:37:24.8751300Z finally Warn about finally clauses that do not terminate normally. 2021-09-04T16:37:24.8752780Z missing-explicit-ctor Warn about missing explicit constructors in public and protected classes in exported packages. 2021-09-04T16:37:24.8753780Z module Warn about module system related issues. 2021-09-04T16:37:24.8754410Z opens Warn about issues regarding module opens. 2021-09-04T16:37:24.8755110Z options Warn about issues relating to use of command line options. 2021-09-04T16:37:24.8755850Z overloads Warn about issues regarding method overloads. 2021-09-04T16:37:24.8756560Z overrides Warn about issues regarding method overrides. 2021-09-04T16:37:24.8757270Z path Warn about invalid path elements on the command line. 2021-09-04T16:37:24.8758010Z processing Warn about issues regarding annotation processing. 2021-09-04T16:37:24.8759040Z rawtypes Warn about use of raw types. 2021-09-04T16:37:24.8759730Z removal Warn about use of API that has been marked for removal. 2021-09-04T16:37:24.8761030Z requires-automatic Warn about use of automatic modules in the requires clauses. 2021-09-04T16:37:24.8762640Z requires-transitive-automatic Warn about automatic modules in requires transitive. 2021-09-04T16:37:24.8763680Z serial Warn about Serializable classes that do not provide a serial version ID. 2021-09-04T16:37:24.8765050Z Also warn about access to non-public members from a serializable element. 2021-09-04T16:37:24.8765870Z static Warn about accessing a static member using an instance. 2021-09-04T16:37:24.8766610Z strictfp Warn about unnecessary use of the strictfp modifier. 2021-09-04T16:37:24.8767930Z synchronization Warn about synchronization attempts on instances of value-based classes. 2021-09-04T16:37:24.8769460Z text-blocks Warn about inconsistent white space characters in text block indentation. 2021-09-04T16:37:24.8770810Z try Warn about issues relating to use of try blocks (i.e. try-with-resources). 2021-09-04T16:37:24.8771580Z unchecked Warn about unchecked operations. 2021-09-04T16:37:24.8772250Z varargs Warn about potentially unsafe vararg methods. 2021-09-04T16:37:24.8772950Z preview Warn about use of preview language features. 2021-09-04T16:37:24.8773540Z none Disable all warnings 2021-09-04T16:37:24.8774850Z Main [-version, -version] [] 2021-09-04T16:37:24.8775370Z rc:0 2021-09-04T16:37:24.8775690Z javac/DIRECT: 2021-09-04T16:37:24.8776460Z javac 18-internal 2021-09-04T16:37:24.8777290Z Main [-fullversion, -fullversion] [] 2021-09-04T16:37:24.8777740Z rc:0 2021-09-04T16:37:24.8778060Z javac/DIRECT: 2021-09-04T16:37:24.8779450Z javac full version "18-internal+0-jaikiran-85748cf4a8efb69cbe69667851a14321804a51b6" 2021-09-04T16:37:24.8780590Z >>>>> Expected string appears more than once: 18 2021-09-04T16:37:24.8780950Z 2021-09-04T16:37:24.8781400Z java.lang.Exception: 1 errors occurred 2021-09-04T16:37:24.8782230Z at OptionModesTester.runTests(OptionModesTester.java:81) 2021-09-04T16:37:24.8783100Z at InfoOptsTest.main(InfoOptsTest.java:41) 2021-09-04T16:37:24.8784350Z at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) 2021-09-04T16:37:24.8786310Z at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) 2021-09-04T16:37:24.8788620Z at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) 2021-09-04T16:37:24.8791250Z at java.base/java.lang.reflect.Method.invoke(Method.java:568) 2021-09-04T16:37:24.8792700Z at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:312) 2021-09-04T16:37:24.8794000Z at java.base/java.lang.Thread.run(Thread.java:833) 2021-09-04T16:37:24.8794420Z 2021-09-04T16:37:24.8794980Z JavaTest Message: Test threw exception: java.lang.Exception 2021-09-04T16:37:24.8795660Z JavaTest Message: shutting down test 2021-09-04T16:37:24.8795990Z 2021-09-04T16:37:24.8796190Z 2021-09-04T16:37:24.8797640Z TEST RESULT: Failed. Execution failed: `main' threw exception: java.lang.Exception: 1 errors occurred ------------- PR: https://git.openjdk.java.net/jdk/pull/5372
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
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: use @implNote to explain the use of the environment variable ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/85748cf4..641864e2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=00-01 Stats: 19 lines in 1 file changed: 10 ins; 9 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
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 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/641864e2..1ded17f9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=01-02 Stats: 225 lines in 4 files changed: 73 ins; 119 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 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 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. ------------- 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 09/09/21 12:51 am, Stuart Marks wrote:
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.
Good catch about the doPriveleged block currently using the lambda. I had overlooked that part.
Assuming we're ok with lambdas, it might be easier to use collections instead of arrays in order to preserve generic types.
That usage of lambda in doPriveleged hasn't caused any issues (due to the lazy/delayed implementation this PR uses to parse the environment variable). So I think using lambdas in the store() implementation should be fine then.
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());
Thank you for this snippet (learned something new :)). I've now updated the PR to use this version instead of the Arrays.sort(...) one. I've rerun a modified JMH benchmark, comparing this version with the Arrays.sort(...) version and this one performs slightly better too, so performance shouldn't be a concern here. package org.myapp; import org.openjdk.jmh.annotations.Benchmark; import java.util.*; import java.util.concurrent.*; import org.openjdk.jmh.annotations.*; public class MyBenchmark { @State(Scope.Thread) public static class TestData { static final Map<Object, Object> tenItems; static final Map<Object, Object> hundredItems; static final Map<Object, Object> thousandItems; static { tenItems = new ConcurrentHashMap<>(8); hundredItems = new ConcurrentHashMap<>(8); thousandItems = new ConcurrentHashMap<>(8); for (int i = 0; i < 1000; i++) { thousandItems.put("foo" + i, "bar"); if (i < 100) { hundredItems.put("hello" + i, "world"); } if (i < 10) { tenItems.put("good" + i, "morning"); } } System.out.println("Test data created with " + tenItems.size() + ", " + hundredItems.size() + " and " + thousandItems.size() + " Map keys"); } } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testTenItemsCollectionSorting(TestData testData) { @SuppressWarnings("unchecked") var entries = new ArrayList<>(((Map<String, String>) (Map) testData.tenItems).entrySet()); entries.sort(Map.Entry.comparingByKey()); } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testHundredItemsCollectionSorting(TestData testData) { @SuppressWarnings("unchecked") var entries = new ArrayList<>(((Map<String, String>) (Map) testData.hundredItems).entrySet()); entries.sort(Map.Entry.comparingByKey()); } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testThousandItemsCollectionSorting(TestData testData) { @SuppressWarnings("unchecked") var entries = new ArrayList<>(((Map<String, String>) (Map) testData.thousandItems).entrySet()); entries.sort(Map.Entry.comparingByKey()); } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testTenItemsArraySorting(TestData testData) { var entries = testData.tenItems.entrySet().toArray(new Map.Entry<?, ?>[0]); Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() { @Override public int compare(Map.Entry<?, ?> o1, Map.Entry<?, ?> o2) { return ((String) o1.getKey()).compareTo((String) o2.getKey()); } }); } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testHundredItemsArraySorting(TestData testData) { var entries = testData.hundredItems.entrySet().toArray(new Map.Entry<?, ?>[0]); Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() { @Override public int compare(Map.Entry<?, ?> o1, Map.Entry<?, ?> o2) { return ((String) o1.getKey()).compareTo((String) o2.getKey()); } }); } @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) public void testThousandItemsArraySorting(TestData testData) { var entries = testData.thousandItems.entrySet().toArray(new Map.Entry<?, ?>[0]); Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() { @Override public int compare(Map.Entry<?, ?> o1, Map.Entry<?, ?> o2) { return ((String) o1.getKey()).compareTo((String) o2.getKey()); } }); } } Benchmark Mode Cnt Score Error Units MyBenchmark.testHundredItemsArraySorting avgt 25 8.375 ± 0.119 us/op MyBenchmark.testHundredItemsCollectionSorting avgt 25 7.608 ± 0.118 us/op MyBenchmark.testTenItemsArraySorting avgt 25 0.261 ± 0.004 us/op MyBenchmark.testTenItemsCollectionSorting avgt 25 0.234 ± 0.004 us/op MyBenchmark.testThousandItemsArraySorting avgt 25 150.934 ± 2.865 us/op MyBenchmark.testThousandItemsCollectionSorting avgt 25 149.356 ± 4.381 us/op -Jaikiran
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
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 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/1ded17f9..867ec999 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=02-03 Stats: 5 lines in 1 file changed: 0 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 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
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 three additional commits since the last revision: - Implement Stuart's suggestion to use Collections instead of Arrays for ordering property keys - update the new tests to match the new date format expectations and also print out some log messages for better debuggability of the tests - Implement Stuart's suggestion on date format: - use the same format for both when writing current date as well as when SOURCE_DATE_EPOCH is set - the format matches the one used in java.util.Date.toString() to preserve backward compatibility - when SOURCE_DATE_EPOCH is set, although the format is the same, the timezone is fixed to UTC and locale is set to ROOT to provide reproducibility of the generated comment ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/867ec999..7736a8f2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=03-04 Stats: 30 lines in 3 files changed: 9 ins; 5 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
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 to match latest changes ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/7736a8f2..c9d3cb8f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=04-05 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
On Thu, 9 Sep 2021 05:46:34 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 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.... ------------- 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
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) ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/c9d3cb8f..a29d0f08 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 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
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
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 10/09/2021 08:01, Robert Scholte wrote:
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?
Properties is a Map so anyone can write the mappings out in any format they want. The properties format that the store methods are specified to write involve escaping characters that aren't in Latin-1 so a bit tricky, but not impossible. The storeToXML method is unfortunate but we have an implementation in java.base so all good. A number of us discussed this topic yesterday and converged on change the existing store methods to have a standard property to configure the timestamp. Separately, we can explore a 1-arg store method that does not write any comments. There is more on the PR on this. -Alan
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 Thu, 9 Sep 2021 18:31:43 GMT, Roger Riggs <rriggs@openjdk.org> wrote:
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)
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.
"different set implementation" as part of the spec may challenge the compatibility test developers to prove or disprove that statement. The type of an instance is frequently understood to be the "implementation". The visible type returned from Properties.entrySet() is SynchronizedSet. Anyone can create one of those. That statement might mislead a subclass into thinking they can not/must not return a SynchronizedSet if they want the built-in sorting. I'm thinking that wording it in other term of the subclass might be better: "...sort order of the keys in the entrySet() unless entrySet() is overridden by a subclass to return a different value than 'super.entrySet'(). " The existing implementation is fine. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372
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: Implement Roger's suggestions: - Rely on java.util.Properties.storeDate system property instead of SOURCE_DATE_EPOCH environment variable - No need for caching the parsed date - Update the javadoc to match new expectations - Update tests to match the new expectations ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/a29d0f08..a9b71d2f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=06-07 Stats: 149 lines in 4 files changed: 50 ins; 39 del; 60 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
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 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/a9b71d2f..06ff3bdb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=07-08 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
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
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:
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.
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
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
Based on these inputs, I have now updated the PR to use the value of this new system property verbatim while writing out the comment. I realize you mentioned that we should use this value if it is non-null and non-empty. In my updated PR, I instead use this value only if it is non-null and non-blank. I based it on my opinion that whitespace-only comment through this system property, shouldn't be allowed. If however you and others think that using non-empty check instead of non-blank check is more appropriate, feel free to let me know and I'll update the PR accordingly. The new tests have been updated accordingly and they continue to pass along with the existing ones in test/jdk/java/util/Properties/. -Jaikiran On 10/09/21 8:30 pm, Roger Riggs wrote:
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
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 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/06ff3bdb..1d24a3a8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=08-09 Stats: 94 lines in 2 files changed: 32 ins; 36 del; 26 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 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
Hello Stuart, On 10/09/21 11:12 pm, Stuart Marks wrote:
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.
Done. I've updated the PR to use this text in the javadoc.
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. -Jaikiran
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
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. -Jaikiran On 13/09/21 7:13 pm, Roger Riggs wrote:
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
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 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/1d24a3a8..c1dfb18f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=09-10 Stats: 17 lines in 1 file changed: 6 ins; 3 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
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
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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 16 additional commits since the last revision: - Merge latest from master branch - Implement Roger's and the agreed upon decision to allow free-form text for the system property. Plus updates to test to match the expectations - Update javadoc based on Stuart's inputs - allow free-form text for java.util.Properties.storeDate system property - jcheck fix - trailing whitespace in test - Implement Roger's suggestions: - Rely on java.util.Properties.storeDate system property instead of SOURCE_DATE_EPOCH environment variable - No need for caching the parsed date - Update the javadoc to match new expectations - Update tests to match the new expectations - 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) - update javadoc to match latest changes - Implement Stuart's suggestion to use Collections instead of Arrays for ordering property keys - update the new tests to match the new date format expectations and also print out some log messages for better debuggability of the tests - ... and 6 more: https://git.openjdk.java.net/jdk/compare/dae096b0...6447f9bb ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/c1dfb18f..6447f9bb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=10-11 Stats: 10450 lines in 483 files changed: 6216 ins; 2542 del; 1692 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
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 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/6447f9bb..92374664 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=11-12 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 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 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! ------------- 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 11:59:56 GMT, Jaikiran Pai <jpai@openjdk.org> wrote:
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?
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 ;-) ------------- 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 13:15:18 GMT, Jaikiran Pai <jpai@openjdk.org> wrote:
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.
Thanks Jaikiran, - the `@implNote` looks good to me. ------------- 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 11:43:25 GMT, Magnus Ihse Bursie <ihse@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 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)
Hello Magnus,
Maybe you should refer to how the comment is written out above, since you use the same method.
I've updated the javadoc to capture this part of the detail.
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)
The new tests that were added in this PR has one specific test to verify how line terminators are dealt with, if the system property value has them https://github.com/openjdk/jdk/pull/5372/files#diff-220f7bcc6d1a7ec33764f81e... ------------- 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
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 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/92374664..14711a92 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=12-13 Stats: 12 lines in 1 file changed: 5 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 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:17:42 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:
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.
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. ------------- 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 13:28:24 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:
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.
Done. A new test `StoreReproducibilityTest#testBackSlashInStoreDateValue` has been added in the latest updated version of this PR. This test passes. Plus I checked the written out properties file, from these tests, for such values in `java.util.Properties.storeDate` and the content matches what the spec says. Just for quick reference - a run of that test case with the "newline-plus-backslash...." system property value (cannot paste that exact string value from that test case because GitHub editor is messing up the special characters) generates output like below: #some user specified comment #newline-plus-backslash\ #c=d a=b ------------- PR: https://git.openjdk.java.net/jdk/pull/5372
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. ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/14711a92..6f5f1be2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=13-14 Stats: 65 lines in 1 file changed: 64 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 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.
Marked as reviewed by smarks (Reviewer). OK, after all the twists and turns, this looks good! src/java.base/share/classes/java/util/Properties.java line 173:
171: // used to format the date comment written out by the store() APIs. 172: // This format matches the one used by java.util.Date.toString() 173: private static final String dateFormatPattern = "EEE MMM dd HH:mm:ss zzz yyyy";
I'd rename this something like DATE_FORMAT_PATTERN to conform to naming conventions for constants. ------------- 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 18:11:00 GMT, Roger Riggs <rriggs@openjdk.org> wrote:
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.
Done. I pushed an update to the PR which switches back to using Date.toString() for the date comment. It also does a minor adjustment to the javadoc to clarify this behaviour.
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.)
Done. I've updated the tests to use a consistent API for parsing these date comments in the stored files. They now use the DateTimeFormatter APIs along with the right pattern ("uuuu") to match the output of Date.toString(). ------------- 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 18:53:27 GMT, Roger Riggs <rriggs@openjdk.org> wrote:
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?
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? ------------- 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 Tue, 14 Sep 2021 21:09:34 GMT, Roger Riggs <rriggs@openjdk.org> wrote:
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.
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. ------------- 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
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: Address review suggestions: - Switch back to using Date.toString() for writing out the date comment - Update the javadoc to match this change - Update the tests to consistently use DateTimeFormatter while parsing dates ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/6f5f1be2..7098a2c5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=14-15 Stats: 22 lines in 3 files changed: 2 ins; 8 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
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: - Clarify how overriden Properties#entrySet() method impacts the order of stored properties - Tests to verify subclasses of Properties ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/7098a2c5..79d10527 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=15-16 Stats: 81 lines in 2 files changed: 65 ins; 1 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
On Wed, 15 Sep 2021 06:11:23 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:
- Clarify how overriden Properties#entrySet() method impacts the order of stored properties - Tests to verify subclasses of Properties
Changes requested by rriggs (Reviewer). 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. 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 "." 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") ------------- 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 15/09/21 9:48 pm, Daniel Fuchs wrote:
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.
Daniel is right - my initial wording wasn't accurate and I couldn't find a better term to express what I meant. Like Daniel notes above, I was actually talking about using the private EntrySet implementation that the Properties class uses to find out if some subclass overrode the entrySet() method. I think Daniel's rewording of that javadoc section above to state "different set implementation" is much more accurate than my initial wording. -Jaikiran
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 15:59:53 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 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! ------------- 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 Wed, 15 Sep 2021 22:32:33 GMT, Roger Riggs <rriggs@openjdk.org> wrote:
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.
"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.
Done. The PR has been updated accordingly. ------------- 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
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: (Only doc/comment changes): - Implement Roger's suggestion to explicitly state that the system property should be set on command line - Change @implNote to @implSpec based on inputs being provided on CSR issue - Use Roger's and Daniel's suggestions to reword the @implSpec text to explain how overridding of entrySet() can impact the order of the written out properties - Improve the internal code comment (meant for maintainers) to be more clear on what kind of check we are doing before deciding the order of properties to write ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/79d10527..e2effb96 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=17 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=16-17 Stats: 12 lines in 1 file changed: 1 ins; 3 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
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: - Implement Mark's suggestion in CSR, to rename java.util.Properties.storeDate system property to java.properties.date - Tests updated accordingly ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/e2effb96..eb31d287 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=18 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=17-18 Stats: 93 lines in 3 files changed: 1 ins; 0 del; 92 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
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: Implement Mark's suggestion in CSR to include the java.properties.date in the list of system properties listed in System::getProperties() ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/eb31d287..458c1fd3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=19 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=18-19 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
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 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/458c1fd3..e350721a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=20 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=19-20 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 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
Marked as reviewed by dfuchs (Reviewer). ------------- 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
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
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?
-------------
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?
-------------
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/util/Properties.java line 928:
926: // in the natural order of their key. Else, we consider that the subclassed implementation may potentially 927: // have returned a differently ordered entries and so we just use the iteration order of the returned instance. 928: if (entries instanceof Collections.SynchronizedSet<?> ss
Would it be possible to re-format the comment to keep the lines a bit inconsistent, it's just a bit annoying to have overly long lines just here. ------------- 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:05:04 GMT, Alan Bateman <alanb@openjdk.org> wrote:
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.
After this change to include this property in System::getProperties() javadoc was added, there have been inputs (here and on the CSR) which suggest that we probably shouldn't include it here. I've now updated this PR to revert this specific change.
src/java.base/share/classes/java/util/Properties.java line 928:
926: // in the natural order of their key. Else, we consider that the subclassed implementation may potentially 927: // have returned a differently ordered entries and so we just use the iteration order of the returned instance. 928: if (entries instanceof Collections.SynchronizedSet<?> ss
Would you mind re-formatting the comment to keep the line length a bit more consistent with the rest of the code, is's just a bit annoying to have it wrapping.
Done. I've updated the PR to reduce the line length of that code comment. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372
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 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/e350721a..944cbf1e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=21 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=20-21 Stats: 9 lines in 2 files changed: 2 ins; 3 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 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
Marked as reviewed by ihse (Reviewer). ------------- 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
Looks good, thanks for the initiative, the followup on the comments, and finding the sweet spot in the goals, compatibility, and implementation constraints. ------------- Marked as reviewed by rriggs (Reviewer). 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
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
It was a pleasure working on this one due to the timely and valuable reviews, inputs and suggestions. Thanks to everyone for helping in getting this done. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372
On Sat, 4 Sep 2021 15:25:59 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/
This pull request has now been integrated. Changeset: af50772d Author: Jaikiran Pai <jpai@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/af50772d39a063652895e79d474da6ebb992... Stats: 837 lines in 4 files changed: 822 ins; 0 del; 15 mod 8231640: (prop) Canonical property storage Reviewed-by: rriggs, smarks, dfuchs, ihse ------------- PR: https://git.openjdk.java.net/jdk/pull/5372
participants (15)
-
Alan Bateman
-
Alan Bateman
-
Andrey Turbanov
-
Daniel Fuchs
-
Jaikiran Pai
-
Jaikiran Pai
-
Joseph D. Darcy
-
Magnus Ihse Bursie
-
Magnus Ihse Bursie
-
Robert Scholte
-
Robert Scholte
-
Roger Riggs
-
Roger Riggs
-
Stuart Marks
-
Stuart Marks