Use SOURCE_DATE_EPOCH for reproducible builds

[bjornfor]

Hi,

Here are some patches that implement reproducible builds with the SOURCE_DATE_EPOCH variable (in the context of dosfstools being used as part of builds).

I've run the test suite and it passes.

[bjornfor]

Hm, the Travis CI build fails during "make check", while it passes on my machine. I guess I'll have to experiment with travis by doing a number of force-pushes.

[andreasbombe]

Admittedly, when I created the --invariant flag I didn't think of using the reproducible build variables for that. I'm not sure how I feel about removing that flag, yet.

Though the tests are failing on the reference filesystem tests, so something there isn't setting the time quite correctly.

[bjornfor]

I figured it out. "SOURCE_DATE_EPOCH=1426325213 command" doesn't work in dash, only bash. I changed it to "export SOURCE_DATE_EPOCH=1426325213; command" which works in both shells.

@andreasbombe: If you don't like deprecating (and eventually removing) the --invariant flag, then you can pick only the first commit. Or I can remove them and force-push, if you like.

[bjornfor]

Reminder to self: there is a gettimeofday() call in src/common.c that I must patch too.

[pali]

@bjornfor: @andreasbombe now merged #94 into master. Can you rebase your pull request?

[bjornfor]

@pali: Rebased. I did the conversion validation as mentioned in #106 (comment). Please have a look.

[pali]

This looks to be wrong. Why you are specifying both -i and SOURCE_DATE_EPOCH? Either you want specific volume ID or you want to use specific date epoch.

[bjornfor]

The tests break if using anything but the specific timestamp and volume_id, as given by the --invariant flag they replace. See here:

dosfstools/src/mkfs.fat.c

Lines 1592 to 1595 in a874650

	case OPT_INVARIANT:
	invariant = 1;
	volume_id = 0x1234abcd;
	create_time = 1426325213;

.

I didn't investigate further, I simply continued the existing behaviour.

[pali]

So invariant now use volume id 0x1234abcd. So update volume id in testing image to match volume id computed from SOURCE_DATE_EPOCH? This could be more cleaner way.

[bjornfor]

I actually like it the way it is; the volume-id is specified as a hex number, so it's easier to spot in the reference image (.xxd file). (SOURCE_DATE_EPOCH is specified in base 10.) And the separate flag provides a separate knob to use in testing (although it is hardcoded in the tests right now.)

Also, I'm not sure @andreasbombe would even like to merge that commit, given this comment. I'm sort of waiting for confirmation whether I should remove that commit.

I can update the reference images and remove the use of volume-id flag in the test, if that is preferred, but I just thought I'd say what's on my mind first.

[pali]

But this does not test that SOURCE_DATE_EPOCH env generates stable volume id.

[bjornfor]

What did you have in mind? To run the test one more time, with different SOURCE_DATE_EPOCH and reference image? I didn't do that because I just wanted to update the test from the special --invariant flag to the generic SOURCE_DATE_EPOCH. I didn't want to make functional changes to the test.

Also, @andreasbombe hasn't replied whether they would like to even deprecate the --invariant flag. So the two last commits might get dropped.

[pali]

What did you have in mind?

I mean that you are using SOURCE_DATE_EPOCH in test, but then volume id is not generated by SOURCE_DATE_EPOCH, but overridden by -i option.

So it does not test that SOURCE_DATE_EPOCH is fully working. What I mean is to use only SOURCE_DATE_EPOCH as I wrote in first post.

@andreasbombe has not replied yet, but personally I do not see reason why to not use SOURCE_DATE_EPOCH in test if you are introducing it. And I guess that --invariant option is there specially only for tests as I do not see any benefit to have static volume id. Static hardcoded volume id just cause problems as it is used as unique identifier.

[bjornfor]

I mean that you are using SOURCE_DATE_EPOCH in test, but then volume id is not generated by SOURCE_DATE_EPOCH, but overridden by -i option.

Right, volume_id is overridden. But not create_time.

If I remove the -i flag then I have to update the reference image to make the test pass. That means:

• There is already (some) test coverage for SOURCE_DATE_EPOCH.
• Replacing -i with SOURCE_DATE_EPOCH (+ updating ref. image) would mean removing test coverage for the -i code path.

I guess instead of updating the existing tests, we can add new tests with SOURCE_DATE_EPOCH=X and ref. image X, where X=[1, 2]? But personally I feel the coverage is OK as is.

[pali]

We do not have answer from @andreasbombe about removal of --invariant option, but I'm for deprecation in next release and later to fully remove it. Therefore I'm fine with removing it from documentation. But also it should be removed from all existing usage, therefore from tests too.

