Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 13:15:18 GMT, Jaikiran Pai 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
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 12:56:28 GMT, Daniel Fuchs 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
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 11:43:25 GMT, Magnus Ihse Bursie 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-220f7bcc6d1a7ec33764f81eb95ccb0f69444e1eb923b015025e2140c3ffe145R280
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 11:59:56 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/java/util/Properties.java line 836:
>>
>>> 834: *
>>> 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
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 02:30:01 GMT, 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 HH:mm:ss 'GMT'` date >> format and `Locale.ROOT` to format and write out such a date. This should >> provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. >> Furthermore, intentionally, no changes in the date format of the "current >> date" have been done. >> >> These modified `store()` APIs work in the presence of the `SecurityManager` >> too. The caller is expected to have a read permission on the >> `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that >> permission, then the implementation of these `store()` APIs will write out >> the "current date" and will ignore any value that has been set for the >> `SOURCE_DATE_EPOCH` env variable. This should allow for backward >> compatibility of existing applications, where, when they run under a >> `SecurityManager` and perhaps with an existing restrictive policy file, the >> presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the >> `store()` APIs. >> >> The modified `store()` APIs will also ignore any value for >> `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such >> cases, the `store()` APIs will write out the "current date" and ignore the >> value set for this environment variable. No exceptions will be thrown for >> such invalid values. This is an additional backward compatibility precaution >> to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking >> applications. >> >> An additional change in the implementation of these `store()` APIs and >> unrelated to the date comment, is that these APIs will now write out the >> property keys in a deterministic order. The keys will be written out in the >> natural ordering as specified by `java.lang.String#compareTo()` API. >> >> The combination of the ordering of the property keys when written out and >> the usage of `SOURCE_DATE_EPOCH` environment value to determine the date >> comment should together allow for reproducibility of the output generated by >> these `store()` APIs. >> >> New jtreg test classes have been introduced to verify these changes. The >> primary focus of `PropertiesStoreTest` is the ordering aspects of the >> property keys that are written out. On the other hand >> `StoreReproducibilityTest` focuses on the reproducibility of the output >> generated by these APIs. The `StoreReproducibilityTest` runs these tests >> both in the presence and absence of `SecurityManager`. Plus, in the presence >> of SecurityManager, it tests both the scenarios where the caller is granted >> the requisite permission and in other case not granted that permission. >> >> These new tests and existing tests under `test/jdk/java/util/Properties/` >> pass with these changes. >> >> [1] >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html >> [2] https://reproducible-builds.org/specs/source-date-epoch/ > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > 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
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 10:47:34 GMT, Daniel Fuchs 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: *
>> 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
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 02:30:01 GMT, 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 HH:mm:ss 'GMT'` date
>> format and `Locale.ROOT` to format and write out such a date. This should
>> provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set.
>> Furthermore, intentionally, no changes in the date format of the "current
>> date" have been done.
>>
>> These modified `store()` APIs work in the presence of the `SecurityManager`
>> too. The caller is expected to have a read permission on the
>> `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that
>> permission, then the implementation of these `store()` APIs will write out
>> the "current date" and will ignore any value that has been set for the
>> `SOURCE_DATE_EPOCH` env variable. This should allow for backward
>> compatibility of existing applications, where, when they run under a
>> `SecurityManager` and perhaps with an existing restrictive policy file, the
>> presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the
>> `store()` APIs.
>>
>> The modified `store()` APIs will also ignore any value for
>> `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such
>> cases, the `store()` APIs will write out the "current date" and ignore the
>> value set for this environment variable. No exceptions will be thrown for
>> such invalid values. This is an additional backward compatibility precaution
>> to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking
>> applications.
>>
>> An additional change in the implementation of these `store()` APIs and
>> unrelated to the date comment, is that these APIs will now write out the
>> property keys in a deterministic order. The keys will be written out in the
>> natural ordering as specified by `java.lang.String#compareTo()` API.
>>
>> The combination of the ordering of the property keys when written out and
>> the usage of `SOURCE_DATE_EPOCH` environment value to determine the date
>> comment should together allow for reproducibility of the output generated by
>> these `store()` APIs.
>>
>> New jtreg test classes have been introduced to verify these changes. The
>> primary focus of `PropertiesStoreTest` is the ordering aspects of the
>> property keys that are written out. On the other hand
>> `StoreReproducibilityTest` focuses on the reproducibility of the output
>> generated by these APIs. The `StoreReproducibilityTest` runs these tests
>> both in the presence and absence of `SecurityManager`. Plus, in the presence
>> of SecurityManager, it tests both the scenarios where the caller is granted
>> the requisite permission and in other case not granted that permission.
>>
>> These new tests and existing tests under `test/jdk/java/util/Properties/`
>> pass with these changes.
>>
>> [1]
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html
>> [2] https://reproducible-builds.org/specs/source-date-epoch/
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 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
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 02:30:01 GMT, 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 HH:mm:ss 'GMT'` date
>> format and `Locale.ROOT` to format and write out such a date. This should
>> provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set.
>> Furthermore, intentionally, no changes in the date format of the "current
>> date" have been done.
>>
>> These modified `store()` APIs work in the presence of the `SecurityManager`
>> too. The caller is expected to have a read permission on the
>> `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that
>> permission, then the implementation of these `store()` APIs will write out
>> the "current date" and will ignore any value that has been set for the
>> `SOURCE_DATE_EPOCH` env variable. This should allow for backward
>> compatibility of existing applications, where, when they run under a
>> `SecurityManager` and perhaps with an existing restrictive policy file, the
>> presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the
>> `store()` APIs.
>>
>> The modified `store()` APIs will also ignore any value for
>> `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such
>> cases, the `store()` APIs will write out the "current date" and ignore the
>> value set for this environment variable. No exceptions will be thrown for
>> such invalid values. This is an additional backward compatibility precaution
>> to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking
>> applications.
>>
>> An additional change in the implementation of these `store()` APIs and
>> unrelated to the date comment, is that these APIs will now write out the
>> property keys in a deterministic order. The keys will be written out in the
>> natural ordering as specified by `java.lang.String#compareTo()` API.
>>
>> The combination of the ordering of the property keys when written out and
>> the usage of `SOURCE_DATE_EPOCH` environment value to determine the date
>> comment should together allow for reproducibility of the output generated by
>> these `store()` APIs.
>>
>> New jtreg test classes have been introduced to verify these changes. The
>> primary focus of `PropertiesStoreTest` is the ordering aspects of the
>> property keys that are written out. On the other hand
>> `StoreReproducibilityTest` focuses on the reproducibility of the output
>> generated by these APIs. The `StoreReproducibilityTest` runs these tests
>> both in the presence and absence of `SecurityManager`. Plus, in the presence
>> of SecurityManager, it tests both the scenarios where the caller is granted
>> the requisite permission and in other case not granted that permission.
>>
>> These new tests and existing tests under `test/jdk/java/util/Properties/`
>> pass with these changes.
>>
>> [1]
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html
>> [2] https://reproducible-builds.org/specs/source-date-epoch/
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> unused imports
src/java.base/share/classes/java/util/Properties.java line 836:
> 834: *
> 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
Re: RFR: 8231640: (prop) Canonical property storage [v13]
> 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 HH:mm:ss 'GMT'` date > format and `Locale.ROOT` to format and write out such a date. This should > provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. Furthermore, > intentionally, no changes in the date format of the "current date" have been > done. > > These modified `store()` APIs work in the presence of the `SecurityManager` > too. The caller is expected to have a read permission on the > `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that > permission, then the implementation of these `store()` APIs will write out > the "current date" and will ignore any value that has been set for the > `SOURCE_DATE_EPOCH` env variable. This should allow for backward > compatibility of existing applications, where, when they run under a > `SecurityManager` and perhaps with an existing restrictive policy file, the > presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the `store()` > APIs. > > The modified `store()` APIs will also ignore any value for > `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such cases, > the `store()` APIs will write out the "current date" and ignore the value set > for this environment variable. No exceptions will be thrown for such invalid > values. This is an additional backward compatibility precaution to prevent > any rogue value for `SOURCE_DATE_EPOCH` from breaking applications. > > An additional change in the implementation of these `store()` APIs and > unrelated to the date comment, is that these APIs will now write out the > property keys in a deterministic order. The keys will be written out in the > natural ordering as specified by `java.lang.String#compareTo()` API. > > The combination of the ordering of the property keys when written out and the > usage of `SOURCE_DATE_EPOCH` environment value to determine the date comment > should together allow for reproducibility of the output generated by these > `store()` APIs. > > New jtreg test classes have been introduced to verify these changes. The > primary focus of `PropertiesStoreTest` is the ordering aspects of the > property keys that are written out. On the other hand > `StoreReproducibilityTest` focuses on the reproducibility of the output > generated by these APIs. The `StoreReproducibilityTest` runs these tests > both in the presence and absence of `SecurityManager`. Plus, in the presence > of SecurityManager, it tests both the scenarios where the caller is granted > the requisite permission and in other case not granted that permission. > > These new tests and existing tests under `test/jdk/java/util/Properties/` > pass with these changes. > > [1] > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html > [2] https://reproducible-builds.org/specs/source-date-epoch/ Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: 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=5372=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5372=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
