Use a separate workspace for the library crates

[bjorn3]

Suggested by @Mark-Simulacrum in #76446 (comment)
Discussion: https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Separating.20rustc.20and.20library.20workspaces/near/209466743

This likely updated some dependencies for the sysroot, as the Cargo.lock has been generated from scratch.

[rust-highfive]

r? @Mark-Simulacrum

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

[jyn514]

I can confirm that running cargo check in library/ 'just works' ™️ now :)

[jyn514]

So you can mark this as closing #76446.

[jyn514]

 tidy error: /checkout/library/Cargo.toml doesn't have `edition = "2018"` on a separate line
thread 'main' panicked at 'could not find package `std` in package list', src/tools/tidy/src/deps.rs:429:36
could not find exception package `fortanix-sgx-abi`
Remove from EXCEPTIONS list if it is no longer used.

[jyn514]

I can confirm that running cargo check in library/ 'just works' ™️ now :)

Oh ... it still breaks in library/std though.

error[E0119]: conflicting implementations of trait `ffi::c_str::CString::new::SpecIntoVec` for type `&str`:
   --> std/src/ffi/c_str.rs:392:9
    |
379 |         impl<T: Into<Vec<u8>>> SpecIntoVec for T {
    |         ---------------------------------------- first implementation here
...
392 |         impl SpecIntoVec for &'_ str {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `&str`
    |
    = note: upstream crates may add a new impl of trait `core::convert::Into<alloc_crate::vec::Vec<u8>>` for type `&str` in future versions

error: aborting due to 119 previous errors; 93 warnings emitted

Do you know why it's not using the same Cargo.toml in both? cargo check --manifest-path ../Cargo.toml works fine.

[cuviper]

Do vendored builds still work? I would think that'd need changes too. That's important for dist-src tarballs, for distros to do offline builds.

[cuviper]

Maybe it just needs cargo vendor --sync library/Cargo.toml now.

rust/src/bootstrap/dist.rs

Lines 1115 to 1124 in b4bdc07

	// If we're building from git sources, we need to vendor a complete distribution.
	if builder.rust_info.is_git() {
	// Vendor all Cargo dependencies
	let mut cmd = Command::new(&builder.initial_cargo);
	cmd.arg("vendor")
	.arg("--sync")
	.arg(builder.src.join("./src/tools/rust-analyzer/Cargo.toml"))
	.current_dir(&plain_dst_src);
	builder.run(&mut cmd);
	}

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[bjorn3]

I fixed tidy and vendoring should now also work. (not tested)

[Mark-Simulacrum]

It would be good to get a diff of the updated dependencies, since std/test dependencies should undergo somewhat careful review on updates, as they're getting linked into every Rust program out there.

I think you can probably do that by diffing cargo metadata output? I'm not sure if that's the best way though. I am hesitant to approve this without such a diff, though.

Alternatively we can check what would happen in the current master workspace on a cargo update and see if that touches anything in the library/ dependency tree.

[bjorn3]

The updated dependencies are:

-"cc 1.0.58"
+"cc 1.0.59"
-"libc 0.2.74"
+"libc 0.2.76"
-"ppv-lite86 0.2.8"
+"ppv-lite86 0.2.9"

(I used cargo metadata | jq '.packages[] | .name + " " + .version' | sort to get a list of all dependencies and then diffed the result for the old Cargo.lock and the new library/Cargo.lock)

[jyn514]

It might be better to land this without the updates and land the updates separately?

cargo update -p cc --precise 1.0.58
cargo update -p libc --precise 0.2.74
cargo update -p ppv-lite86 --precise 0.2.8

[Mark-Simulacrum]

Those all look benign to me. We switch to a single workspace in #37016, but I think that was done as part of the (more important) move to cargo metadata vs manual inspection of things, so I don't think it's a problem to revert that here.

This needs to update dist.rs to also copy the library/Cargo.lock I think, or at least some inspection of whether we're copying the right Cargo.lock is needed (maybe we're copying an extra one). It should be fairly quick to build that tarball locally with x.py dist src, and it should be possible to cargo check within that tarball.

