Add support for setting the build time and clamping to the build time #2944
Conversation
macros.in
Outdated
| @@ -245,6 +245,10 @@ Supplements: (%{name} = %{version}-%{release} and langpacks-%{1})\ | |||
| # Is ignored when SOURCE_DATE_EPOCH is not set. | |||
| %clamp_mtime_to_source_date_epoch 0 | |||
|
|
|||
| # If true, make sure that timestamps in built rpms | |||
| # are not later than the build time of the package. | |||
| %clamp_mtime_to_buildtime 0 | |||
There was a problem hiding this comment.
This means we now have two independent knobs for the same thing, and both cannot be right.
I'd rather have something like %clamp_mtime macro that accepts none/buildtime/source_date_epoch as possible values. %clamp_mtime_to_source_date_epoch can then be deprecated and routed to the new macro for now, for backwards compatibility.
There was a problem hiding this comment.
I was expecting that you write something like this ;-) But I kept it simple to start the discussion.
There's also Jan Zerebecki who wants to have a way to tell rpm to set all file mtimes to the build time. (I don't like that personally.) OTOH he could also to that with a brp script, I guess.
There was a problem hiding this comment.
I have not verified if that works, but if one can use lua in the %{?_buildtime} macro to read and set environment variables and execute the date binary then we could do the logic in macros.
And thus also use %{?_mtime} as returning the epoch to set. If it is something like a false value that is not literal 0, the we would read it from disk like the current default. Not sure why someone would want to clamp it, instead of setting it, but if we need to we could have %{?_mtime_treatment} with 'set' or 'clamp'. I guess a callback to a macro for each file might be too slow? What do you think?
There was a problem hiding this comment.
I think this pull request is very reasonable. Though, as @pmatilai said, a "%clamp_mtime macro that accepts none/buildtime/source_date_epoch" would be nicer. It's more forward-looking, because maybe there will be another idea in the future and then we can just another value. And because there's just one setting, there can be no question of precedence between the different options.
There was a problem hiding this comment.
I will preface this that I think adding this knob is a seriously bad idea. But, I think @pmatilai is right that if we're going to do this, we should change to a mode-setting macro like %clamp_mtime and have mode options.
|
Ok, let's move on with this. Time for some bike shedding. Proposal 1: Proposal 2: I added proposal 2 because the original pull request from Jan supported setting all the files to the build time. So we could add a Opinions? |
|
I think proposal 2 is more extensible. With proposal 1 we assume that no policy except clamping would be wanted, but people are clearly interested in other approaches. So I'd go with 2., even if initially there are no plans to provide non-clamping choices. |
|
Proposal 2 looks good to me. |
|
I've updated the pull request. |
macros.in
Outdated
| # which makes sure the the file timestamps are not later than | ||
| # the build time of the package or the value of | ||
| # SOURCE_DATE_EPOCH, respectively. | ||
| #%mtime_policy |
There was a problem hiding this comment.
Sorry I forgot to comment on this: please make that something like "%build_mtime_policy" so its clear this is a build feature.
mlschroe
force-pushed
the
master
branch
2 times, most recently
from
c9579db to
36e2f42
Compare
macros.in
Outdated
| # Defines file timestamp handling in built rpms. Possible values | ||
| # are "clamp_to_buildtime" and "clamp_to_source_date_epoch", | ||
| # which makes sure the the file timestamps are not later than | ||
| # the build time of the package or the value of |
There was a problem hiding this comment.
I'd spell "the build time" as "BUILDTIME" to avoid ambiguity.
build/files.c
Outdated
| fl->processingFailed = 1; | ||
| /* backward compatibility */ | ||
| if (!*mtime_policy_str) { | ||
| if (srcdate && rpmExpandNumeric("%{?clamp_mtime_to_source_date_epoch}")) { |
There was a problem hiding this comment.
Maybe this should issue a deprecation warning?
macros.in
Outdated
| # which makes sure the the file timestamps are not later than | ||
| # the build time of the package or the value of | ||
| # SOURCE_DATE_EPOCH, respectively. | ||
| #%build_mtime_policy |
There was a problem hiding this comment.
This new stuff should also be documented in docs/manual/buildprocess.md
|
Oh and update (some of) the tests to use the new macros, optimally add a new one for the clamp_to_buildtime behavior. The above nits aside, I'm not going to say no to a reproducible builds patch that appears to have consensus from everybody 😅 |
|
I accidentally merged this by pushing to the wrong remote. I'm really sorry about this. |
|
Ack, s*** happens. No worries. |
|
Based on a quick look, the changes did what I asked for so it's all good. If you want to add extra tests later, that's of course okay. |
|
In the light of the xz attack, could you please remove me from the list of people that have direct push rights to the rpm code? I don't see why I would need it because everything is done with pull requests, and it's just increasing the attack surface. |
|
Oh wait, I can do this myself. |
This makes it easier to reproduce a build that was done without SOURCE_DATE_EPOCH. The two new macros are opt in so that the current functionality is not touched.