SOURCE_DATE_EPOCH seems like a standard solution how to deal with stable timestamps and --invariant option is just dosfstools specific.

So I'm for:

• removing --invariant option from help and documentation
• updating test scripts and test files to use SOURCE_DATE_EPOCH
• do not use --invariant in tests anymore
• let --invariant to be work in next release with release notes information about deprecation
• removal of --invariant option from code after next release

[bjornfor]

I pushed another update:

Changes v3 -> v4:
* Use die() for fatal errors, instead of fprintf() + exit().
* Fix bug from v3 where volume-id was calculated with 20 bits of zero
  instead of real value bits from SOURCE_DATE_EPOCH.
* Refactoring / review feedback.

Please have a look.

[bjornfor]

@pali: I pushed a new rebase where trailing '\n' in calls to die() have been removed.

[bjornfor]

Ping.

[bjornfor]

@andreasbombe: Friendly ping. Would you mind taking a look?

[pali]

Also remove changelog from commit message as it is not needed there.

[bjornfor]

Rebased on master to resolve new conflicts.

[bjornfor]

Hi, is there anything I can do to get this merged?

[pali]

@bjornfor I'm fine with first two commits. And about deprecation we have not get answer from @andreasbombe (my proposal is in #106 (comment))

[pali]

@bjornfor: As there is no answer from @andreasbombe, I'm going to accept all 3 patches into master branch. Can you rebase this pull request on top of the master branch?

[bjornfor]

@pali: Thanks! Rebased on master now (no conflicts).

[pali]

@bjornfor: tests are not passing :-( can you look at it?

[pali]

@bjornfor: Are you going to look at these issues? (Also more other pull requests were merged into master, therefore a new rebasing would be required)

[bjornfor]

I had a quick look, was not able to fix it (or understand why it broke) and I haven't had time to look more into it.

[pali]

Ok, I'm letting this pull request open.

[pali]

@bjornfor or anybody else: Are you going to fix this pull request?

[hieunv-tsdv]

@bjornfor @pali ,

I checked this pull request, it seems the problem is coming from the "random disk signature".
The mkfs.fat should not generate a random disk signature when "SOURCE_DATE_EPOCH" and volume_id already exists, similar to using "--invariant" option.
You can see all the CI are PASS after I fixed it:

hieunv-tsdv@fbb5e91
https://travis-ci.com/github/hieunv-tsdv/dosfstools

[bjornfor]

@hieunv-tsdv: Thank you! I've squashed your fix into my originally broken commit and rebased onto master. Now the tests pass!

[pali]

Hello! Tests were not started on github/travis. Could you please rebase commits on top of master branch? After new push tests should be started again automatically.

[bjornfor]

Rebased.

[pali]

I'm looking at this change and I would like to know: Why check for volume_id is needed here? On line 514 is: volume_id = generate_volume_id(); And generate_volume_id() introduced in this patch returns value from getenv("SOURCE_DATE_EPOCH") if this env is set.

[hieunv-tsdv]

Hi @pali ,

And generate_volume_id() introduced in this patch returns value from getenv("SOURCE_DATE_EPOCH") if this env is set

Yes.
However, the volume_id can be passed from "-i" option. I'm not sure if an user passed in "-i" option a null value.
We can remove the check for volume_id if we make sure that "-i" cannot pass a null value.

case 'i':               /* i : specify volume ID */
            errno = 0;
            conversion = strtoll(optarg, &tmp, 16);
        
            if (!*optarg || isspace(*optarg) || *tmp || conversion < 0) {
                printf("Volume ID must be a hexadecimal number\n");

[pali]

Well, I have not find any restriction to volume id number. So I think that zero is also valid volume id.

Microsoft fatgen103 specification says about volume id:

This ID is usually generated by simply combining the current date and time into a 32-bit value.

So I think that zero volume id should be allowed.

Also FAT12 and FAT16 allows to indicate that volume id is not recorded at all and therefore allow to distinguish between volume id is set to 0 and volume id is not set.

[hieunv-tsdv]

@pali: Thanks for the information.

@bjornfor: Could you please help me update this merge request:
if (invariant || (getenv("SOURCE_DATE_EPOCH") && volume_id)) => if (invariant || getenv("SOURCE_DATE_EPOCH"))
This change doesn't affect to the CI jobs: https://travis-ci.com/github/hieunv-tsdv/dosfstools/builds/235387092
sorry for this inconvenience.

[bjornfor]

@hieunv-tsdv @pali: Updated diff.

[pali]

Thanks! Merged.