Please also squash commits into one.

[ehuss]

Can you add a clear description to the PR comment why this change is being made? This seems like a very major change that is likely to have a wide impact (and potentially break things). I also feel like this is going in the opposite direction that workspaces should go, so I'm not sure I fully understand why it is being done.

Some things that will be more difficult with this:

• Vendoring: Not everything uses the python script to vendor.
• Updating profiles: there are now multiple places to update.
• Updating dependencies: there are now multiple places to update things like cc.
• The way tidy works is now more complicated and harder to understand.
• Cross-dependencies to the standard library are impossible, should we want to add them (which is something I was looking at).

As Mark mentioned, this will break the rust-src component.

[bjorn3]

Quoting the comment mentioned in the PR description:

I suspect having two workspaces in rust-lang/rust might be a good idea long-term regardless to lessen various other pains, so I wouldn't mind having a dedicated library workspace. That'll take some work, though.

[bjorn3]

This needs to update dist.rs to also copy the library/Cargo.lock I think, or at least some inspection of whether we're copying the right Cargo.lock is needed (maybe we're copying an extra one).

The whole library dir is copied except for library/backtrace/crates. In addition the now unnecessary ./Cargo.lock is copied.

[ehuss]

That doesn't say why, other than Mark thinks it is a good idea. What are the technical reasons for the change? What does it enable or fix?

[bjorn3]

I also feel like this is going in the opposite direction that workspaces should go, so I'm not sure I fully understand why it is being done.

Workspaces are to share compiled dependencies between crates. Bootstrap actually tried to prevents dependencies included through libstd and through rustc from getting the same filename in order to prevent conflicts due to it being able to include a crate using the sysroot and as a regular dependency.

[bjorn3]

The way tidy works is now more complicated and harder to understand.

This is mostly because I didn't quite understand all the logic. Tidy already tries to handle runtime and rustc dependencies differently with regard to for example allowed licenses. Having a separate Cargo.lock for both should make it possible to simplify this logic.

Cross-dependencies to the standard library are impossible, should we want to add them (which is something I was looking at).

What would be the use? Also you can still depend on the standard library. There are not even additional recompilations, because as I mentioned earlier bootstrap already causes recompilation of crates used by both the standard library and the compiler.

[Mark-Simulacrum]

@ehuss I agree with you that we would ideally not do this -- it would be better to have a single workspace. AFAICT, though, Cargo's current understanding of what to build when in workspaces doesn't really make that easy. I would be fine with us patching that instead, but it means changes upstream in Cargo.

The primary thing this seeks to enable is that cd library && cargo test would start being feasible to support; cargo check would already work after this PR. We can do that with a shared workspace and manual --manifest-path library/test/Cargo.toml, but that feels suboptimal. Plus, with this you can do things like cargo check --all --all-targets in the library workspace and that works without needing to enumerate the crates with -p.

I was also hopeful that this change is actually nice for things like -Zbuild-std because presumably that needs to manufacture a subset of the current Cargo.lock? If you feel like there's a better solution to the "different default manifests per directory" problem than multiple workspaces, then I'm all ears :)

Maybe we can do most of the above with something like sub-workspaces, but my current impression is that sort of solution requires lots of design work before we can get it in rust-lang/rust.

[ehuss]

What would be the use?

The intent is to possibly make the dependencies of rustc to std more explicit, simplifying the build for rustc. The intent is to also get rid of rustc-dep-of-std. This is very far off, so not a big concern, but this would need to be undone to make that happen.

cargo check would already work after this PR.

Why does cargo check not work now? Does this mean cargo check only for the standard library? If so, how is cd library && cargo check much simpler than cargo check -p test (or cd library/test && cargo check)?

If you feel like there's a better solution to the "different default manifests per directory" problem than multiple workspaces, then I'm all ears :)

I haven't really thought of this use case because nobody has ever mentioned it. We've been discussing nested workspaces, and this is good feedback for that. But I'm still unclear what the goals are here. Is it specifically, "while in the library/ directory, cargo defaults to all of the standard library crates"? Can you say more about exactly what behavior you want different and why?

