On Fri, 10 Sep 2021 10:15:45 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> The commit in this PR implements the proposal for enhancement that was >> discussed in the core-libs-dev mailing list recently[1], for >> https://bugs.openjdk.java.net/browse/JDK-8231640 >> >> At a high level - the `store()` APIs in `Properties` have been modified to >> now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env >> variable is set, then instead of writing out the current date time as a date >> comment, the `store()` APIs instead will use the value set for this env >> variable to parse it to a `Date` and write out the string form of such a >> date. The implementation here uses the `d MMM yyyy HH:mm:ss 'GMT'` date >> format and `Locale.ROOT` to format and write out such a date. This should >> provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. >> Furthermore, intentionally, no changes in the date format of the "current >> date" have been done. >> >> These modified `store()` APIs work in the presence of the `SecurityManager` >> too. The caller is expected to have a read permission on the >> `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that >> permission, then the implementation of these `store()` APIs will write out >> the "current date" and will ignore any value that has been set for the >> `SOURCE_DATE_EPOCH` env variable. This should allow for backward >> compatibility of existing applications, where, when they run under a >> `SecurityManager` and perhaps with an existing restrictive policy file, the >> presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the >> `store()` APIs. >> >> The modified `store()` APIs will also ignore any value for >> `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such >> cases, the `store()` APIs will write out the "current date" and ignore the >> value set for this environment variable. No exceptions will be thrown for >> such invalid values. This is an additional backward compatibility precaution >> to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking >> applications. >> >> An additional change in the implementation of these `store()` APIs and >> unrelated to the date comment, is that these APIs will now write out the >> property keys in a deterministic order. The keys will be written out in the >> natural ordering as specified by `java.lang.String#compareTo()` API. >> >> The combination of the ordering of the property keys when written out and >> the usage of `SOURCE_DATE_EPOCH` environment value to determine the date >> comment should together allow for reproducibility of the output generated by >> these `store()` APIs. >> >> New jtreg test classes have been introduced to verify these changes. The >> primary focus of `PropertiesStoreTest` is the ordering aspects of the >> property keys that are written out. On the other hand >> `StoreReproducibilityTest` focuses on the reproducibility of the output >> generated by these APIs. The `StoreReproducibilityTest` runs these tests >> both in the presence and absence of `SecurityManager`. Plus, in the presence >> of SecurityManager, it tests both the scenarios where the caller is granted >> the requisite permission and in other case not granted that permission. >> >> These new tests and existing tests under `test/jdk/java/util/Properties/` >> pass with these changes. >> >> [1] >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html >> [2] https://reproducible-builds.org/specs/source-date-epoch/ > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > jcheck fix - trailing whitespace in test 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
