Be more conservative about which files are linked to the output dir. #5460

ehuss · 2018-05-02T18:19:08Z

This changes it so that only top-level targets requested on the command-line will be included in the output directory. Dependencies are no longer included.

Fixes #5444.

rust-highfive · 2018-05-02T18:19:11Z

r? @matklad

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

ehuss · 2018-05-02T18:20:54Z

I'm concerned about this breaking other projects. Let me know if you have ideas for public projects I could test that might be at risk of being affected.

matklad

LGTM, though I, being recently burnt with the --features flag, fear that this change might break someones build :)

Let's hold this off until the beta is branched perhaps?

matklad · 2018-05-02T18:21:36Z

src/cargo/core/compiler/context/compilation_files.rs

-        let &(ref dir, ref roots) = self.export_dir.as_ref()?;
-        if roots.contains(unit) {
-            Some(dir.clone())
+        if self.roots.contains(unit) {


I think this check is no longer necessary, because we now do the same filtering for target_dir and export_dir

That sounds correct, I have removed it.

matklad · 2018-05-02T18:24:31Z

Let me know if you have ideas for public projects I could test that might be at risk of being affected.

Servo definitely is worth checking!

alexcrichton · 2018-05-02T18:43:43Z

src/cargo/core/compiler/context/compilation_files.rs

-            if self.ws.members().find(|&p| p == unit.pkg).is_none() && !unit.target.is_bin() {
-                None
-            } else {
+            if self.roots.contains(unit) {


I think this is a little suspect in the sense that this is pretty carefully crafted and I'm wary to switch wholesale to roots. Could we perhaps start off by basically skipping any rlib units? That is, we only care about binaries in the main workspace, mostly (and dylib/staticlib/cdylib/etc)

We can do that, though I felt like that would be a bigger change. It would still leave behind the race condition for the other output types (particularly rmeta files, though they don't cause catastrophic failures).

I made an experimental branch to try this here: ehuss@68e7d2e
It seems more risky to me, though.

It seems like a pretty heavy change just to avoid these non-panic dependencies. I'm still trying to think if there's an easier way...

Hm ok yeah it's definitely not worth this becoming a larger change.

So today the logic for "should I hardlink this up" is basically "am I a workspace member or am I a binary". That encompasses far too much as workspace member rlibs/rmeta have no business being linked up.

So this is instead saying that "if you were originally requested you're linked up". That includes libraries and rmeta business but you can't request more than one (even though we may compile many), so we should be resolving the race this way.

Writing this all down, I think the only difference is that all other workspace members aren't automatically linked up. I don't think, however, that it was possible for a workspace member to generate a relevant binary/staticlib for us to link upwards.

Or well, in other words, I think that this should be correct. I can't think of a way in which it would be wrong at least! In light of that I think we should be good to merge as soon as beta is branched.

I think what might go wrong are dynamic libraries. We add top target directory to LD_LIBRARY_PATH:

cargo/src/cargo/core/compiler/compilation.rs

Lines 154 to 155 in 6cd841f

search_path.push(self.root_output.clone());

search_path.push(self.deps_output.clone());

However, in the next line we add the deps dir as well, so we should be fine here as well!

ehuss · 2018-05-02T19:32:57Z

Let's hold this off until the beta is branched perhaps?

If we delay, we should probably temporarily disable the two _build_all_targets tests I added to avoid spurious CI errors.

alexcrichton · 2018-05-02T19:39:40Z

I think this is fixing active regressions, though, right? In that sense we may not want to delay it?

ehuss · 2018-05-02T20:18:31Z

I think this is fixing active regressions, though, right?

Not exactly. cargo build --all-targets with a panic profile didn't work at all in previous versions. The only other way I can think of to trigger the race condition is to use the new build-override profile with a shared dependency in a workspace.

alexcrichton · 2018-05-02T22:31:04Z

Ah ok makes sense! Then yeah it's probably good to hold off until next week to merget (but we can sort out in the meantime)

This changes it so that only top-level targets requested on the command-line will be included in the output directory. Dependencies are no longer included. Fixes rust-lang#5444.

ehuss · 2018-05-10T18:10:11Z

I've been trying to think of a less risky way to implement this, but I haven't been able to come up with anything good. Let me know if I should try something else. Unfortunately I'm not familiar with the use cases where a crate would depend on a cdylib/staticlib, so I'm not clear if/how it might be a problem.

alexcrichton · 2018-05-10T18:41:11Z

@bors: r+

bors · 2018-05-10T18:41:12Z

📌 Commit d0c2939 has been approved by alexcrichton

bors · 2018-05-10T19:01:12Z

⌛ Testing commit d0c2939 with merge ef719bc...

Be more conservative about which files are linked to the output dir. This changes it so that only top-level targets requested on the command-line will be included in the output directory. Dependencies are no longer included. Fixes #5444.

bors · 2018-05-10T19:54:59Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing ef719bc to master...

Fixes rust-lang#5435 Also re-enable tests now that rust-lang#5460 has landed.

Fix `panic` for binaries built during tests. Fixes #5435 Also re-enable tests now that #5460 has landed.

Regression fixes: - rust-lang/cargo#5503 - cargo rustc broken for tests in project with bins - rust-lang/cargo#5520 - (rust-lang#50640) shared proc-macro dependency built incorrectly Changes: - rust-lang/cargo#5522 - Add option to set user-agent - rust-lang/cargo#5519 - NFC: fix a couple of typos, found by codespell. - rust-lang/cargo#5513 - Fix `panic` for binaries built during tests. - rust-lang/cargo#5512 - simplify build_requirements - rust-lang/cargo#5301 - Add --build-plan for 'cargo build' - rust-lang/cargo#5460 - Be more conservative about which files are linked to the output dir. - rust-lang/cargo#5509 - Use the new stable - rust-lang/cargo#5507 - Does not print seconds fraction with minutes - rust-lang/cargo#5498 - Bump to 0.29.0 - rust-lang/cargo#5497 - Mention +nightly in ARCHITECTURE.md

Update Cargo Regression fixes: - rust-lang/cargo#5503 - cargo rustc broken for tests in project with bins - rust-lang/cargo#5520 - (rust-lang#50640) shared proc-macro dependency built incorrectly Changes: - rust-lang/cargo#5522 - Add option to set user-agent - rust-lang/cargo#5519 - NFC: fix a couple of typos, found by codespell. - rust-lang/cargo#5513 - Fix `panic` for binaries built during tests. - rust-lang/cargo#5512 - simplify build_requirements - rust-lang/cargo#5301 - Add --build-plan for 'cargo build' - rust-lang/cargo#5460 - Be more conservative about which files are linked to the output dir. - rust-lang/cargo#5509 - Use the new stable - rust-lang/cargo#5507 - Does not print seconds fraction with minutes - rust-lang/cargo#5498 - Bump to 0.29.0 - rust-lang/cargo#5497 - Mention +nightly in ARCHITECTURE.md

Unblocking PRs: - rust-lang/cargo#5535 - Ignore <tab> in libtest output. (unblocks rust-lang#50387) - rust-lang/cargo#5537 - Remove -Zno-trans test. (unblocks rust-lang#50615) - rust-lang/cargo#5540 - Fix tests when CARGO_TARGET_DIR is set. (unblocks self) Regression fixes: - rust-lang/cargo#5503 - cargo rustc broken for tests in project with bins - rust-lang/cargo#5520 - (rust-lang#50640) shared proc-macro dependency built incorrectly Changes: - rust-lang/cargo#5527 - Point Source Replacement to the Overriding Dependencies section - rust-lang/cargo#5533 - Detail how to run locally-built nightly cargo - rust-lang/cargo#5522 - Add option to set user-agent - rust-lang/cargo#5519 - NFC: fix a couple of typos, found by codespell. - rust-lang/cargo#5513 - Fix `panic` for binaries built during tests. - rust-lang/cargo#5512 - simplify build_requirements - rust-lang/cargo#5301 - Add --build-plan for 'cargo build' - rust-lang/cargo#5460 - Be more conservative about which files are linked to the output dir. - rust-lang/cargo#5509 - Use the new stable - rust-lang/cargo#5507 - Does not print seconds fraction with minutes - rust-lang/cargo#5498 - Bump to 0.29.0 - rust-lang/cargo#5497 - Mention +nightly in ARCHITECTURE.md

Update Cargo Unblocking PRs: - rust-lang/cargo#5535 - Ignore <tab> in libtest output. (unblocks rust-lang#50387) - rust-lang/cargo#5537 - Remove -Zno-trans test. (unblocks rust-lang#50615) - rust-lang/cargo#5540 - Fix tests when CARGO_TARGET_DIR is set. (unblocks self) Regression fixes: - rust-lang/cargo#5503 - cargo rustc broken for tests in project with bins - rust-lang/cargo#5520 - (rust-lang#50640) shared proc-macro dependency built incorrectly Changes: - rust-lang/cargo#5527 - Point Source Replacement to the Overriding Dependencies section - rust-lang/cargo#5533 - Detail how to run locally-built nightly cargo - rust-lang/cargo#5522 - Add option to set user-agent - rust-lang/cargo#5519 - NFC: fix a couple of typos, found by codespell. - rust-lang/cargo#5513 - Fix `panic` for binaries built during tests. - rust-lang/cargo#5512 - simplify build_requirements - rust-lang/cargo#5301 - Add --build-plan for 'cargo build' - rust-lang/cargo#5460 - Be more conservative about which files are linked to the output dir. - rust-lang/cargo#5509 - Use the new stable - rust-lang/cargo#5507 - Does not print seconds fraction with minutes - rust-lang/cargo#5498 - Bump to 0.29.0 - rust-lang/cargo#5497 - Mention +nightly in ARCHITECTURE.md

Unblocking PRs: - rust-lang/cargo#5535 - Ignore <tab> in libtest output. (unblocks rust-lang#50387) - rust-lang/cargo#5537 - Remove -Zno-trans test. (unblocks rust-lang#50615) - rust-lang/cargo#5540 - Fix tests when CARGO_TARGET_DIR is set. (unblocks self) Regression fixes: - rust-lang/cargo#5503 - cargo rustc broken for tests in project with bins - rust-lang/cargo#5520 - (rust-lang#50640) shared proc-macro dependency built incorrectly Changes: - rust-lang/cargo#5527 - Point Source Replacement to the Overriding Dependencies section - rust-lang/cargo#5533 - Detail how to run locally-built nightly cargo - rust-lang/cargo#5522 - Add option to set user-agent - rust-lang/cargo#5519 - NFC: fix a couple of typos, found by codespell. - rust-lang/cargo#5513 - Fix `panic` for binaries built during tests. - rust-lang/cargo#5512 - simplify build_requirements - rust-lang/cargo#5301 - Add --build-plan for 'cargo build' - rust-lang/cargo#5460 - Be more conservative about which files are linked to the output dir. - rust-lang/cargo#5509 - Use the new stable - rust-lang/cargo#5507 - Does not print seconds fraction with minutes - rust-lang/cargo#5498 - Bump to 0.29.0 - rust-lang/cargo#5497 - Mention +nightly in ARCHITECTURE.md

Update Cargo Unblocking PRs: - rust-lang/cargo#5535 - Ignore tab in libtest output. (unblocks #50387) - rust-lang/cargo#5537 - Remove -Zno-trans test. (unblocks #50615) - rust-lang/cargo#5540 - Fix tests when CARGO_TARGET_DIR is set. (unblocks self) Regression fixes: - rust-lang/cargo#5503 - cargo rustc broken for tests in project with bins - rust-lang/cargo#5520 - shared proc-macro dependency built incorrectly Changes: - rust-lang/cargo#5527 - Point Source Replacement to the Overriding Dependencies section - rust-lang/cargo#5533 - Detail how to run locally-built nightly cargo - rust-lang/cargo#5522 - Add option to set user-agent - rust-lang/cargo#5519 - NFC: fix a couple of typos, found by codespell. - rust-lang/cargo#5513 - Fix `panic` for binaries built during tests. - rust-lang/cargo#5512 - simplify build_requirements - rust-lang/cargo#5301 - Add --build-plan for 'cargo build' - rust-lang/cargo#5460 - Be more conservative about which files are linked to the output dir. - rust-lang/cargo#5509 - Use the new stable - rust-lang/cargo#5507 - Does not print seconds fraction with minutes - rust-lang/cargo#5498 - Bump to 0.29.0 - rust-lang/cargo#5497 - Mention +nightly in ARCHITECTURE.md The PR fixes #50640.

There's a case where Cargo will recompile a project even if the fingerprint looks like it's fresh, when some output files are missing. This was intended to cover the case where an output file was deleted manually or otherwise messed with. The check here was a bit too eager, however. It checked not only the actual output destination of the compiler but *also* the location that we hard link the output file up to. Due to recent changes in rust-lang#5460 we don't always create the hard links for path dependencies in the top-level dir, and this meant that if the library were compiled and then tested later on the test may recompile the original library by accident. The fix in this commit is to cease looking for the hardlink if it exists or not. This way we only check for the presence of the output file itself and only recompile if that file is missing. The reason for this is that we unconditionally relink files into place whether it's fresh or not, so we'll always recreate the hard link anyway if it's missing. cc rust-lang/rust#51717

Fix avoiding a rebuild when moving around a workspace There's a case where Cargo will recompile a project even if the fingerprint looks like it's fresh, when some output files are missing. This was intended to cover the case where an output file was deleted manually or otherwise messed with. The check here was a bit too eager, however. It checked not only the actual output destination of the compiler but *also* the location that we hard link the output file up to. Due to recent changes in #5460 we don't always create the hard links for path dependencies in the top-level dir, and this meant that if the library were compiled and then tested later on the test may recompile the original library by accident. The fix in this commit is to cease looking for the hardlink if it exists or not. This way we only check for the presence of the output file itself and only recompile if that file is missing. The reason for this is that we unconditionally relink files into place whether it's fresh or not, so we'll always recreate the hard link anyway if it's missing. cc rust-lang/rust#51717

There's a case where Cargo will recompile a project even if the fingerprint looks like it's fresh, when some output files are missing. This was intended to cover the case where an output file was deleted manually or otherwise messed with. The check here was a bit too eager, however. It checked not only the actual output destination of the compiler but *also* the location that we hard link the output file up to. Due to recent changes in rust-lang#5460 we don't always create the hard links for path dependencies in the top-level dir, and this meant that if the library were compiled and then tested later on the test may recompile the original library by accident. The fix in this commit is to cease looking for the hardlink if it exists or not. This way we only check for the presence of the output file itself and only recompile if that file is missing. The reason for this is that we unconditionally relink files into place whether it's fresh or not, so we'll always recreate the hard link anyway if it's missing. cc rust-lang/rust#51717

@alexcrichton

Fix avoiding a rebuild when moving around a workspace This is a backport of #5669. There's a case where Cargo will recompile a project even if the fingerprint looks like it's fresh, when some output files are missing. This was intended to cover the case where an output file was deleted manually or otherwise messed with. The check here was a bit too eager, however. It checked not only the actual output destination of the compiler but *also* the location that we hard link the output file up to. Due to recent changes in #5460 we don't always create the hard links for path dependencies in the top-level dir, and this meant that if the library were compiled and then tested later on the test may recompile the original library by accident. The fix in this commit is to cease looking for the hardlink if it exists or not. This way we only check for the presence of the output file itself and only recompile if that file is missing. The reason for this is that we unconditionally relink files into place whether it's fresh or not, so we'll always recreate the hard link anyway if it's missing. cc rust-lang/rust#51717 r? @alexcrichton

Fix dylib reuse with uplift. Due to #5460, the files that are copied to the root of the target dir depend on whether or not it is a direct target. This causes a problem with dylibs if you build a dylib directly, and then separately run a target that depends on that dylib. It will run with the dylib path preferring the root dir, which picks up the old dylib. This PR swaps the preference of the dylib search path so that the dylib in the `deps` dir is preferred. This is maybe not the best solution for dynamic dependencies. I can imagine if you are trying to create a package, you'll want all of the shared libs along with the binary, and digging through the `deps` dir is not optimal. However, unconditionally linking dependencies into the root has issues (which #5460 fixed). There are other issues because shared libs do not include a hash in the filename. A grander solution might involve coming up with a better strategy for organizing the `target` directory to avoid conflicts between targets. Fixes #6162

rust-highfive assigned matklad May 2, 2018

matklad reviewed May 2, 2018

View reviewed changes

alexcrichton reviewed May 2, 2018

View reviewed changes

ehuss mentioned this pull request May 9, 2018

Fix cargo rustc for test with implicit binary. #5503

Merged

ehuss added 4 commits May 10, 2018 08:46

Be more conservative about which files are linked to the output dir.

27edc43

This changes it so that only top-level targets requested on the command-line will be included in the output directory. Dependencies are no longer included. Fixes rust-lang#5444.

Fix broken test.

f4dc629

Update due to rust-lang#5503.

251502b

Address review comment, no need to check export_dir.

d0c2939

ehuss force-pushed the conservative-link branch from eb39909 to d0c2939 Compare May 10, 2018 16:35

bors merged commit d0c2939 into rust-lang:master May 10, 2018

ehuss added a commit to ehuss/cargo that referenced this pull request May 10, 2018

Fix panic for binaries built during tests.

3071734

Fixes rust-lang#5435 Also re-enable tests now that rust-lang#5460 has landed.

ehuss mentioned this pull request May 10, 2018

Fix panic for binaries built during tests. #5513

Merged

bors added a commit that referenced this pull request May 10, 2018

Auto merge of #5513 - ehuss:test-binary-panic, r=alexcrichton

dfaba34

Fix `panic` for binaries built during tests. Fixes #5435 Also re-enable tests now that #5460 has landed.

ehuss mentioned this pull request May 12, 2018

Update Cargo rust-lang/rust#50696

Merged

alexcrichton mentioned this pull request Jun 29, 2018

Fix avoiding a rebuild when moving around a workspace #5669

Merged

Mark-Simulacrum mentioned this pull request Jun 29, 2018

Fix avoiding a rebuild when moving around a workspace #5671

Merged

ehuss mentioned this pull request Oct 4, 2018

cargo test without cargo build won't create target/debug/libfoo.rlib while cargo build does #6131

Closed

ehuss mentioned this pull request Oct 12, 2018

Fix dylib reuse with uplift. #6167

Merged

ehuss mentioned this pull request Nov 13, 2018

Duplicate artifact tracking issue. #6313

Open

ehuss added this to the 1.28.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Be more conservative about which files are linked to the output dir. #5460

Be more conservative about which files are linked to the output dir. #5460

ehuss commented May 2, 2018

rust-highfive commented May 2, 2018

ehuss commented May 2, 2018

matklad left a comment

matklad May 2, 2018

ehuss May 10, 2018

matklad commented May 2, 2018

alexcrichton May 2, 2018

ehuss May 3, 2018

alexcrichton May 3, 2018

matklad May 3, 2018

ehuss commented May 2, 2018

alexcrichton commented May 2, 2018

ehuss commented May 2, 2018

alexcrichton commented May 2, 2018

ehuss commented May 10, 2018

alexcrichton commented May 10, 2018

bors commented May 10, 2018

bors commented May 10, 2018

bors commented May 10, 2018

	search_path.push(self.root_output.clone());
	search_path.push(self.deps_output.clone());

Be more conservative about which files are linked to the output dir. #5460

Be more conservative about which files are linked to the output dir. #5460

Conversation

ehuss commented May 2, 2018

rust-highfive commented May 2, 2018

ehuss commented May 2, 2018

matklad left a comment

Choose a reason for hiding this comment

matklad May 2, 2018

Choose a reason for hiding this comment

ehuss May 10, 2018

Choose a reason for hiding this comment

matklad commented May 2, 2018

alexcrichton May 2, 2018

Choose a reason for hiding this comment

ehuss May 3, 2018

Choose a reason for hiding this comment

alexcrichton May 3, 2018

Choose a reason for hiding this comment

matklad May 3, 2018

Choose a reason for hiding this comment

ehuss commented May 2, 2018

alexcrichton commented May 2, 2018

ehuss commented May 2, 2018

alexcrichton commented May 2, 2018

ehuss commented May 10, 2018

alexcrichton commented May 10, 2018

bors commented May 10, 2018

bors commented May 10, 2018

bors commented May 10, 2018