Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix no_std tests that load libc from the sysroot when download-rustc is enabled #110229

Merged
merged 1 commit into from
Apr 19, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Apr 12, 2023

There were a series of unfortunate interactions here. Here's an MCVE of the test this fixes (committed as tests/ui/meta/no_std-extern-libc.rs):

#![crate_type = "lib"]
#![no_std]
#![feature(rustc_private)]
extern crate libc;

Before, this would give an error about duplicate versions of libc:

error[E0464]: multiple candidates for `rlib` dependency `libc` found
  --> fake-test-src-base/allocator/no_std-alloc-error-handler-default.rs:15:1
   |
LL | extern crate libc;
   | ^^^^^^^^^^^^^^^^^^
   |
   = note: candidate #1: /home/gh-jyn514/rust/build/aarch64-unknown-linux-gnu/stage2/lib/rustlib/aarch64-unknown-linux-gnu/lib/liblibc-358db1024b7d9957.rlib
   = note: candidate #2: /home/gh-jyn514/rust/build/aarch64-unknown-linux-gnu/stage2/lib/rustlib/aarch64-unknown-linux-gnu/lib/liblibc-ebc478710122a279.rmeta

Both these versions were downloaded from CI, but one came from the rust-std component and one came from rustc-dev:

; tar -tf build/cache/f2d9a3d0771504f1ae776226a5799dcb4408a91a/rust-std-nightly-x86_64-unknown-linux-gnu.tar.xz | grep liblibc
rust-std-nightly-x86_64-unknown-linux-gnu/rust-std-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liblibc-68a2d9e195dd6ed2.rlib
; tar -tf build/cache/f2d9a3d0771504f1ae776226a5799dcb4408a91a/rustc-dev-nightly-x86_64-unknown-linux-gnu.tar.xz | grep liblibc
rustc-dev-nightly-x86_64-unknown-linux-gnu/rustc-dev/lib/rustlib/x86_64-unknown-linux-gnu/lib/liblibc-f226c9fbdd92a0fd.rmeta

The fix was to only copy files from rust-std unless a Step explicitly requests for the rustc-dev components to be available by calling builder.ensure(compile::Rustc).

To avoid having to re-parse the rustc-dev.tar.xz tarball every time, which is quite slow, this adds a new build/host/ci-rustc/.rustc-dev-contents cache file which stores only the names of files we need to copy into the sysroot.

This also allows reverting the hack in #110121; now that we only copy rustc-dev on-demand, we can correctly add the Rustc check artifacts into the sysroot, so that this works correctly even when download-rustc is forced to true and some tool depends on a local change to compiler.


See #108767 (comment) for why no_std is required for the MCVE test to fail; it's complicated and not particularly important.

Fixes #108767.

@jyn514 jyn514 added the A-download-rustc Area: Related to the `rust.download-rustc` build option label Apr 12, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 12, 2023

r? @albertlarsan68

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 12, 2023
@jyn514
Copy link
Member Author

jyn514 commented Apr 12, 2023

Hmm, this breaks x check :/ I can revert #110121, but then check --stage 1 breaks again:

Checking stage1 library artifacts (x86_64-unknown-linux-gnu)
error: process didn't exit successfully: `/home/jyn/src/rust2/build/bootstrap/debug/rustc -vV` (exit status: 127)
--- stderr
/home/jyn/src/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc: error while loading shared libraries: librustc_driver-678dead588f20b29.so: cannot open shared object file: No such file or directory

@jyn514
Copy link
Member Author

jyn514 commented Apr 12, 2023

Ok, pushed a fix.

@bors

This comment was marked as resolved.

@albertlarsan68
Copy link
Member

What happens if there is a step that needs the Rustc sysroot, then another doesn't need one?

What if a first x invocation copies the files, then a second needs them not to be here?

@jyn514
Copy link
Member Author

jyn514 commented Apr 16, 2023

x runs these tests in a pre-determined order that isn't affected by the order of the command line arguments:

; x t ui-fulldeps ui --force-rerun --dry-run
Assembling stage2 compiler
Uplifting library (stage1 -> stage2)
Check compiletest suite=ui mode=ui (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
Uplifting rustc (stage1 -> stage3)
Check compiletest suite=ui-fulldeps mode=ui (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

Kind::Test => describe!(
crate::toolstate::ToolStateCheck,
test::ExpandYamlAnchors,
test::Tidy,
test::TidySelfTest,
test::Ui,
test::RunPassValgrind,
test::MirOpt,
test::Codegen,
test::CodegenUnits,
test::Assembly,
test::Incremental,
test::Debuginfo,
test::UiFullDeps,

So if there is a new Step that requires not having Rustc artifacts in the sysroot, it will need to be added in an order such that it runs before any step that does require Rustc.

I think this is the same as how things normally work even when download-rustc is disabled, but I haven't tested that.

@albertlarsan68
Copy link
Member

I was thinking of the case where you run something that needs full sysroot, then in a second command something that needs the less full sysroot. Is there a second sysroot created, are the unneeded files deleted, or does everything break?

@jyn514
Copy link
Member Author

jyn514 commented Apr 16, 2023

Bootstrap deletes and recreates the sysroot each time it's invoked:

let _ = fs::remove_dir_all(&sysroot);

So artifacts that were copied in a previous run can't affect the next run.

@albertlarsan68
Copy link
Member

r=me with a comment specifying this quirk

@albertlarsan68 albertlarsan68 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2023
There were a series of unfortunate interactions here. Here's an MCVE of the test this fixes (committed as `tests/ui/meta/no_std-extern-libc.rs`):
```rust
 #![crate_type = "lib"]
 #![no_std]
 #![feature(rustc_private)]
extern crate libc;
```

Before, this would give an error about duplicate versions of libc:
```
error[E0464]: multiple candidates for `rlib` dependency `libc` found
  --> fake-test-src-base/allocator/no_std-alloc-error-handler-default.rs:15:1
   |
LL | extern crate libc;
   | ^^^^^^^^^^^^^^^^^^
   |
   = note: candidate #1: /home/gh-jyn514/rust/build/aarch64-unknown-linux-gnu/stage2/lib/rustlib/aarch64-unknown-linux-gnu/lib/liblibc-358db1024b7d9957.rlib
   = note: candidate #2: /home/gh-jyn514/rust/build/aarch64-unknown-linux-gnu/stage2/lib/rustlib/aarch64-unknown-linux-gnu/lib/liblibc-ebc478710122a279.rmeta
```
Both these versions were downloaded from CI, but one came from the `rust-std` component and one came from `rustc-dev`:
```
; tar -tf build/cache/f2d9a3d0771504f1ae776226a5799dcb4408a91a/rust-std-nightly-x86_64-unknown-linux-gnu.tar.xz | grep liblibc
rust-std-nightly-x86_64-unknown-linux-gnu/rust-std-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liblibc-68a2d9e195dd6ed2.rlib
; tar -tf build/cache/f2d9a3d0771504f1ae776226a5799dcb4408a91a/rustc-dev-nightly-x86_64-unknown-linux-gnu.tar.xz | grep liblibc
rustc-dev-nightly-x86_64-unknown-linux-gnu/rustc-dev/lib/rustlib/x86_64-unknown-linux-gnu/lib/liblibc-f226c9fbdd92a0fd.rmeta
```
The fix was to only copy files from `rust-std` unless a Step explicitly requests for the `rustc-dev` components to be available by calling `builder.ensure(compile::Rustc)`.

To avoid having to re-parse the `rustc-dev.tar.xz` tarball every time, which is quite slow, this adds a new `build/host/ci-rustc/.rustc-dev-contents` cache file which stores only the names of files we need to copy into the sysroot.

This also allows reverting the hack in
rust-lang#110121; now that we only copy
rustc-dev on-demand, we can correctly add the `Rustc` check artifacts
into the sysroot, so that this works correctly even when
`download-rustc` is forced to `true`.

---

See rust-lang#108767 (comment) for why `no_std` is required for the MCVE test to fail; it's complicated and not particularly important.

Fixes rust-lang#108767.
@jyn514
Copy link
Member Author

jyn514 commented Apr 18, 2023

@bors r=albertlarsan68 rollup=iffy

@bors
Copy link
Contributor

bors commented Apr 18, 2023

📌 Commit 71f04bd has been approved by albertlarsan68

It is now in the queue for this repository.

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 18, 2023
@bors
Copy link
Contributor

bors commented Apr 19, 2023

⌛ Testing commit 71f04bd with merge da48140...

@bors
Copy link
Contributor

bors commented Apr 19, 2023

☀️ Test successful - checks-actions
Approved by: albertlarsan68
Pushing da48140 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 19, 2023
@bors bors merged commit da48140 into rust-lang:master Apr 19, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 19, 2023
@jyn514 jyn514 deleted the download-rustc-tests branch April 19, 2023 03:04
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (da48140): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.1% [3.1%, 3.1%] 1
Regressions ❌
(secondary)
2.7% [2.7%, 2.8%] 2
Improvements ✅
(primary)
-2.4% [-2.8%, -2.0%] 2
Improvements ✅
(secondary)
-2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) -0.6% [-2.8%, 3.1%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.8% [1.3%, 5.1%] 10
Regressions ❌
(secondary)
2.9% [2.5%, 3.2%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.8% [-4.0%, -3.6%] 2
All ❌✅ (primary) 2.8% [1.3%, 5.1%] 10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-download-rustc Area: Related to the `rust.download-rustc` build option merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests/ui/allocator/no_std-alloc-error-handler* fail when download-rustc is enabled
5 participants