Change build-std to use --sysroot #7421

ehuss · 2019-09-24T02:25:27Z

This transitions build-std to use --sysroot instead of --extern. This is necessary because existing crates have a certain expectation of how standard library crates are exposed. It was intended that explicit dependencies in Cargo.toml would solve this problem, but I didn't really consider this would be a backwards-incompatible change, so using --sysroot is probably the best way to go, even though it's not ideal.

Closes rust-lang/wg-cargo-std-aware#31
Closes rust-lang/wg-cargo-std-aware#40

* Minimize the sysroot crates in play * Don't use build scripts to inject args * Use `RUSTC_WRAPPER` to dynamically switch `--sysroot` depending on whether we're building sysroot crates or not. * Minimize dependency graph in sysroot, only have each crate depend on a dummy crates.io crate for testing and otherwise don't depend on anything to load the desired sysroot crate directly.

rust-highfive · 2019-09-24T02:25:37Z

r? @alexcrichton

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

ehuss · 2019-09-24T02:26:45Z

Thanks for working on the tests, Alex! That was the hardest part.

A few notes about this PR:

This line uses the wrong sysroot (it uses the one from rustc). Should I just skip that line if build-std is enabled? I don't 100% understand the purpose of it in the first place (is it only for building rustc?). target_dylib_path also doesn't handle .json targets correctly.
- EDIT: The only test that relies on it is cross_test_dylib. Since build-std doesn't have dylibs, maybe it should be skipped?
Build-plan is not really supported properly. It doesn't include the steps for copying the artifacts to the sysroot. Maybe it should just include those paths in the links list? It's kinda hard to do since it isn't using the built-in artifact uplifting mechanism. I'm not really sure about the future of build-plan in general.
This uses link_or_copy, but I think there may be a race condition in the "copy" case. Copies are used on filesystems that don't support hardlinks (like some network filesystems). My concern is for rmeta/rlib pipelining. It will copy over the rmeta file, then pipelining will launch a new rustc. However, I'm concerned that the rlib file may be copied over while the new rustc is running. There is a tiny window where it may have an partially filled rlib, and I'm not sure how rustc will handle that.
This does not handle CRT files or bundled binaries like lld. I'd like to punt on that and figure out what to do later.

alexcrichton · 2019-09-24T15:46:21Z

Since build-std doesn't have dylibs, maybe it should be skipped?

Agreed, should be fine to skip that line for -Zbuild-std

Build-plan is not really supported properly.

Good point! I think we should probably just give a first class error very early on saying that you can't combine -Zbuild-std and -Zbuild-plan

There is a tiny window where it may have an partially filled rlib, and I'm not sure how rustc will handle that.

This is already somewhat true for pipelining I think because although we pass --extern to specifically just the *.rmeta rustc still scans the directory for other rlibs and may read half-written rlibs. In general though rustc has had this problem quite a few times in the past, and at this point it's pretty resilient to errors which is to say it ignores all of them and only actually reports an error if it for sure can't make progress.

This does not handle CRT files or bundled binaries like lld.

Agreed. Let's open an issue in wg-std-aware-cargo after this lands?

alexcrichton · 2019-09-24T15:48:44Z

src/cargo/core/compiler/layout.rs

+        // Compute the sysroot path for the build-std feature.
+        let build_std = ws.config().cli_unstable().build_std.as_ref();
+        let (sysroot, sysroot_libdir) = if let Some(tp) = build_std.and(triple_path) {
+            let sysroot = dest.join(".sysroot");


I'd have a mild preference for not hiding this directory by default, but I'm curious why you added the leading period?

The issue is that named profiles will be taking over this namespace (each named profile has a directory with its name). I'd like to avoid potential conflicts. An alternative would be to reserve "sysroot" and not allow it as a profile name (which won't be much of an option once named profiles stabilizes). Or we could come up with some other structure or naming convention. Any preferences?

Ah ok sounds good to me! Can you add that to a comment indicating why there's a leading dot?

alexcrichton · 2019-09-24T15:49:27Z

src/cargo/core/compiler/job_queue.rs

+        if unit.is_std && unit.kind == super::Kind::Target && !cx.bcx.build_config.build_plan {
+            let rmeta = artifact == Artifact::Metadata;
+            standard_lib::add_sysroot_artifact(cx, unit, rmeta)?;
+        }


Would it be possible to sink this work into the unit itself? Ideally we wouldn't be doing much extra stuff here in job_queue.rs and it keeps all the major work on each thread cargo spawns for work.

I'm not sure how that would work, particularly with pipelining. The rmeta json artifact notification goes through a generic route that most conveniently ends up here in JobQueue. I could disable pipelining and move it to the Work closure instead?

