Bug 62617

Hi Julien,

>> By default, the old behavior is preserved, unless SOURCE_DATE_EPOCH is defined in the environment, in which case it is used as the current date.

I think, the proposed patch might not preserve the current behaviour. Since, right now, even if the SOURCE_DATE_EPOCH is set in the system where the build is run, the tstamp task will not use it and instead will use the current time. With this patch, the behaviour will change in such build files. Maybe it's a better idea to have an explicitly set attribute (a new attribute) on the task which will start honouring this environment variable? That way the default semantics of the attribute can continue to ignore this environment variable unless explicitly set to honour it. Some attribute like "useSourceDateEpoch".


As for the patch:

-            Date d = new Date();
+            Date d;
+            String epoch = System.getenv("SOURCE_DATE_EPOCH");
+            if(epoch.compareTo("") == 0)
+                d = new Date();

I believe you will need a epoch != null check here. In its current form, this will lead to a NPE on systems where the environment variable isn't set. Furthermore, I don't think the compareTo call is required. The "spec" that you pointed to, in the linked content, explicitly states that the value of such an environment variable MUST be an integer, so I think you can just try to use it as an Integer value. If you still want to check if the environment variable value is empty, then I think epoch.isEmpty() is a better API to use.

Hi Jaikiran,

I would be very interested in reproducible build support in Apache Ant as well.

>(In reply to Jaikiran Pai from comment #1)
> I think, the proposed patch might not preserve the current behaviour. Since,
> right now, even if the SOURCE_DATE_EPOCH is set in the system where the
> build is run, the tstamp task will not use it and instead will use the
> current time. With this patch, the behaviour will change in such build
> files.
I don't see the point in preserving backwards compatibility in this case: if SOURCE_DATE_EPOCH is set, the user explicitly expects time stamps to be set to this fixed date instead of the current time as per the specification Julien linked above. Using the current time instead is undesirable behaviour if the environment variable is set, so I would consider the current behaviour a bug that requires backwards-incompatible changes in order to be fixed.

Maybe it's a better idea to have an explicitly set attribute (a new
> attribute) on the task which will start honouring this environment variable?
> That way the default semantics of the attribute can continue to ignore this
> environment variable unless explicitly set to honour it. Some attribute like
> "useSourceDateEpoch".
That would defeat the goal of having reproducible support by default: every projects needs to be aware of this attribute and explicitly set it, which will not happen e.g. for legacy projects. All artifacts created by Apache Ant should be reproducible out of the box if SOURCE_DATE_EPOCH is set.

> I believe you will need a epoch != null check here. In its current form,
> this will lead to a NPE on systems where the environment variable isn't set.
> Furthermore, I don't think the compareTo call is required. The "spec" that
> you pointed to, in the linked content, explicitly states that the value of
> such an environment variable MUST be an integer, so I think you can just try
> to use it as an Integer value. If you still want to check if the environment
> variable value is empty, then I think epoch.isEmpty() is a better API to use.
ACK on the review of the patch.

Actually I don't think it is very likely that SOURCE_DATE_EPOCH has been set by accident when a user tuns the build, so I would probably live with the backwards incompatibility. Point it out in WHATSNEW and maybe log a prominent message in Tstamp when it is used.

As for the patch itself, I'd probably drop the check for empty string but catch a NumberFormatException that parseInt may throw. If the env varibale holds anything that connot be parsed log a warning and continue with a new Date() instance.

Created attachment 36845 [details]
new patch

Here is a new patch (although untested) rebased on ant master. Please let me know what you think of it. Thank you for your interest in this issue :)

Thank you very much for this patch. I've done minor modifications to it and committed it upstream. This change will be available in our 1.10.8 release.

https://gitbox.apache.org/repos/asf?p=ant.git;a=commitdiff;h=1bfa7880afc7cbbc1e3d73be5cfa77dc2fd216d2

Thank you for adding SOURCE_DATE_EPOCH support to Ant. Note that the timezone should also be clamped to a fixed value, for example UTC, otherwise the formatted dates will depend on the user timezone and the result won't be reproducible.

Here is for example the patch I've implemented in Debian to get reproducible timestamps:

https://salsa.debian.org/java-team/ant/blob/debian/1.10.6-1/debian/patches/0009-reproducible-timestamp-task.patch

Reopening because DateUtils.getDateForHeader() also needs to support SOURCE_DATE_EPOCH.

Suggested fix:
https://salsa.debian.org/java-team/ant/blob/debian/1.10.6-1/debian/patches/0011-reproducible-propertyfile-task.patch

Hello Emmanuel, would you be interested in contributing those patches that you have included in Debian, to upstream Ant project itself? If so, can you open individual pull requests for them, so that they can be reviewed?

The <touch> task is also sensitive to the current time, SOURCE_DATE_EPOCH should override the calls to System.currentTimeMillis().