Add binary dependencies to dep-info files

[Mark-Simulacrum]

I'm not sure about the lack of incremental-tracking here, but since I'm pretty sure this runs on every compile anyway it might not matter? If there's a better place/way to get at the information I want, I'm happy to refactor the code to match.

r? @alexcrichton

[Mark-Simulacrum]

Based on local testing once this is in the beta compiler we can delete in-stage clear_if_dirty calls in bootstrap and things will continue to work (and, indeed, become better -- since e.g. codegen backends will stop depending on all of the compiler crates and instead properly depend on just some of the compiler crates). We'll still need inter-stage calls since we don't emit dependencies on rustc or codegen backend files with this patch (and that's probably out of scope). However, the sysroot itself is tracked.

[cramertj]

Does this include all crates in library search paths, or only the ones brought in via extern crate?

[Mark-Simulacrum]

I believe it'll be only those "used" (extern crate, use, etc)

[cramertj]

Can you add a test confirming this?

[Mark-Simulacrum]

I think such a test would be relatively complicated (probably some sort of run-make, I guess) but basically the crates_untracked() bit here starting to pick up all tests would be noticed by plenty of other tests AFAIK since it interacts with incremental, etc. too. I have confirmed manually that not all crates in the sysroot and other search paths show up.

[cramertj]

cc #57717

[alexcrichton]

So to confirm that I understand the change here, the purpose of this is that rustbuild wants to do less manual cleaning and instead rely on Cargo as much as it can to automatically rebuild things. Rustbuild will always execute Cargo 6 times (std0, test0, rustc0, std1, test1, rustc1), but the idea is that today between each of these invocations we have this weird clear_if_dirty thing whereas after this patch we'll only need clear_if_dirty between rustc0 and std1 (which is much easier since it's just checking the timestamp of the rustc binary rather than a bunch of sysroot things).

The purpose of this patch is to tell Cargo more about the dep-info of inputs. Cargo currently today uses dep-info to learn about source code to recompile when something changes, but naturally if any of the binary dependencies (like libstd) change the crate also needs to be recompiled. That's the purpose of this, right?

Can you also test out that this doesn't break Cargo? It should be the case that PATH=/path/to/rustc/build/target/stage2/bin:$PATH cargo test in Cargo itself should still pass all tests, but I'd like to make sure that this doesn't accidentally break Cargo's own logic in recompilation. (if it does we can just be sure to update that and land this at the same time).

[Mark-Simulacrum]

So to confirm that I understand the change here, the purpose of this is that rustbuild wants to do less manual cleaning and instead rely on Cargo as much as it can to automatically rebuild things. Rustbuild will always execute Cargo 6 times (std0, test0, rustc0, std1, test1, rustc1), but the idea is that today between each of these invocations we have this weird clear_if_dirty thing whereas after this patch we'll only need clear_if_dirty between rustc0 and std1 (which is much easier since it's just checking the timestamp of the rustc binary rather than a bunch of sysroot things).

Yes, that's the plan once this patch is in a beta compiler. We'll still need clear_if_dirty checks between stages (for rustc and codegen-backends, I believe). rustc clear-if-dirty is technically already done by Cargo if we switch to using RUSTC_WRAPPER I believe instead of RUSTC for the environment variable then we'll get free tracking for the rustc binary (that's an orthogonal change to this one and can be done in parallel/later).

The purpose of this patch is to tell Cargo more about the dep-info of inputs. Cargo currently today uses dep-info to learn about source code to recompile when something changes, but naturally if any of the binary dependencies (like libstd) change the crate also needs to be recompiled. That's the purpose of this, right?

Yes, exactly -- I think this is probably the 95% solution or so, as it doesn't track native dependencies (C libraries that we link to, primarily), but I'm not sure to what extent we know about those when we emit dep-info. I can investigate adding them but since they're not really the primary use case I think it can probably be delayed for a later patch.

Cargo tests all pass with this change; some of the logic in Cargo may no longer be necessary (i.e., we are possibly double-fingerprinting rlibs, etc. and can skip that work now), but that's more of a performance question, not a correctness question. I'm also not sure whether Cargo fingerprints rlibs at all.

[alexcrichton]

@bors: r+

Nice! That all sounds great to me!

I agree that Cargo is probably double fingerprinting things or checking too much, but that's ok until it causes issues :)

FWIW I'd love to get rid of src/bootstrap/bin/rustc.rs, or at least vastly reducing its scope. Back when rustbuild was first added even RUSTFLAGS didn't exist, so 90% of the functionality in that script can probably be replaced if not entirely removing the script. (aka I like the idea of switching to RUSTC_WRAPPER)

[bors]