Ah I see what you mean. Thinking more on this though, I was actually assuming that we'd just change --out-dir for is_std crates. It looks like otherwise they're gonna show up both in the libdir as well as the main deps folder, right? If we switched --out-dir I think it'd solve this issue naturally?

Er wait no we already talked about it and ruled it out because we want to clean out the sysroot on each build.

In any case this poses an interesting scenario where the dependencies of std can be loaded from either the sysroot or -Ldependency=.../deps, they're actually all the same. I'm asking this from the point of view of concurrency which I think this issue is trying to solve (making sure we do this before the next compilation starts), but maybe that's not actually necessary?

Ok I want to try to work this through here in a coment...

So for std->std dependencies, as well as non-std->non-std dependencies, those all use --extern I believe. If that's the case then I think that nothing is necessary here in terms of sequencing things right because everything is loaded from deps

The only case I think this cares about is non-std -> std dependencies, since they're loaded through the sysroot. Specifically you're worried here that we will kick off compilations that depend on core, but nothing may actually be in the sysroot yet, so the compiler won't be able to find anything.

Does that sound right? If so...

So presumably this would otherwise go somewhere around here (transitively), and you're right that's a "pretty generic path" which isn't a great spot to put it.

Ok if you agree with what I've said above, let's leave this here, but add a comment perhaps saying that we may want to figure out how to offload it to worker threads eventually?

That would be cleaner. For some reason I was thinking that was going to be harder (since the output directory is somewhat global), but it probably won't be too hard. I'll give it a shot.

EDIT: Oh, and I think I was also concerned about not atomically adding files while concurrent rustc's are running, but you mentioned that it shouldn't be too much of a problem.

we want to clean out the sysroot on each build

Oh, yea, that's the actual reason. Sorry, I'm mis-remembering things.

Ok if you agree with what I've said above, let's leave this here, but add a comment perhaps saying that we may want to figure out how to offload it to worker threads eventually?

That makes sense. I'll add a comment.

I was previously trying to think of a way that OutputFile::hard_link (or something like it) could be used, but it still runs into the problem with rmeta files needing to be available, and linking isn't done until after compilation finishes.

alexcrichton · 2019-09-24T16:08:53Z

src/cargo/core/compiler/layout.rs

