-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Description
The rust-src
component that gets installed by Rustup to ~/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust
includes a Cargo.lock
that spans the standard library crates, but does not include a corresponding Cargo.toml
(essentially this Cargo.toml
).
Cargo's -Zbuild-std
feature makes this work anyway by constructing a virtual workspace for the standard library, but this doesn't work for editors that want to provide users with completion and analysis all the way into std
/alloc
/core
. They end up running cargo metadata
directly in the various standard library crates (library/std
for example) (IntelliJ, rust-analyzer). However, since there's no workspace-level Cargo.toml
next to the Cargo.lock
, they do not pick up on the locked versions at all, and instead use whatever the latest versions of the standard library's dependencies are. This in turn means that go-to-definition and similar mechanisms do not go to the actual versions used by the standard library the user is actually using.
In chatting to @Mark-Simulacrum about this, I think the editors are arguably doing the right thing here by running cargo metadata
inside the individual crates (like std), and Cargo is doing the right thing not looking at the Cargo.lock
since there isn't a workspace there. And it feels like the right thing to do is to synthesize a workspace Cargo.toml
for inclusion with rust-src
. But if we explicitly won't (or can't) include a Cargo.toml
in rust-src
's root, we have a couple of options:
- We could maybe special-case Cargo to recognize when it's run on a crate from within the standard library.
cargo metadata
could have something like--with-lockfile Cargo.lock
so that you can run it in arbitrary places on disk while pointing to (for example) a checked out repositoriesCargo.lock
.- We could change Cargo to always include std + deps in everyone's
Cargo.lock
as some kind of special dep kind. - We could create
cargo metadata --include-std
.
All of these feel brittle to one extent or another though, and it's not clear that they are sufficient for what editors may need (cc @matklad), so I think we should push for including Cargo.toml
if we can.
Meta
rustc --version --verbose
:
rustc 1.59.0 (9d1b2106e 2022-02-23)
binary: rustc
commit-hash: 9d1b2106e23b1abd32fce1f17267604a5102f57a
commit-date: 2022-02-23
host: x86_64-unknown-linux-gnu
release: 1.59.0
LLVM version: 13.0.0
Activity
jonhoo commentedon Apr 6, 2022
A simple way to reproduce this is:
jonhoo commentedon Apr 6, 2022
This also seems like a regression from #44076
matklad commentedon Apr 6, 2022
The absolute best for the editors would be if
/library
was a separate workspace with dedicated Cargo.toml, Cargo.lock, and vendored sourced of crates.io deps (such that cargo-metadata works without net).Practically, this would allow us to not worry about "large" compiler codebase and only deal with the small set of library crates. Theoretically, it still feels to me that "compiler" and "library" actually exist in separate dimensions, and using a single Cargo.lock for them seems accidental.
Obviously, that's "perfect is the enemy of the good" kind of thing, just restoring Cargo.toml/.lock would be helpful
jonhoo commentedon Apr 6, 2022
Another thing worth adding to this is that
cargo metadata
only accidentally works inrustlib/src/rust/library/*
because empty versions of crates likerustc-std-workspace-alloc
exist on crates.io. Without the workspaceCargo.toml
to pick up the localpath
, that means the editors end up going to fetch the crates.io versions of these even though the right versions are sitting inrustlib/src/rust/library
.RalfJung commentedon Apr 10, 2022
FWIW, the lock file is included because I needed it to make Miri more reliable, and then just added it. I think a PR to simply add Cargo.toml to the list of shipped files would probably go through in a similar vein.
However, it is not clear to me whether you are saying the original Cargo.toml should be shipped, or whether it should be somehow modified before shipping. The latter will be more tricky, of course -- is it even clear what the "right" way to adjust the toml file is?
RalfJung commentedon Apr 10, 2022
Oh, you actually linked to my old PR that added the Cargo.lock file. 😂 Looks like it originally intended to also add Cargo.toml but that part got bailed out before landing.
This is where I had to remove the Cargo.toml because it caused trouble with distcheck that I didn't have the patience to debug.
FWIW, those crates existing on crates.io is quite deliberate and a key part to their function.
eddyb commentedon Apr 11, 2022
Cross-posting (potentially relevant) #78790 (comment) (note that that PR was reverted):
bjorn3 commentedon Apr 11, 2022
I think we should just split the sysroot into a new workspace and use it as part of bootstrapping instead of generating a new workspace in the dist step. That may also simplify the tidy license check by not requiring to check if a crate is a dependency of the sysroot to apply stricter license checks. Instead it can apply them to the entire contents of
library/Cargo.lock
.jonhoo commentedon Apr 14, 2022
One more observation: this problem is exacerbated by the fact that when an editor runs
cargo metadata
in, say,<rust-src>/library/std
, it will end up generating aCargo.lock
. The same will happen when it's run inalloc
,core
, etc. This means you end up with oneCargo.lock
in every sub-crate, plus one at the root, all of which can contain different versions from one another since all the sub-crate ones contain whatever was the latest version at the timecargo metadata
was run.Rollup merge of rust-lang#97228 - jonhoo:patch-1, r=bjorn3
lf- commentedon Oct 10, 2022
This winds up extra fun under Nix using $RUST_SRC_PATH where the rust sources are literally on a readonly file system, which is the r-a issue here: rust-lang/rust-analyzer#13393
The correct fix for this issue is to add the lockfile and manifest to rust-src, I think, so cargo doesn't try to relock it.
RalfJung commentedon Oct 10, 2022
IMO it's a bug in
cargo metadata
that it doesn't work on a read-only file system (rust-lang/cargo#10096).Veykril commentedon Aug 5, 2024
I believe #128534 essentially fixes this?
bjorn3 commentedon Aug 5, 2024
It does. I would have linked this issue from that PR if I had remembered this issue existed.