Fix freshness when linking is interrupted. #8087

ehuss · 2020-04-08T19:48:06Z

Fixes a scenario where hitting Ctrl-C while linking would leave a corrupted executable, but Cargo would think it is "fresh" and fail to rebuild it.

This also includes a separate commit which adds more documentation on fingerprinting.

Fixes #7767

rust-highfive · 2020-04-08T19:48:10Z

r? @alexcrichton

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

ehuss · 2020-04-08T19:50:23Z

Truncating the file seems a little hacky, but it seemed like a reasonable way to invalidate the fingerprint. I don't want to delete it because that would break the compare debug logging in some cases.

bors · 2020-04-09T14:46:59Z

☔ The latest upstream changes (presumably #8073) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton

Thanks for the docs! I've got a question about the scenario this is fixing, but in general looks great to me

alexcrichton · 2020-04-09T14:28:43Z

tests/testsuite/freshness.rs

+    // Interrupt the child.
+    child.kill().unwrap();
+    // Note: rustc and the linker are still running, let them exit here.
+    conn.write(b"X").unwrap();


Technically it should be sufficient to drop the conn here rather than writing to it, but either way is fine.

alexcrichton · 2020-04-09T15:21:14Z

src/cargo/core/compiler/fingerprint.rs

+    // up-to-date until after a successful build.
+    if loc.exists() {
+        paths::write(&loc, b"")?;
+    }


Trying to make sure I fully understand this. So today we consider step 4 fresh, but it should actually rerun because the linker didn't actually finish successfully. When linking is interrupted the compiler has already emitted the dep-info file, so when we finish the unit and wrap up the fingerprint stuff it looks like it succeeded because the output file exists and the dep-info exists? Is that right?

This feels like it may be best handled by we truncate the file if the process failed for whatever reason. I'm a little hazy on this though. Is the idea that this truncation happens in step 3, which when interrupted, means step 4 picks up the truncated file?

and the dep-info exists? Is that right?

I don't think it matters whether or not rustc emitted the .d file. Cargo doesn't translate it into the fingerprint directory unless the compilation finished successfully.

The issue is that there is a valid fingerprint from the previous run (step 1). During step 3, the new ~~unit~~ integration test executable is created but not completed. In step 4, all components of the ~~unit~~ integration test fingerprint are fresh from step 1 (nothing changed), and the mtime comparison of the incomplete executable is newer than the library's rlib (and all the ~~unit~~ integration test source files), so it thinks it is fresh.

truncate the file if the process failed

I don't think we can only truncate on failure, because Cargo is forcefully terminated. (I guess we could catch signals but that doesn't help with SIGKILL, and I'd prefer not to deal with signals.)

Is the idea that this truncation happens in step 3, which when interrupted, means step 4 picks up the truncated file?

Yes, step 4 compares an empty string with the expected hash string, and determines it is dirty because they are not equal.

Man see this is when I have mega-doubts about everything. Doing anything with mtimes is so tricky that it seems like a folly almost. In any case agreed we shouldn't catch signals (was misinterpreting this for a second as failed rustc vs interrupted cargo).

I think the way we should think about this is that when we start a compilation we delete the expected outputs. That way if something bad happens, the next time we pick up the outputs won't exist and we'll see we need to recompile. This only works if the outputs are Cargo-created, not well-known files (like the linked artifact). The dep-info here, consequently, is a Cargo-output which only Cargo creates, so it makes sense to delete it before a build and create it after a successful build.

Would it perhaps make more sense to delete the file here rather than truncating it to zero bytes?

The reason it truncates instead of deleting is that the fingerprint debug logging doesn't work in the scenario:

Successful build.

Change something.

Build with a compile error. Logs fingerprint reason. Delete fingerprint.

Build again. Fingerprint log just says "file not found" with no other detail.

I wanted to make sure step 4 continued to log the real reason for the fingerprint error (like "current filesystem status shows we're outdated") rather than "file not found".

I don't think it is too important (the first build failure logs the real reason), so I can switch it to delete if you prefer. I'm curious what concerns you have about truncating, though?

Nah that makes sense to me, so I think truncation here is fine. Could you update the comment with some of this discussion though? (about what files are flowing into what steps and why we're truncating instead of deleting).

I'm slightly confused again though. I would have thought that step 4 can't read the fingerprint file of step 1 (without this patch) because the mtime of the input files (from step 2) shows that the fingerprint file is stale. The mtime of the file from step 2 would be later than that of the fingerprint file from step 1, right?

(or do we write out the fingerprint file again in step 3?)

Checking mtime is done at the same time as checking the fingerprint hash here. So in a sense it checks both the fingerprint hash and the fs_status at the same time. If either of them fail, it falls through to the code below to log the failure.

If it deleted the file, the first line in compare_old_fingerprint would fail, circumventing the logging.

(Fingerprints are only written after a successful build.)

Er sorry I was roundaboutedly asking a question on your response above where you said

The issue is that there is a valid fingerprint from the previous run (step 1). During step 3, the new unit test executable is created but not completed. In step 4, all components of the unit test fingerprint are fresh from step 1 (nothing changed), and the mtime comparison of the incomplete executable is newer than the library's rlib (and all the unit test source files), so it thinks it is fresh.

You say "all components of the unit test fingerprint are fresh from step 1" but I thought that the final fingerprint file that Cargo writes for the executable woul dnot be fresh, right? The executable is there (and corrupt) but a source file is still newer than the final fingerprint file because we don't write that until rustc finishes I thought?

Sorry, I made a typo in my reply. Where I said "unit test" I meant "integration test". Going back to the original example, "lib.rs" is changed, and compiles successfully. Then Cargo goes to build the integration test executable, and is interrupted. The next time around (step 4) "lib.rs" is up-to-date (and correct on-disk). And none of the integration test sources have changed, so that check appears up-to-date as well.

That was a bit of a subtle point, I'll emphasize that in the comment.

I pushed a commit with extra details. Hopefully it's not too confusing, I tried to be more precise with the wording.

alexcrichton · 2020-04-10T16:33:20Z

@bors: r+

Ah ok that was a key part I was missing. This workaround isn't necessary for a one-unit compilation, but it's required for multi-unit compilations where you don't change the source of previous compilations. This happens because we don't hash the contents of each output, we just rely on mtimes. In any case looks good to me :)

bors · 2020-04-10T16:33:21Z

📌 Commit 14e86cc has been approved by alexcrichton

bors · 2020-04-10T16:33:29Z

⌛ Testing commit 14e86cc with merge 53b1c48...

bors · 2020-04-10T16:52:03Z

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 53b1c48 to master...

Update cargo 12 commits in 390e8f245ef2cd7ac698b8a76abf029f9abcab0d..74e3a7d5b756d7c0e94399fc29fcd154e792c22a 2020-04-07 17:46:45 +0000 to 2020-04-13 20:41:52 +0000 - Update dependencies to support illumos target (rust-lang/cargo#8093) - Whitelist another known spurious curl error (rust-lang/cargo#8102) - Fix nightly test matching rustc "warning" output. (rust-lang/cargo#8098) - Update default for codegen-units. (rust-lang/cargo#8096) - Fix freshness when linking is interrupted. (rust-lang/cargo#8087) - Add `cargo tree` command. (rust-lang/cargo#8062) - Add "build-finished" JSON message. (rust-lang/cargo#8069) - Extend -Zpackage-features with more capabilities. (rust-lang/cargo#8074) - Disallow invalid dependency names through crate renaming (rust-lang/cargo#8090) - Use the same filename hash for pre-release channels. (rust-lang/cargo#8073) - Index the commands section (rust-lang/cargo#8081) - Upgrade to mdBook v0.3.7 (rust-lang/cargo#8083)

rust-highfive assigned alexcrichton Apr 8, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 8, 2020

alexcrichton reviewed Apr 9, 2020

View reviewed changes

ehuss added 2 commits April 9, 2020 08:42

Add some more exposition on fingerprints.

cc53eca

Fix freshness when linking is interrupted.

cd396f3

ehuss force-pushed the freshness-interrupted2 branch from 8c54e58 to cd396f3 Compare April 9, 2020 15:46

More fingerprint and metadata comments.

14e86cc

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2020

bors merged commit 53b1c48 into rust-lang:master Apr 10, 2020

bors mentioned this pull request Apr 10, 2020

Refactor BuildContext #8068

Merged

ehuss mentioned this pull request Apr 14, 2020

Update cargo rust-lang/rust#71138

Merged

ehuss mentioned this pull request Apr 29, 2020

Running broken build Syntax error: ")" unexpected #8182

Closed

alexcrichton mentioned this pull request Nov 9, 2020

Interrupting cargo can cause linker errors #8838

Open

ehuss added this to the 1.44.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix freshness when linking is interrupted. #8087

Fix freshness when linking is interrupted. #8087

ehuss commented Apr 8, 2020

rust-highfive commented Apr 8, 2020

ehuss commented Apr 8, 2020

bors commented Apr 9, 2020

alexcrichton left a comment

alexcrichton Apr 9, 2020

alexcrichton Apr 9, 2020

ehuss Apr 9, 2020 •

edited

Loading

alexcrichton Apr 9, 2020

ehuss Apr 9, 2020

alexcrichton Apr 9, 2020

ehuss Apr 9, 2020

alexcrichton Apr 9, 2020

ehuss Apr 9, 2020

ehuss Apr 9, 2020

alexcrichton commented Apr 10, 2020

bors commented Apr 10, 2020

bors commented Apr 10, 2020

bors commented Apr 10, 2020

Fix freshness when linking is interrupted. #8087

Fix freshness when linking is interrupted. #8087

Conversation

ehuss commented Apr 8, 2020

rust-highfive commented Apr 8, 2020

ehuss commented Apr 8, 2020

bors commented Apr 9, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Apr 10, 2020

bors commented Apr 10, 2020

bors commented Apr 10, 2020

bors commented Apr 10, 2020

ehuss Apr 9, 2020 •

edited

Loading