I was also hopeful that this change is actually nice for things like -Zbuild-std because presumably that needs to manufacture a subset of the current Cargo.lock?

It won't have an effect assuming the rust-src component is fixed. If it is not fixed, various tools will need to be updated.

but my current impression is that sort of solution requires lots of design work before we can get it in rust-lang/rust.

Probably, we're still collecting use cases for nested workspaces, but there is very little feedback, so it is hard to guess what people would want.

[jyn514]

I haven't really thought of this use case because nobody has ever mentioned it. We've been discussing nested workspaces, and this is good feedback for that. But I'm still unclear what the goals are here. Is it specifically, "while in the library/ directory, cargo defaults to all of the standard library crates"? Can you say more about exactly what behavior you want different and why?

The main use case I want is #76444. Currently, cargo check in library gives 151 errors that are entirely unhelpful when the fix is to use cargo check -p test instead.

[Mark-Simulacrum]

I personally don't care much about this either way, I just feel that this PR is the only fix I'm aware of that makes cargo check for std work without teaching people -p test or similar. I don't think the average user expects to need to pass -p arguments for functionality to work for them, nor are most users familiar with what they do and how to use them. Most Rust projects just work without needing to think about which crate you're building in particular.

I think our best bet at guessing that the user is working on the standard library rather than the compiler (or both), i.e., a mode where we can readily provide cargo check/build/test in theory without bootstrap being involved, is precisely cd library. It is possible that we shouldn't optimize for this "bootstrap avoidance" until we can eliminate bootstrap completely, though, which is either impossible or quite far off.

[jyn514]

It is possible that we shouldn't optimize for this "bootstrap avoidance" until we can eliminate bootstrap completely, though, which is either impossible or quite far off.

To be clear - I think it's fine for cargo check not to work and say 'go through x.py instead'. What I don't like is cargo check looking like you did something wrong (the 151 errors I mentioned earlier). So while having cargo check work is a great goal, I think it's much more important to give some sort of useful error message.

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[Mark-Simulacrum]

I think probably we should close this, and pursue a change in Cargo's workspaces that permits specifying default members for particular subdirectories. I imagine that would not be too hard to implement as default members already exist for workspaces. I think it would resolve the main concern of "cd library && cargo check" not working, right?

[jyn514]

If we could have cd library && cargo check default to cargo check -p library/test I'd be very happy with that, yeah.

[bjorn3]

This might help rust-analyzer with loading the sysroot because it makes both Cargo.toml and Cargo.lock for the library workspace available. Currently Cargo.lock for the only workspace in this repo is available, but the corresponding Cargo.toml is not as most members are missing. This means that cargo metadata will fail.

https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Fwg-rls-2.2E0/topic/Fixing.20sysroot.20loading/near/210671160

[Mark-Simulacrum]

I am personally unwilling to drive this forward; I think while rust-analyzer and other use cases that want to resolve just the std crate graph are interesting, I am not sure this is the right solution. It also doesn't sound like it's entirely a pressing issue on the rust-analyzer side, so I am inclined to wait until it becomes one, given the added complexity in tooling here.

I'm going to close this; thanks @bjorn3 for creating it, but I think I'm unprepared to accept it just now.

[matklad]

We've recently realized that for rust-analyzer it would be slightly more beneficial than previously thought: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Where.20are.20third.20party.20crates.20in.20rustc-src.3F

Currently, rust-analyzer can't deal with external crates from stdlib. The most end-user visible issue is that os-specific modules like std::os::unix are unresolved, because they are implemented using a cfg_if macro.

rust-analyzer ideally needs two things:

• Cargo.toml for the sysroot, so that it can cargo metadata deps instead of hard-coding them
• sources of all third-party crates in rust-src component

We can hack-around this by running cargo metadata for the whole workspace, and pruning non-library parts of the output, but it feels like having a separate workspace for std would make this more direct (which naturally doesn't mean that we should do this just for the sake of rust-analyzer).