📌 Commit da73a36 has been approved by alexcrichton

[ehuss]

Cargo tests all pass with this change

That's strange. I'm getting a few errors.

plugins::plugin_to_the_max

---- plugins::plugin_to_the_max stdout ----
running `/Users/eric/Proj/rust/cargo/target/debug/cargo build`
running `/Users/eric/Proj/rust/cargo/target/debug/cargo doc`
thread 'plugins::plugin_to_the_max' panicked at '
Expected: execs
    but: exited with exit code: 101
--- stdout
1
--- stderr

warning: path /Users/eric/Proj/rust/cargo/target/cit/t0/foo/src/foo_lib.rs was erroneously implicitly accepted for library foo_lib,

please rename the file to src/lib.rs or set lib.path in Cargo.toml

Documenting baz v0.0.1 (/Users/eric/Proj/rust/cargo/target/cit/t0/baz)

Checking baz v0.0.1 (/Users/eric/Proj/rust/cargo/target/cit/t0/baz)

Checking foo v0.0.1 (/Users/eric/Proj/rust/cargo/target/cit/t0/foo)

Documenting bar v0.0.1 (/Users/eric/Proj/rust/cargo/target/cit/t0/bar)

thread 'rustc' panicked at 'assertion failed: (left == right)

left: 11,

right: 0', src/librustc_metadata/decoder.rs:296:9

stack backtrace:

0: std::sys_common::backtrace::print

1: std::panicking::default_hook::{{closure}}

2: std::panicking::default_hook

3: std::panicking::rust_panic_with_hook

4: std::panicking::continue_panic_fmt

5: std::panicking::begin_panic_fmt

6: <rustc_metadata::decoder::DecodeContext as serialize::serialize::SpecializedDecoder<syntax_pos::span_encoding::Span>>::specialized_decode

7: serialize::serialize::Decoder::read_struct

8: serialize::serialize::Decoder::read_enum

9: serialize::serialize::Decoder::read_seq

10: <syntax::tokenstream::TokenStream as serialize::serialize::Decodable>::decode

11: serialize::serialize::Decoder::read_enum

12: serialize::serialize::Decoder::read_seq

13: <syntax::tokenstream::TokenStream as serialize::serialize::Decodable>::decode

14: serialize::serialize::Decoder::read_struct

15: <core::iter::adapters::Map<I,F> as core::iter::traits::iterator::Iterator>::fold

16: <alloc::vec::Vec as alloc::vec::SpecExtend<T,I>>::from_iter

17: rustc_metadata::decoder::::get_item_attrs

18: rustc_metadata::cstore_impl::provide_extern::item_attrs

19: rustc::ty::query::::compute

20: rustc::dep_graph::graph::DepGraph::with_task_impl

21: rustc::ty::query::plumbing::::get_query

22: rustc::ty::::get_attrs

23: <rustc::hir::def_id::CrateNum as rustdoc::clean::Cleanrustdoc::clean::ExternalCrate>::clean

24: <rustdoc::visit_ast::RustdocVisitor as rustdoc::clean::Cleanrustdoc::clean::Crate>::clean

25: rustc::ty::context::tls::enter_global

26: rustc_interface::passes::BoxedGlobalCtxt::access::{{closure}}

27: rustc_interface::passes::create_global_ctxt::{{closure}}

28: rustc_interface::passes::BoxedGlobalCtxt::enter

29: rustc_interface::interface::run_compiler_in_existing_thread_pool

30: rustdoc::core::run_core

31: <std::panic::AssertUnwindSafe as core::ops::function::FnOnce<()>>::call_once

32: std::panicking::try::do_call

33: __rust_maybe_catch_panic

34: std::panicking::try

35: rustdoc::main_options

36: std::thread::local::LocalKey::with

37: scoped_tls::ScopedKey::set

38: syntax::with_globals

note: Some details are omitted, run with RUST_BACKTRACE=full for a verbose backtrace.
error: internal compiler error: unexpected panic
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
note: rustc 1.37.0-dev running on x86_64-apple-darwin
error: Could not document bar.
Caused by:

process didn't exit successfully: rustdoc --crate-name bar /Users/eric/Proj/rust/cargo/target/cit/t0/bar/src/lib.rs --color never -o /Users/eric/Proj/rust/cargo/target/cit/t0/foo/target/doc -L dependency=/Users/eric/Proj/rust/cargo/target/cit/t0/foo/target/debug/deps --extern baz=/Users/eric/Proj/rust/cargo/target/cit/t0/foo/target/debug/deps/libbaz-c1b180d05b7252a7.rmeta (exit code: 1)

', tests/testsuite/support/mod.rs:842:13

