Files modified in the past should not trigger a fingerprint change

[shepmaster]

Previously, we would bail from the fingerprint computation if the old
and new mtimes had changed in any way. This caused some issues of
rebuilding with filesystems that do not preserve nanosecond
granularity (#2874).

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[shepmaster]

Please double and triple check this logic; my brain got a bit mushy while thinking through it. Having names other than a and b helped a lot, but I'm not sure if they are the best they could be.

[alexcrichton]

Ah yeah this all looks good to me, helpful names are indeed helpful!

[alexcrichton]

Thanks! I think this will take effect, but just to be sure could you add a test for this as well?

[shepmaster]

@alexcrichton yes, that makes sense. However, I'm having some trouble reproducing in a smaller case.

As far as I can determine, the build.rs file should use mtime-based fingerprints by default. However, the comparison short-circuits when we check the old hash. Investigating further...

[alexcrichton]

I think that the new hash has the mtime internally, which should in theory short-circuit that hash check. You should also be able to just edit tests/paths.rs as path dependencies use mtime checks as well

[shepmaster]

as path dependencies use mtime checks as well

Yeah, that's the avenue I'm exploring now, but now I'm getting even more confused:

$ cargo build
   Compiling a v0.0.1 (file:///.../backwards_in_time/a)
   Compiling backwards_in_time v0.0.1 (file:///.../backwards_in_time)
$ cargo build
$ touch -t 199001010001 a/src/lib.rs
$ cargo build
$ touch a/src/lib.rs
$ cargo build
   Compiling a v0.0.1 (file:///.../backwards_in_time/a)
   Compiling backwards_in_time v0.0.1 (file:///.../backwards_in_time)

So, cargo already ignores when things go backwards in time, except when it doesn't...

[shepmaster]

Looking back at my original issue, It appears that it's the "output" file (e.g. target/debug/build/tempfile-1b82f6307d61d5f0/output):

$ cargo build
   Compiling a v0.0.1 (file:///.../backwards_in_time/a)
   Compiling backwards_in_time v0.0.1 (file:///.../backwards_in_time)
$ touch -t 199001010102 ./target/debug/build/a-e09e69f5aeec0ac1/output
$ cargo build
   Compiling a v0.0.1 (file:///.../backwards_in_time/a)
   Compiling backwards_in_time v0.0.1 (file:///.../backwards_in_time)

This file contains the result of running the build script.

[alexcrichton]

Hm ok, I think if you have path dependencies though you should only need to touch all the files in the crate back in time. (note specifically every file needs to go back in time)

[alexcrichton]

ping @shepmaster, any update on this? (I can certainly help out if needed)

[shepmaster]

any update on this? (I can certainly help out if needed)

That might be very useful. Basically, I got stuck and then distracted. 😊 I've pushed my latest branch with the tests that I think should fail before and pass after the change. As you can see, that's not the case.

Even just another set of eyes looking at the path that the code is taking and commenting on it would probably prove fruitful.

[alexcrichton]

Awesome, thanks for the update! Of the two tests that are failing:

• rebuild_if_build_artifacts_move_forward_in_time - I think this is just due to the updated output on master, should be an easy fix.
• no_rebuild_if_build_artifacts_move_backwards_in_time - this I think is actually indicative of a larger problem!

So today the "fingerprint" of a package not only counts the files for that package itself but also the fingerprint of all transitive dependencies. That way if anything changes the updates are pushed upwards through the DAG and we recompile everything. I think the problem here is that the fingerprint of a crate is changing in a way that it decides it shouldn't rebuilt, but the value itself is getting modified. This I believe is because the mtime is considered part of the fingerprint.

So today the mtime considered is actually the mtime of the fingerprint file itself, which I think is fair to say shouldn't be tampered with if you want to not recompile things. Put another way, this patch against your branch should fix the tests in question.

This does raise an interesting question in my mind though! Does this still suffice for the use case you were seeing on Docker? This means that if the mtime of the fingerprint files are changing we're still causing rebuilds, which may not be desirable. Specifically, if a project has two "path crates" (those defined through path dependencies), then you'll have recompiles as the actual fingerprint will change (due to mtimes changing). If you have only one path crate, though, then you won't get a recompile.

An alternative solution, to perhaps fix this entirely, is to just store the mtime in the file rather than relying on the file's mtime itself. That way we can guarantee we never lose precision!

wdyt?

[alexcrichton]

ping @shepmaster curious about your thoughts on my last comment

[alexcrichton]

I've pushed the test fixes to #3246 and I r+'d that, I figure we can handle the "real" problem if comes up, which it's not clear if it does yet!