makefs: Honor timestamp passsed through 'SOURCE_DATE_EPOCH'
Needs ReviewPublic
Actions

Authored by bnovkov on Tue, Apr 1, 11:44 AM.

Details

Reviewers

emaste

Group Reviewers

Klara

Summary

Allow users to specify default timestamps using the 'SOURCE_DATE_EPOCH'
environment variable. Add FFS tests that check timestamps passed
using 'SOURCE_DATE_EPOCH' and make sure -T takes precedence over
'SOURCE_DATE_EPOCH'.

PR: 285630
Sponsored by: Klara, Inc.
Sponsored by: The FreeBSD Foundation

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

bnovkov created this revision.Tue, Apr 1, 11:44 AM

Herald added subscribers: ziaee, imp. · View Herald TranscriptTue, Apr 1, 11:44 AM

bnovkov requested review of this revision.Tue, Apr 1, 11:44 AM

Harbormaster completed remote builds in B63244: Diff 152955.Tue, Apr 1, 11:44 AM

bnovkov added a parent revision: D49492: makefs: Add tests for the -T flag.Tue, Apr 1, 11:52 AM

bnovkov added a reviewer: Klara.Tue, Apr 1, 11:55 AM

jlduran added a subscriber: jlduran.Thu, Apr 3, 6:05 AM

jlduran mentioned this in D49492: makefs: Add tests for the -T flag.Thu, Apr 3, 7:24 AM

markj added a subscriber: markj.Mon, Apr 7, 11:21 AM

markj added inline comments.

usr.sbin/makefs/makefs.8
261–262	This is from D49531? Presumably SOURCE_DATE_EPOCH should be documented?
usr.sbin/makefs/makefs.c
132

guest-patmaddox added a subscriber: guest-patmaddox.Mon, Apr 7, 12:01 PM

Address @markj 's comments.

bnovkov marked an inline comment as done.Mon, Apr 7, 5:19 PM

bnovkov added inline comments.

usr.sbin/makefs/makefs.8
261–262	Whoops, not sure how that first manpage change made it here. I've added the correct change.

ziaee added inline comments.Mon, Apr 7, 5:42 PM

usr.sbin/makefs/makefs.8
598–614

Oops, just had an even better idea than the suggestion:
"Specify a file-creation timestamp using the same syntax as
.Fl T .
Overridden by
.Fl T ."

Sorry for the noise.
EDIT: Update for markj's explaination.

markj added inline comments.Mon, Apr 7, 5:54 PM

usr.sbin/makefs/makefs.8
605
613	This is backwards, -T overrides SOURCE_DATE_EPOCH.

There's a spec for SOURCE_DATE_EPOCH: https://reproducible-builds.org/specs/source-date-epoch/

It doesn't make reference to suggested priority when there are multiple sources of timestamps, but does indicate it's intended to provide an alternate notion of "now" or "current time" so logically a command-line flag should override it.

Thinking some more about this, SOURCE_DATE_EPOCH actually should not affect the timestamps of files in the produced image (they should come from -T or the file system). SOURCE_DATE_EPOCH should really affect things that would otherwise be the current time, like iso9660's "Volume Creation Date and Time".

In D49602#1133494, @emaste wrote:

There's a spec for SOURCE_DATE_EPOCH: https://reproducible-builds.org/specs/source-date-epoch/

Thank you for linking the spec, I wasn't aware of it.

Thinking some more about this, SOURCE_DATE_EPOCH actually should not affect the timestamps of files in the produced image (they should come from -T or the file system). SOURCE_DATE_EPOCH should really affect things that would otherwise be the current time, like iso9660's "Volume Creation Date and Time".

Looking at some of the existing tooling that supports this (namely dosfstools's mkfs.fat), it seems that SOURCE_DATE_EPOCH also gets used for filesystem timestamps, but I think what you proposed is the way to go.

With that in mind, I think it'd best to rework this diff to parse and save the value of SOURCE_DATE_EPOCH and plug any users of current time in separate reviews. A quick glance reveals mtree.c:1057 as one of the offenders, but I'm sure more will pop up.
I'll also make sure that makefs implements proper timestamp clamping, as defined by the SOURCE_DATE_EPOCH spec.
Does that sound reasonable?

In D49602#1134702, @bnovkov wrote:

In D49602#1133494, @emaste wrote:

There's a spec for SOURCE_DATE_EPOCH: https://reproducible-builds.org/specs/source-date-epoch/

Thank you for linking the spec, I wasn't aware of it.

Thinking some more about this, SOURCE_DATE_EPOCH actually should not affect the timestamps of files in the produced image (they should come from -T or the file system). SOURCE_DATE_EPOCH should really affect things that would otherwise be the current time, like iso9660's "Volume Creation Date and Time".

Looking at some of the existing tooling that supports this (namely dosfstools's mkfs.fat), it seems that SOURCE_DATE_EPOCH also gets used for filesystem timestamps, but I think what you proposed is the way to go.

With that in mind, I think it'd best to rework this diff to parse and save the value of SOURCE_DATE_EPOCH and plug any users of current time in separate reviews. A quick glance reveals mtree.c:1057 as one of the offenders, but I'm sure more will pop up.
I'll also make sure that makefs implements proper timestamp clamping, as defined by the SOURCE_DATE_EPOCH spec.
Does that sound reasonable?

It sounds reasonable to me, FWIW. We need something similar in mkimg (see D49716), so maybe SOURCE_DATE_EPOCH could be handled with a helper function in libutil? That is, libutil can provide a function which returns a time_t from SOURCE_DATE_EPOCH if it is set.

> It sounds reasonable to me, FWIW. We need something similar in mkimg (see D49716), so maybe SOURCE_DATE_EPOCH could be handled with a helper function in libutil? That is, libutil can provide a function which returns a time_t from SOURCE_DATE_EPOCH if it is set.

Right, I imagine more tools will need it as well, so a libutil helper function sounds like the best way to handle this.