freshness::rename_with_path_deps

---- freshness::rename_with_path_deps stdout ----
running `/Users/eric/Proj/rust/cargo/target/debug/cargo build`
running `/Users/eric/Proj/rust/cargo/target/debug/cargo build`
thread 'freshness::rename_with_path_deps' panicked at '
Expected: execs
    but: differences:
  0 - |[FINISHED] [..]|
    + |   Compiling a v0.5.0 (/Users/eric/Proj/rust/cargo/target/cit/t695/foo2/a)|
1 -

+ |   Compiling foo v0.5.0 (/Users/eric/Proj/rust/cargo/target/cit/t695/foo2)|
2 -

+ |    Finished dev [unoptimized + debuginfo] target(s) in 0.43s|

Maybe it's because I'm running on macos? I can dig in a little later.

[alexcrichton]

@bors: r-

[Mark-Simulacrum]

I cannot reproduce either failure on either macOS or Linux so I think maybe CI is our best bet at this point. As a starting point, let's try to get official builds on at least Linux: @bors try

[bors]

⌛ Trying commit da73a36 with merge a474739...

[ehuss]

The rustdoc error went away after I did x.py clean. However, rename_with_path_deps still fails for me on both macos and linux.

The issue is that the dep-info file contains absolute paths like /Users/eric/Proj/rust/cargo/target/cit/t0/foo2/target/debug/deps/libb-3c49bcf9d5165638.rlib. The test is verifying that renaming a project stays fresh, but with absolute paths in the dep-info file, the .rlib files don't exist at the old path.

FYI, I don't think using PATH to rustc works in cargo's testsuite if you run cargo from a rustup wrapper. The rustup wrapper prepends ~/.cargo/bin to PATH. You either need to do it in the rust repo (./x.py test src/tools/cargo) or set up a custom rustup toolchain that points to the correct binaries (similar to rustup toolchain link).

EDIT: Or don't use rustup, as in: PATH=/path/to/rustc/build/target/stage2/bin:$PATH target/debug/cargo test

[eddyb]

Just to double-check: after this gets into beta, we can fix #54712 relatively easily, right?

[Mark-Simulacrum]

Okay, I'm able to reproduce the Cargo test failure now -- I'm not sure how easily we can fix that. Maybe I can remove the behavior of including non-sysroot dependencies (or anything passed via --extern crate=path, perhaps?) but that seems non-ideal.

I'm personally not sure that such a test is critical -- it appears to have been added by rust-lang/cargo@e9428cb. I feel like the case of moving the directory is largely uncommon so regressing on it may not be that bad? @alexcrichton, what do you think?

Just to double-check: after this gets into beta, we can fix #54712 relatively easily, right?

I think it'll be fixed trivially, yes, but there may be unexpected roadblocks down the line, I'm not sure.

[bors]

☀️ Try build successful - checks-travis
Build commit: a474739

[ehuss]

I was hoping this would fix #59105 (I run into it a lot), but cargo does not parse the dep-info file for registry dependencies. 😢

[Mark-Simulacrum]

Oh, hm, that's rather a blocker for this being useful for rustbuild. Can we remove that limitation? Presumably it's for performance reasons?

[alexcrichton]

Hm so we should handle absolute paths in dep files since Cargo's own internal format stores everything relative to the project root (or something like that, I remember something along those lines). I do think this is a feature we need to preserve in Cargo, it's been requested for CI systems historically where caches are persisted but the build directory changes over time.

We don't track dep-info files for registry deps because dep info tracking is historically only giving us source information, and there's no need to track source information for registry deps because cargo assumes it can't change. If we learn about sysroot dependencies as well though then we could probably go ahead and parse it, it's not exactly a slow part of the build!

[Mark-Simulacrum]

For reference this is the new cargo.d when building with this PR applied; the new content starts at line 142.

I'm investigating why we're not properly handling paths to dependencies in Cargo and will likely file a PR with necessary changes soon.

If we learn about sysroot dependencies as well though then we could probably go ahead and parse it, it's not exactly a slow part of the build!

I'll include a patch which enables parsing and tracking dep-files for dependencies as well (or file a separate PR for it, depending on complexity).

[Mark-Simulacrum]

Marking this as S-blocked on rust-lang/cargo#7030 and rust-lang/cargo#7034.

[ehuss]

@Mark-Simulacrum I've been testing this on Windows and am having problems with it creating a mixture of dos paths and device paths. That is, I see things like:

D:\path\to\proj\target\debug\deps\foo-hash.d: …

\\?\C:\path\to\rustc\rustlib\x86_64-pc-windows-msvc\lib\std-hash.dll:
\\?\C:\path\to\proj\target\debug\deps\libfoo-hash.rlib:

Would it be possible to get all one style? It makes it difficult to check for the prefix in Cargo. Cargo typically uses dos-style paths, since that's the default for most rust things. Or if you have ideas how to easily handle both kinds, that would be great.

[Mark-Simulacrum]

I would expect us to have all once style... I'm guessing some part of the compiler isn't properly handling these paths and/or is canonicalizing them unlike the others. It might be rather hard for me to debug since I don't have easy access to windows... but I do think it should be possible to make the path types uniform. I'll look at what I can do to debug Windows in a bit.

[mati865]

@ehuss if you put paths through std::fs::canonicalize they will become UNC. AFAIK there is no method to create DOS path from UNC.

[eddyb]

@ehuss You should probably canonicalize both sides before checking for prefix, but I would try to avoid giving the user UNC paths, because of the arguable worse UX.
So you might need to store both sometimes (if you don't want to canonicalize often)?
I wonder if @retep998 or @alexcrichton have any strong opinions here.

[alexcrichton]

The compiler uses fs::canonicalize internally to find paths to binary dependencies (rlibs/dlls/etc) to handle symlinks between files and automatically deduplicate based on that. The compiler also prints out output files relative to what the CLI flags are.

I think what's happening is Cargo is executing rustc as:

rustc --out-dir D:/path/to/proj ./foo.rs 

which emits:

D:/path/to/proj/foo.d: ./foo.rs

\\?\C:\...: ...

One thing Cargo should be sure to do is handle whatever path comes from users and work regardless of whether it's UNC or otherwise absolute. In this case, however, I think all paths are synthesized by Cargo and passed down to rustc.

---

With that background, I think there's perhaps two ways to fix this issue @ehuss raised. One is that rustc could be less aggressive about printing UNC paths. The path to the standard library, for example, seems reasonable to be UNC. The path to libfoo-hash.rlib, however, was either -L with Cargo or --extern with Cargo, neither of which is a UNC path today. If rustc preserved the same style of path that was passed to it going out to the dep-info file, I think it would solve the issue here. In general rustc tries pretty hard to preserve the input paths (e.g. through debuginfo or through foo-hash.d which is relative to the exact value of --out-dir which isn't UNC), so it seems reasonable that rustc could and maybe should do the same for dlls/rlibs if they're getting emited to the filesystem now. (note that this is an issue with this PR since I think previously those internally canonicalized paths never made their way back out of the compiler).

Alternatively Cargo, since it's synthesizing all the arguments here, could more aggressively canoicalize everything going into rustc. For example it could canonicalize the value of --out-dir which would canonicalize the foo-hash.d path.

I would suspect in terms of man-hours its easier to change Cargo instead of rustc, but this is arguably not a great feature of rustc that it's canonicalizing paths in its output where it doesn't do that anywhere else.

[ehuss]

It might be rather hard for me to debug since I don't have easy access to windows

I often use the free VMs from microsoft when I'm not near a windows machine.

I added a hack to my Cargo PR to deal with the extended-length paths. I don't have much of an opinion on it, but I'd be curious if there are any other consumers of the .d files besides Cargo. That might influence your decision on which style to expose, if other tools have problems with mixing them.

[Mark-Simulacrum]

Yes, I'm aware of the free VMs (though I use these). I also don't know Windows at all so getting started is usually quite a hassle -- if it becomes necessary, I can invest the time it'll take me to get Windows working, but I'd rather not, if that makes sense :)

I think I agree with Alex here that fixing/doing the right thing in Cargo would be easier than modifying rustc. I think we don't guarantee the .d contents too much today so it should be feasible to fix this in rustc itself later down the road.

[alexcrichton]

I think @ehuss actually brings up a good point, can this be tested to make sure it actually works on Windows with make? That's AFAIK the primary other consumer of .d files, and while not quite as prevalent as Cargo there are Windows builds which use makefiles and rely on .d information originating from rustc to hook things up right.

[Mark-Simulacrum]

I spent a couple hours yesterday trying to get a Windows VM working to compile Rust within it, but unfortunately didn't really make any headway. I can try to get just make working and attempt to synthesize a .d file, I suppose, but that seems like a poor test. Maybe someone with easy access to Windows could try it?

[Mark-Simulacrum]

@alexcrichton I've now implemented a -Z flag to gate the new behavior. I think it might be worthwhile to land this now to enable easier testing and such -- e.g. we would have Windows builds at that point. Do you think that's a viable strategy? If not I can try to get a Windows VM going to test out the Makefile issues.

[alexcrichton]

@bors: r+

Sounds good to me!

[bors]

📌 Commit d749b5e has been approved by alexcrichton