+        let build_std = ws.config().cli_unstable().build_std.as_ref();
+        let (sysroot, sysroot_libdir) = if let Some(tp) = build_std.and(triple_path) {
+            let sysroot = dest.join(".sysroot");
+            let sysroot_libdir = sysroot.join("lib").join("rustlib").join(tp).join("lib");


So technically this isn't going to work on all compilers digging in a bit. The sysroot finding function in rustc takes into account CFG_LIBDIR_RELATIVE which is set by taking the difference between --libdir and --prefix when building rustc itself.

Now that's pretty esoteric though and only really relevant for Linux distributions, but it may be worth keeping in mind that this may run afoul of that at some point.

Oh… I'll try to investigate. There doesn't seem to be a way to discover the actual path.

alexcrichton · 2019-09-24T16:09:57Z

tests/testsuite/standard_lib.rs

+                    let is_sysroot_crate = args.iter()
+                        .any(|a| a.starts_with("rustc_std_workspace") || crates.iter().any(|b| a == b));
+
+                    if is_sysroot_crate {


This can actually be simplified now I think to testing for RUSTC_BOOTSTRAP perhaps? (maybe with some extra checks for --target being passed as well)

ehuss · 2019-09-24T21:04:28Z

I think we should probably just give a first class error very early on saying that you can't combine -Zbuild-std and -Zbuild-plan

I decided to just add a warning. I sometimes use it for testing, and I don't think anyone's going to use them together anytime soon.

Let's open an issue in wg-std-aware-cargo after this lands?

Yea, I'm planning to open a few issues after this is done.

alexcrichton · 2019-09-25T15:57:58Z

@bors: r+

bors · 2019-09-25T15:57:59Z

📌 Commit 9d86d86 has been approved by alexcrichton

bors · 2019-09-25T15:58:08Z

⌛ Testing commit 9d86d86 with merge c59e1f4...

Change build-std to use --sysroot This transitions build-std to use `--sysroot` instead of `--extern`. This is necessary because existing crates have a certain expectation of how standard library crates are exposed. It was intended that explicit dependencies in `Cargo.toml` would solve this problem, but I didn't really consider this would be a backwards-incompatible change, so using `--sysroot` is probably the best way to go, even though it's not ideal. Closes rust-lang/wg-cargo-std-aware#31 Closes rust-lang/wg-cargo-std-aware#40

bors · 2019-09-25T16:32:35Z

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing c59e1f4 to master...

4 commits in aa6b7e01abce30091cc594cb23a15c46cead6e24..ab6fa8908c9b6c8f7e2249231c395eebfbc49891 2019-09-24 17:19:12 +0000 to 2019-09-25 17:00:56 +0000 - Fix macOS collection of CPU data (rust-lang/cargo#7429) - Don't ever capture CPU state if timings are disabled (rust-lang/cargo#7428) - Change build-std to use --sysroot (rust-lang/cargo#7421) - set -Zmtime_on_use from config or ENV (rust-lang/cargo#7411)

Update cargo 4 commits in aa6b7e01abce30091cc594cb23a15c46cead6e24..ab6fa8908c9b6c8f7e2249231c395eebfbc49891 2019-09-24 17:19:12 +0000 to 2019-09-25 17:00:56 +0000 - Fix macOS collection of CPU data (rust-lang/cargo#7429) - Don't ever capture CPU state if timings are disabled (rust-lang/cargo#7428) - Change build-std to use --sysroot (rust-lang/cargo#7421) - set -Zmtime_on_use from config or ENV (rust-lang/cargo#7411)

Ericson2314 · 2019-09-26T01:34:59Z

Do we have a ticket tracking not doing this?

Switch build-std to use --extern Switch `build-std` to use `--extern` flags instead of `--sysroot`. This is mostly a revert of #7421. It uses the new extern flag options introduced in rust-lang/rust#67074. It avoids modifying the extern prelude which was the source of the problem of rust-lang/wg-cargo-std-aware#40. Closes rust-lang/wg-cargo-std-aware#49 Reopens rust-lang/wg-cargo-std-aware#31

ehuss and others added 3 commits September 23, 2019 18:53

Change build-std to use --sysroot.

e9dd306

Some more --sysroot tests.

53a3db0

rust-highfive assigned alexcrichton Sep 24, 2019

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

alexcrichton reviewed Sep 24, 2019

View reviewed changes

ehuss added 3 commits September 24, 2019 12:45

Simplify mock rustc wrapper.

d0ab04a

Don't add sysroot lib path to dylib path for build-std.

581da42

Add warning when mixing -Zbuild-std with --build-plan.

3afb5d7

ehuss mentioned this pull request Sep 24, 2019

Refactor Kind to carry target name in Target #7425

Merged

ehuss added 2 commits September 24, 2019 14:49

Add some comments about Layout naming and .sysroot.

abe92bc

Add comment for unusual placement of add_sysroot_artifact.

9d86d86

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 Sep 25, 2019

bors merged commit 9d86d86 into rust-lang:master Sep 25, 2019

bors mentioned this pull request Sep 25, 2019

Support for named profiles (RFC 2678) #6989

Merged

This was referenced Sep 25, 2019

Possibly add a way to disable sysroot on rustc rust-lang/wg-cargo-std-aware#31

Open

Using --extern is apparently not equivalent to --sysroot rust-lang/wg-cargo-std-aware#40

Closed

alexcrichton mentioned this pull request Sep 25, 2019

Update cargo rust-lang/rust#64774

Merged

ehuss mentioned this pull request Sep 26, 2019

Support --build-plan rust-lang/wg-cargo-std-aware#45

Open

ehuss mentioned this pull request Dec 12, 2019

Switch build-std to use --extern #7699

Merged

ehuss added this to the 1.40.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change build-std to use --sysroot #7421

Change build-std to use --sysroot #7421

ehuss commented Sep 24, 2019

rust-highfive commented Sep 24, 2019

ehuss commented Sep 24, 2019 •

edited

Loading

alexcrichton commented Sep 24, 2019

alexcrichton Sep 24, 2019

ehuss Sep 24, 2019

alexcrichton Sep 24, 2019

alexcrichton Sep 24, 2019

ehuss Sep 24, 2019

alexcrichton Sep 24, 2019

alexcrichton Sep 24, 2019

ehuss Sep 24, 2019 •

edited

Loading

ehuss Sep 24, 2019

alexcrichton Sep 24, 2019

ehuss Sep 24, 2019

alexcrichton Sep 24, 2019

ehuss commented Sep 24, 2019

alexcrichton commented Sep 25, 2019

bors commented Sep 25, 2019

bors commented Sep 25, 2019

bors commented Sep 25, 2019

Ericson2314 commented Sep 26, 2019 •

edited

Loading

Change build-std to use --sysroot #7421

Change build-std to use --sysroot #7421

Conversation

ehuss commented Sep 24, 2019

rust-highfive commented Sep 24, 2019

ehuss commented Sep 24, 2019 • edited Loading

alexcrichton commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Sep 24, 2019

alexcrichton commented Sep 25, 2019

bors commented Sep 25, 2019

bors commented Sep 25, 2019

bors commented Sep 25, 2019

Ericson2314 commented Sep 26, 2019 • edited Loading

ehuss commented Sep 24, 2019 •

edited

Loading

ehuss Sep 24, 2019 •

edited

Loading

Ericson2314 commented Sep 26, 2019 •

edited

Loading