Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cargo build does not rebuild if a source file was modified during a build #2426

Closed
kamalmarhubi opened this issue Mar 1, 2016 · 32 comments · Fixed by #6484
Closed

cargo build does not rebuild if a source file was modified during a build #2426

kamalmarhubi opened this issue Mar 1, 2016 · 32 comments · Fixed by #6484

Comments

@kamalmarhubi
Copy link
Contributor

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.

@kamalmarhubi
Copy link
Contributor Author

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

@alexcrichton
Copy link
Member

I personally view this as a feature rather than a bug, but it shouldn't be too too hard to fix this.

@alexcrichton alexcrichton added the E-easy Experience: Easy label Mar 1, 2016
@kamalmarhubi
Copy link
Contributor Author

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. :-)

@kamalmarhubi
Copy link
Contributor Author

(Though the fix to the greater issue would likely require changing from timestamps to hashes to determine if inputs have changed.)

@alexcrichton
Copy link
Member

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

@srinivasreddy
Copy link
Contributor

Anyone working on this?

@alexcrichton
Copy link
Member

I don't believe so, no, feel free to take it!

@alexcrichton
Copy link
Member

@Jimmio92 that sounds distinct from this issue, feel free to open a separate one!

@Jimmio92
Copy link

Jimmio92 commented May 13, 2016

@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.

@alexcrichton
Copy link
Member

Aha, even better! Glad it worked out :)

@bmusin
Copy link

bmusin commented Jan 10, 2018

Does the problem exist? Maybe issue should be closed?

@alexcrichton
Copy link
Member

Yes I believe this is still an issue unfortunately

@dwijnand
Copy link
Member

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?

@matklad
Copy link
Member

matklad commented Apr 17, 2018

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.

@alexcrichton
Copy link
Member

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.

@matklad
Copy link
Member

matklad commented Apr 17, 2018

Make the proc macro connect to a TCP server on a local thread

😨 🤣

@dwijnand
Copy link
Member

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.

@alexcrichton
Copy link
Member

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!

@matklad
Copy link
Member

matklad commented Jun 9, 2018

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.

@dwijnand
Copy link
Member

dwijnand commented Jun 9, 2018

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.

@kurnevsky
Copy link

kurnevsky commented Jun 9, 2018

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.

@alexander-irbis
Copy link

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.

@alexcrichton
Copy link
Member

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

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 7, 2019

@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?

@matklad
Copy link
Member

matklad commented Jan 7, 2019

@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.

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 9, 2019

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().

@NajeebUllah161

This comment was marked as spam.

@NajeebUllah161

This comment was marked as abuse.

@Eh2406
Copy link
Contributor

Eh2406 commented Jun 9, 2020

@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.

@otaxhu
Copy link

otaxhu commented Jul 25, 2023

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

This comment was marked as abuse.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 26, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.