On 2021-09-09 10:27, Jaikiran Pai wrote:
On 31/08/21 7:09 pm, Jaikiran Pai wrote:
Hello Magnus,
On 30/08/21 5:29 pm, Magnus Ihse Bursie wrote:
On 2021-08-28 17:16, Alan Bateman wrote:
On 28/08/2021 05:45, Jaikiran Pai wrote:
I hadn't considered the system property approach to switch to old
behaviour in my proposals, so this is a very good input and I
personally think the most logical proposals so far.
Roger may be right that few would care but it would be changing
behavior that goes back to JDK 1.0. In your list (and thanks for
writing down the options with pros/cons) then your 1a where
SOURCE_DATE_EPOCH changes to write the epoch date rather than the
current date seems to be most consistent with other tools and the
wider eco system. It seems the least disruptive in that it doesn't
change existing behavior and would be opt-in when reproducibility
is required. Previous discussion was leading towards your option 2
(or variants of) but isn't option doesn't help the cases where
build tools use libraries that rely on the older methods.
If I understood the numbering and alternatives correctly, I don't
think there is a conflict between 1a and 2, and I think both should
be implemented, for different reasons.
This is my understanding of the options, with the rationale I see
for choosing them:
* 1a says that we change the store() method to write the date from
SOURCE_DATE_EPOCH, if it is set -- otherwise it keeps the old
behavior. This allows users who want to use old Java code (or code
they can't modify) to produce output in a reproducible way to do so
without changing the source code of the product.
* 2 says that we add a new override to store() which also takes an
Instant. This way programmers who write new code and want to make
sure it is always reproducible can send in Instant.EPOCH, and not
rely on the user to configure SOURCE_DATE_EPOCH.
(In fact, when thinking of it, this seems overly complex. There is
no real need to send in a generic Instant, just an override with a
boolean skipTimestamp, or something like that. Basically, any
solution which allows programmers who write new code to get property
files without timestamps easily, is what's needed to fulfill this spot.
I do agree that it would be good to just get rid of the date comment
and let callers control what exact comments (if any) get written out.
However, I think that having both - a new method (overloaded or named
differently) and at the same time changing the implementation of the
existing store(...) methods to make the date comment reproducible is
a bit of a overkill for a comment that doesn't even play a functional
role in that API. I listed that option of a new overloaded API and
would have been in favour of it, if that was the only addition/change
that we did to support the date comment disabling/reproducibility.
Having said that, I will gladly go ahead and include/work on that
additional new API, if there is some general agreement on doing so.
The PR[1] related to the changes that have been agreed upon in this
discussion has reached a decent state, I think. There's however still
a valid question from Magnus. I am pasting that comment from Magnus,
from the PR, here:
"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."
Magnus, I too think that there should be a way to completely get rid
of the date comment. It's my opinion that this date comment is almost
useless for Java programs because the Properties.load() doesn't load
any comments. So whatever/whoever is relying on these properties is
most likely going to be external (non-Java) scripts/tooling or some
really specific/odd Java code which reads this stored properties file,
per line and does something with the date comment. Highly unlikely but
still possible. So far, from what I have followed, no one has stated
that they use the date comment in some form. I even searched JBS
issues to see if there was any hint of this usage. On the contrary, I
found an (old) issue where someone asked for disabling the date
comment altogether and they explained a use case where they think the
current default of writing out that comment isn't helpful[2].
At the same time, I think the concern for backward compatibility is
valid, plus the concern that introducing a new store API (either
overloaded or differently named) which allows reproducibility plus the
ability to disable date comment, will require existing code to migrate
to this new ones, which realistically takes many years.
So, if callers/users still want to get rid of the date comment when
dealing with the Properties.store() APIs, we could ask them to come up
with a custom implementation which extends the java.util.Properties
class. That is easier said than done, because the store() APIs have
very involved logic when it comes to formatting and writing out the
key/values of that instance. That logic is private to the
java.util.Properties, which effectively means these custom extended
classes can't use it (unless they copy it over and make these
implementations very brittle). So extending/overriding the entirety of
these store() APIs is realistically ruled out.
Having said all that and reviewing again the potential options that I
listed in my earlier mail, I think there's probably a more easier
middle ground here, which allows callers/tools/libraries to control
whether or not the date comment gets written out. So here's a proposal
which might help - how about, *in addition to* what we have already
done in this PR, we also introduce a new protected method in
java.util.Properties which looks like:
/**
* Returns {@code true} if the {@link #store} methods of this properties
* instance must write out the date comment. Returns {@code false}
otherwise.
* @since 18
*/
protected boolean mustStoreDateComment() {
// by default we store a date comment
return true;
}
(ignore any javadoc issues, please :))
Then the implementation of existing store() APIs in
java.util.Properties, can call this method to decide whether or not to
write out a date comment. By default, the implementation of this new
mustStoreDateComment() in java.util.Properties will return true, which
means there shouldn't be any backward compatibility issues. At the
same time this will allow tools/libraries/callers to extend the
java.util.Properties class and (only) override this method to return
false, if they want to disable writing out the date comment. The
advantage of this would be:
- Ability to fully disable the date comment and have full control on
what comments (if any) get written out by the existing store() APIs
- These custom extended classes don't have to worry or deal with the
formatting rules of key/values that the existing store() APIs will
continue to take care off.
- No system property or additional environment variable necessary for
disabling the date comment altogether
- The JDK will still have full control on how the date comment is
written out and will continue to write it out in a reproducible manner
when SOURCE_DATE_EPOCH is set.
- The new "protected" API, IMO, doesn't introduce a big new API
surface, unlike the previously proposed overloaded or differently
named variants of the store() APIs.
The name of this new method of course is something that can be decided
- I just came up with that name just to explain what I mean.
Is this something that is acceptable and worth adding?
Why is this complexity needed?
Let me be more concrete: I suggest adding a new overloaded method,
publicvoidstore(Writerout, Stringcomments, boolean includeTimestamp)
(As Roger says, there's no point in duplicating the OutputStream version
of the API as well, since it's trivial to convert an OutputStream to a
Writer.)
If called with includeTimestamp = true, it will be identical to
store(Writer out, String comments). But if called with false, the
timestamp line will be omitted. SOURCE_DATE_EPOCH has no relevance in
this case, since no timestamp is produced.
This way someone who writes a new tool (or whatever) can decide upfront
to generate reproducible output.
The method will also be "discoverable" for developers, since an IDE will
typically show the overloaded method as an alternative for code
completion, which will immediately signal to programmers that there is a
way to disable timestamps.
I can't say I understood any arguments against a solution like this. Are
there any?
/Magnus
[1] https://github.com/openjdk/jdk/pull/5372
[2] https://bugs.openjdk.java.net/browse/JDK-5032603
-Jaikiran
