cargo build does not rebuild if a source file was modified during a build
Closed this issue · 32 comments
I haven't checked, but I would guess this is a timestamp issue: the input is modified before the output artifact is created, even though the modified input was not used to build the artifact. This would happen any time after rustc is parsing.
As an example with the current master of cargo (8fc3fd8):
cargo$ git checkout 8fc3fd8
cargo$ cargo clean
cargo$ cargo build &
[Cargo output snipped]
[Wait until it a few seconds after 'Compiling cargo v0.9.0 (file://path/to/checkout/cargo)']
cargo$ patch src/cargo/lib.rs <<EOF
82a83
> println!("a-new-line");
EOF
cargo$ wait $! && cargo build
[1]+ Done cargo build
cargo$ target/debug/cargo | grep a-new-line
[no output]
cargo$ touch src/cargo/lib.rs && cargo build
Compiling cargo v0.9.0 (file:///home/kamal/clones/cargo)
cargo$ target/debug/cargo | grep a-new-line
a-new-line
I personally view this as a feature rather than a bug, but it shouldn't be too too hard to fix this.
Yea I figured this could be a bit philosophical! My personal view is that a build should be a pure function of the build inputs, and that skipping rebuilds is an optimization that is allowed providing it maintains the purity. This is a violation of that. :-)
(Though the fix to the greater issue would likely require changing from timestamps to hashes to determine if inputs have changed.)
Right now the mtime we compare against is the foo.d file, not the output file. It looks like rustc just generates foo.d far before the output in the compilation process.
This should just be a change to this line
Anyone working on this?
I don't believe so, no, feel free to take it!
@Jimmio92 that sounds distinct from this issue, feel free to open a separate one!
@alexcrichton The issue I mentioned in the comment has been resolved in the nightly build 20160511. Sorry for the totally unhelpful comment. I was on stable.
Aha, even better! Glad it worked out :)
Does the problem exist? Maybe issue should be closed?
Yes I believe this is still an issue unfortunately
as this is labeled E-easy, may I ask for some mentor instructions here please?
specifically:
- how do I write a unit test for this?
- alternatively, how do I package and run a custom cargo?
- in terms of the code which one is the "foo.d file", and which the output file?
how do I write a unit test for this?
My understanding is that this essentially requires racing against rustc, so I don't think a true unit-test is possible. Although perhaps CargoPathExt::move_into_the_future could be used to simulate a similar effect?
alternatively, how do I package and run a custom cargo?
Cargo is a stand-alone binary, so just
$ cargo build
$ export my_cargo="$(pwd)/target/debug/cargo"
would work.
in terms of the code which one is the "foo.d file", and which the output file?
dep_info_loc is the foo.d. The outptut file is more tricky, because different crate types and targets may produce different output files (and several of them at a time!). Context::outputs lists unit's outputs.
Oh actually for testing this what I'd recommend is:
- Create a workspace with two crates, one binary which depends on a proc-macro
- Make the proc macro connect to a TCP server on a local thread in the test. Upon successful connection it waits until EOF to continue
- When the binary is being compiled it uses the procedural macro (aka custom derive). This blocks compilation
- When compilation is blocked, a file is modified.
- Let the compilation finish
- Execute the compile again and ensure there's a rebuild.
Make the proc macro connect to a TCP server on a local thread
😨 🤣
Make the proc macro connect to a TCP server on a local thread
Sounds ideal but not that E-easy for me ;-)
Thank you, I'll share if I make any progress on this.
Ah yeah sorry this isn't necessarily E-easy any more per se, but if you've got a patch you feel fixes this I don't mind taking care of writing a test!
I personally view this as a feature rather than a bug, but it shouldn't be too too hard to fix this.
I think I disagree on both points :)
This behavior comes out in practice in non-esoteric circumstances:
- build is started
- you notice error, fix it and save a file, the build is still running
- build is finished
- subsequent
cargo builds are a no-op
A similar scenario was just reported in the ruRust gitter, with a slight difference that the build is cargo check, started by Emacs automatically (such automatic builds should inflate probabilities here).
Right now the mtime we compare against is the foo.d file, not the output file. It looks like rustc just generates foo.d far before the output in the compilation process.
This should just be a change to this line
I might be wrong here, but looks like this won't fix the problem and instead make it worse? So, we have src_time for the modified source file, depinfo_time for depinfo and output_time for output. Currently, when we fail to schedule a rebuild, we observe src_time < depinfo_time. And, because depinfo_time < output_time, swapping depinfo for output would't change the inequality.
The fundamental problem here is that we know the set of files to fingerprint (the depinfo), only after compilation is finished. So whichever fingerprint we calculate, there won't be a causality edge from it to the compilation, and we'll get a data race. Not even hashes will save us!
Action Item: I think a possible solution here is to use "mtime of the compilation process". That is, before we invoke rustc, we touch some file in a target directory and use its modification time, which will be less than depinfo_time or output_time.
I am not sure my solution would always work though. I use touch and not SystemTime::now to make sure times are actually correlated. The problem is that sources are in src, and we touch a file in target, and these directories might be on different filesystems, so their mtimes need not to be correlated. OTOH, this is already the case for depinfo time, so maybe this is not a problem?
Fundamentally, the problem is that filesystem just can't give us a consistent snapshot of files. It might be interesting to investigate approach which Bazel uses. I am not sure, but I think it copies all sources into the special read-only directory in the equivalent of target directory. That way, it creates an isolated fs enclave which has some consistency guarantees, because it isn't modified by anything except the build system itself.
Please note that there's a failed attempt (on my behalf) at fixing this in #5417. In particular I want to point out that it has a test for the desired behaviour, following Alex's guidelines.
A similar scenario was just reported in the ruRust gitter, with a slight difference that the build is cargo check, started by Emacs automatically (such automatic builds should inflate probabilities here).
It was me who reported it and I just checked - by default flycheck runs cargo test --no-run so that's why I had problems with running outdated tests afterwards: https://github.com/flycheck/flycheck/blob/master/flycheck.el#L9583-L9587
The worst part is that you don't know that tests/code is outdated and start to investigate what's wrong with your code when it's fine.
A similar scenario was just reported in the ruRust gitter, with a slight difference that the build is cargo check, started by Emacs automatically (such automatic builds should inflate probabilities here).
Something similar happens with cargo-web.
Ah good poitns @matklad! I think the solution of touching a file before the build starts should work well. I don't think it's really feasible to do sandboxing/enclaving in Cargo right now, and despite multiple fs issues I suspect that a file in the target directory will be "good enough" to solve the 95% of use cases
@matklad the fix for this in #6484 made a temporary file and read the mtime of that file. This was done instead of just SystemTime::now() because of your concerns that the file system may be on a different clock. I just noted that the windows documentation, does not appear to be concerned. The discussion in #5919 (comment) suggests that on linux SystemTime::now(), can be more precise then touching a file. Can you share your experiences with related problems so I can be appropriately careful?
@Eh2406 that was purely a theoretical concern: ideally, we don't rely on times at all and use a logical lamport-style "happens before" relation.
The discussion in #5919 (comment) suggests that on linux SystemTime::now(), can be more precise then touching a file.
This is not about precision, that is about time skew. Consider two machines, A and B, such that A mounts a folder form B's file system via NFS or something like that. The times on that shared folder may differ from times on other A's files not because of network delay, but because system times on A and B differ (I think in NFS setups local time is written to the filesystem?). I have no idea if a similar scenario works in practice at all.
Hm, actually, what happens if you just set the system time (daylight savings, ntpd or manual user intervention)? This won't affect times stored on the disk, right? This seems like a more reasonable scenario to me, though still quite unlikely.
The fact that the timestamp is not monotonic, is the most convincing argument I have herd for switching to a crypto-hash based system. I can't test it (my admin locked me out of manually setting the clock), but for example I think if you set the clock to the future and do a build, then reset the clock, then change the input file and try to rebuild nothing will happen. (The last build has a larger timestamp then the input file, so no rebuild is needed.) I also can't test (for the same reason) which clock is used when modifying a file on a network drive. Set your clock to the future, add a file to the network drive, does it have a creation time that is accurate (using the hosts clock) or in the future (using your clock). If it uses the hosts clock, and your clock is in the past, then having a target dir on the network drive can leave you in the first scenario.
Overall I am convinced that we should move away from relying on the timestamp ware performance allows it. I am not convinced that we solve any of the problems by avoiding SystemTime::now().
JUST PRESS Ctrl + S and then use cargo run command.
That solves the problem.
I haven't checked, but I would guess this is a timestamp issue: the input is modified before the output artifact is created, even though the modified input was not used to build the artifact. This would happen any time after rustc is parsing.
JUST PRESS Ctrl + S (which will update the code) and then use cargo run command.
That solves the problem.
@NajeebUllah161 please don't double comment, and don't shout, and avoid sentences that start with "just" they come across as more aggressive then you intended. Also when I was running into these issues my cargo build step took 10-30 min. I don't know about you but I could not sit at my computer and not touch my project for that long. Mostly because it broke my flow, but also because I could be fired for not doing anything work-related for that long.
This problem is not solved. I read that the problem is because somethin with the timestamp of the file and if the timestamp is always equals it doesn't compile at all. My computer have a problem with the time clock and it always reset after a time pass, so the timestamp of the file is sometimes the same even if it has diferent content and code.
@NajeebUllah161 please don't double comment, and don't shout, and avoid sentences that start with "just" they come across as more aggressive then you intended. Also when I was running into these issues my
cargo buildstep took 10-30 min. I don't know about you but I could not sit at my computer and not touch my project for that long. Mostly because it broke my flow, but also because I could be fired for not doing anything work-related for that long.
Shut up please.
@otaxhu even if the underlying causes the same, the pattern is different enough to justify a opening a new issue. Cargo does now have mitigations for modifying a file during compilation, so this issue is legitimately closed. Your issue about unreliable clock leading to not rebuilding when needed is legitimate and should be tracked as a new issue. Please open one.