Bug 62617 - Honor SOURCE_DATE_EPOCH to allow reproducible builds
Summary: Honor SOURCE_DATE_EPOCH to allow reproducible builds
Status: REOPENED
Alias: None
Product: Ant
Classification: Unclassified
Component: Core tasks (show other bugs)
Version: unspecified
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: 1.10.8
Assignee: Ant Notifications List
URL:
Keywords:
Depends on:
Blocks: 61269
  Show dependency tree
 
Reported: 2018-08-10 18:34 UTC by Julien Lepiller
Modified: 2019-10-25 08:21 UTC (History)
2 users (show)



Attachments
patch for tstamp (1.07 KB, patch)
2018-08-10 18:34 UTC, Julien Lepiller
Details | Diff
new patch (1.50 KB, patch)
2019-10-17 11:32 UTC, Julien Lepiller
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Lepiller 2018-08-10 18:34:20 UTC
Created attachment 36086 [details]
patch for tstamp

Hi,

the <tstamp> task allows programmers to refer to the current time in the build process.

I would like to suggest honoring the SOURCE_DATE_EPOCH environment variable. This variable is set by distributions during build and refer to a fixed timestamp that can be used reproducibly. More information on https://reproducible-builds.org/specs/source-date-epoch/.

This bug report is related to #61269.

Attached is a patch for this issue. 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.

Thank you :)
Comment 1 Jaikiran Pai 2018-08-13 05:39:27 UTC
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.
Comment 2 Jonas Witschel 2019-10-15 14:00:54 UTC
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.
Comment 3 Stefan Bodewig 2019-10-17 07:43:03 UTC
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.
Comment 4 Julien Lepiller 2019-10-17 11:32:17 UTC
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 :)
Comment 5 Jaikiran Pai 2019-10-20 13:01:26 UTC
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.
Comment 7 Emmanuel Bourg 2019-10-24 14:34:55 UTC
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
Comment 8 Emmanuel Bourg 2019-10-24 14:37:34 UTC
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
Comment 9 Jaikiran Pai 2019-10-25 08:21:30 UTC
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?