Support absolute paths in dep-info files #7030

Mark-Simulacrum · 2019-06-12T20:35:19Z

These changes are a little more invasive then I would've liked, but I couldn't come up with a significantly better way to structure this. Comments (or backwards-compat) concerns are appreciated, of course!

cc rust-lang/rust#61727

r? @alexcrichton

alexcrichton

Looks good to me! To be clear the change here (if I read this right) is supporting where the target directory is not within the package root. This means that previous absolute paths to the target directory are now considered relative and we store what they're relative to so when it moves over time that's ok.

Can you be sure to add a test as well which fails before this change and passes after? Presumably moving the target directory out of the project should avoid rebuilds whereas today it should cause rebuilds.

alexcrichton · 2019-06-13T13:54:59Z

src/cargo/core/compiler/fingerprint.rs

 fn use_dep_info(unit: &Unit<'_>) -> bool {
-    let path = unit.pkg.summary().source_id().is_path();
-    !unit.mode.is_doc() && path
+    !unit.mode.is_doc()


Could this perhaps be a separate change? This is something I'd want to take more time to dig into the source and figure out what's going on and what the implications of this change are

Yep, I'll split this out. I agree that the implications are perhaps not entirely clear -- let me know if it'd be helpful for me to do some digging and provide a report of some sort.

I'm basically curious about the performance impact of this and how many locations call use_dep_info. I'm worried, for example, that we're going to try to open a git repository all over the place and cause issues. I don't mind looking into this though to see what's going on.

alexcrichton · 2019-06-13T13:57:05Z

src/cargo/core/compiler/fingerprint.rs

-        let path = absolute.strip_prefix(pkg_root).unwrap_or(&absolute);
-        new_contents.extend(util::path2bytes(path)?);
+        let file = PathBuf::from(file);
+        let (ty, path) = if file.is_relative() {


I think this can probably be simplified to perhaps only two cases, one which is maybe package-root relative and one which is target-root-relative. The path is always calculated the same here (rustc_cwd.join(file)), and then if it happens to be prefixed by the target root or package root we strip it and store such, and otherwise we don't gain a huge amount from having a separate absolute path variant.

I'm also more inclined to make the path absolute as fast as possible and then consider it all relative to various locations instead of checking for it being relative first, since I think that'll help various cwd switching mechanics over time.

I don't quite follow you here. Is the idea that we rely on foo.join(bar) not having any effect if bar is absolute?

Right yeah (and afaik that's documented behavior). It's something I rely on basically every time I call join at least :)

Mark-Simulacrum · 2019-06-13T15:40:11Z

I've dropped c5d6b5055bc5637f67fd3f3888c89eb0acbe7dfa from this PR and will open a separate one shortly with just that change. I suspect that's likely the cause of the test timeout but haven't tested [yet].

Mark-Simulacrum · 2019-06-13T15:46:37Z

Looks good to me! To be clear the change here (if I read this right) is supporting where the target directory is not within the package root. This means that previous absolute paths to the target directory are now considered relative and we store what they're relative to so when it moves over time that's ok.

That's a side-effect more so than the intent. Before rust-lang/rust#61727 dep-info files pretty much only contained src/foo.rs and other files in src, which meant that even in a workspace situation the subdirectory crates would still have all dep-info files relative to the package root, not the workspace root (where target is). With this we can handle that case better (since we support "workspace root" relative files)

Can you be sure to add a test as well which fails before this change and passes after? Presumably moving the target directory out of the project should avoid rebuilds whereas today it should cause rebuilds.

Yep, will work on a test case.

Mark-Simulacrum · 2019-06-13T18:32:20Z

I'm not sure the fix this produces can be reproduced without rust-lang/rust#61727. I've attempted to synthesize a non-relative path in the dep files (or even one that begins outside of the package root), but so far I've been unable to come up with anything that'll do so -- and looking at the rustc code it might not be possible. The existing tests without this patch already fail with the rustc PR and succeed with this patch and the PR.

The simple case of target directory moved out of package root we already support just fine.

Looks good to me! To be clear the change here (if I read this right) is supporting where the target directory is not within the package root. This means that previous absolute paths to the target directory are now considered relative and we store what they're relative to so when it moves over time that's ok.

That's a side-effect more so than the intent. Before rust-lang/rust#61727 dep-info files pretty much only contained src/foo.rs and other files in src, which meant that even in a workspace situation the subdirectory crates would still have all dep-info files relative to the package root, not the workspace root (where target is). With this we can handle that case better (since we support "workspace root" relative files)

To further clarify this without rust-lang/rust#61727 this patch is completely unnecessary as we can never get dep-info paths not based on the package root. With that PR though we get target/debug/....rlib and /path/to/sysroot/libstd...rlib which then breaks without taking into account workspace-root relative paths. This is also why there's not really a test I can come up with that'll only pass with this PR.

alexcrichton

I think it should be possible to reproduce this today by using codegen in a build script with include! pointing to an absolute path in the target directory. Using that could a test be added which renames the target directory and ensures that things aren't recompiled?

alexcrichton · 2019-06-14T17:22:54Z

src/cargo/core/compiler/fingerprint.rs

+        } else {
+            // It's definitely not target root relative, but this is an absolute path (since it was
+            // joined to rustc_cwd) and as such re-joining it later to the target root will have no
+            // effect.


Could this throw in an assert that it's absolute?

rustc wants to provide sysroot dependencies and perhaps eventually statically/dynamically linked C libraries discovered in library serach paths to Cargo. Mostly this is only useful today for rustbuild as otherwise Cargo's assumption that the sysroot is only changed if `rustc` itself changes is pretty much always correct.

Mark-Simulacrum · 2019-06-14T19:37:37Z

Okay, pushed up an update with the assertion and a test that fails on master but succeeds with this PR (presuming I tested right, but I'm pretty sure I did).

alexcrichton · 2019-06-14T23:36:27Z

@bors: r+

bors · 2019-06-14T23:36:28Z

📌 Commit 34fd5cc has been approved by alexcrichton

bors · 2019-06-14T23:36:36Z

⌛ Testing commit 34fd5cc with merge b1add4b...

@alexcrichton

…chton Support absolute paths in dep-info files These changes are a little more invasive then I would've liked, but I couldn't come up with a significantly better way to structure this. Comments (or backwards-compat) concerns are appreciated, of course! cc rust-lang/rust#61727 r? @alexcrichton

bors · 2019-06-15T00:02:22Z

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

Update cargo 17 commits in 807429e1b6da4e2ec52488ef2f59e77068c31e1f..4c1fa54d10f58d69ac9ff55be68e1b1c25ecb816 2019-06-11 14:06:10 +0000 to 2019-06-24 11:24:18 +0000 - Fix typo in comment (rust-lang/cargo#7066) - travis: enforce formatting of subcrates as well (rust-lang/cargo#7063) - _cargo: Make function style consistent (rust-lang/cargo#7060) - Update some fix comments. (rust-lang/cargo#7061) - Stabilize default-run (rust-lang/cargo#7056) - Fix typo in comment (rust-lang/cargo#7054) - fix(fingerpring): do not touch intermediate artifacts (rust-lang/cargo#7050) - Resolver test/debug cleanup (rust-lang/cargo#7045) - Rename to_url → into_url (rust-lang/cargo#7048) - Remove needless lifetimes (rust-lang/cargo#7047) - Revert test directory cleaning change. (rust-lang/cargo#7042) - cargo book /reference/manifest: fix typo (rust-lang/cargo#7041) - Extract resolver tests to their own crate (rust-lang/cargo#7011) - ci: Do not install addons on rustfmt build jobs (rust-lang/cargo#7038) - Support absolute paths in dep-info files (rust-lang/cargo#7030) - ci: Run cargo fmt on all workspaces (rust-lang/cargo#7033) - Deprecated ONCE_INIT in favor of Once::new() (rust-lang/cargo#7031)

Fix some issues with absolute paths in dep-info files. There were some issues with how #7030 was handling translating paths in dep-info files. The main consequence is that when coupled with rust-lang/rust#61727 (which has not yet merged), the fingerprint would fail and be considered dirty when it should be fresh. It was joining [`target_root`](https://github.com/rust-lang/cargo/blame/1140c527c4c3b3e2533b9771d67f88509ef7fc16/src/cargo/core/compiler/fingerprint.rs#L1352-L1360) which had 3 different values, but stripping [only one](https://github.com/rust-lang/cargo/blame/1140c527c4c3b3e2533b9771d67f88509ef7fc16/src/cargo/core/compiler/mod.rs#L323). This means for different scenarios (like using `--target`), it was creating incorrect paths in some cases. For example a target having a proc-macro dependency which would be in the host directory. The solution here is to always use CARGO_TARGET_DIR as the base that all relative paths are checked against. This should handle all host/target differences. The tests are marked with `#[ignore]` because 61727 has not yet merged. This includes a second commit (which I can open as a separate PR if needed) which is an alternate solution to #7034. It adds dep-info tracking for registry dependencies. However, it tries to limit which kinds of paths it tracks. It will not track package-relative paths (like source files). It also adds an mtime cache to significantly reduce the number of stat calls (because every dependency was stating every file in sysroot). Finally, I've run some tests using this PR with 61727 in rust-lang/rust. I can confirm that a second build is fresh (it doesn't erroneously rebuild). I can also confirm that the problem in rust-lang/rust#59105 has *finally* been fixed! My confidence in all this is pretty low, but I've been unable to think of any specific ways to make it fail. If anyone has ideas on how to better test this, let me know. Also, feel free to ask questions since I've been thinking about this a lot for the past few weeks, and there is quite a bit of subtle stuff going on.

rust-highfive assigned alexcrichton Jun 12, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2019

Mark-Simulacrum changed the title ~~Support more absolute paths in~~ Support absolute paths in dep-info files Jun 12, 2019

alexcrichton reviewed Jun 13, 2019

View reviewed changes

Mark-Simulacrum force-pushed the support-new-dep-info branch from c5d6b50 to 14037e8 Compare June 13, 2019 15:40

Mark-Simulacrum mentioned this pull request Jun 13, 2019

Utilize dep-info for all dependencies #7034

Closed

Mark-Simulacrum force-pushed the support-new-dep-info branch from 14037e8 to 7fe1a41 Compare June 13, 2019 18:32

Mark-Simulacrum mentioned this pull request Jun 13, 2019

Add binary dependencies to dep-info files rust-lang/rust#61727

Merged

ehuss mentioned this pull request Jun 13, 2019

Rustbuild: changing and testing libcore results in "found possibly newer version of crate std which rand depends on" rust-lang/rust#50481

Closed

alexcrichton reviewed Jun 14, 2019

View reviewed changes

Mark-Simulacrum force-pushed the support-new-dep-info branch from 7fe1a41 to 9ea3961 Compare June 14, 2019 19:35

Mark-Simulacrum force-pushed the support-new-dep-info branch from 9ea3961 to 34fd5cc Compare June 14, 2019 19:37

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 Jun 14, 2019

bors merged commit 34fd5cc into rust-lang:master Jun 15, 2019

Mark-Simulacrum deleted the support-new-dep-info branch June 15, 2019 00:17

ehuss mentioned this pull request Jun 24, 2019

Update cargo rust-lang/rust#62100

Merged

ehuss mentioned this pull request Jul 15, 2019

Fix some issues with absolute paths in dep-info files. #7137

Merged

alexcrichton mentioned this pull request Aug 6, 2019

cargo nightly build with RUSTFLAGS having --remap-path-prefix broke since last nightly on Windows #7211

Closed

ehuss added this to the 1.37.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support absolute paths in dep-info files #7030

Support absolute paths in dep-info files #7030

Mark-Simulacrum commented Jun 12, 2019

alexcrichton left a comment

alexcrichton Jun 13, 2019

Mark-Simulacrum Jun 13, 2019

alexcrichton Jun 13, 2019

alexcrichton Jun 13, 2019

Mark-Simulacrum Jun 13, 2019

alexcrichton Jun 13, 2019

Mark-Simulacrum commented Jun 13, 2019

Mark-Simulacrum commented Jun 13, 2019 •

edited

Loading

Mark-Simulacrum commented Jun 13, 2019

alexcrichton left a comment

alexcrichton Jun 14, 2019

Mark-Simulacrum commented Jun 14, 2019

alexcrichton commented Jun 14, 2019

bors commented Jun 14, 2019

bors commented Jun 14, 2019

bors commented Jun 15, 2019

Support absolute paths in dep-info files #7030

Support absolute paths in dep-info files #7030

Conversation

Mark-Simulacrum commented Jun 12, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Jun 13, 2019

Choose a reason for hiding this comment

Mark-Simulacrum Jun 13, 2019

Choose a reason for hiding this comment

alexcrichton Jun 13, 2019

Choose a reason for hiding this comment

alexcrichton Jun 13, 2019

Choose a reason for hiding this comment

Mark-Simulacrum Jun 13, 2019

Choose a reason for hiding this comment

alexcrichton Jun 13, 2019

Choose a reason for hiding this comment

Mark-Simulacrum commented Jun 13, 2019

Mark-Simulacrum commented Jun 13, 2019 • edited Loading

Mark-Simulacrum commented Jun 13, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Jun 14, 2019

Choose a reason for hiding this comment

Mark-Simulacrum commented Jun 14, 2019

alexcrichton commented Jun 14, 2019

bors commented Jun 14, 2019

bors commented Jun 14, 2019

bors commented Jun 15, 2019

Mark-Simulacrum commented Jun 13, 2019 •

edited

Loading