Allow running x.py test --stage 2 src/tools/linkchecker with download-rustc = true

[jyn514]

Previously, the LD_LIBRARY_PATH for the linkchecker looked like
build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib, because the linkchecker depends on the master copy of the standard library. This is true, but doesn't include the library path for the compiler libraries:

/home/joshua/src/rust/rust/build/x86_64-unknown-linux-gnu/stage1-tools-bin/error_index_generator: error while loading shared libraries: libLLVM-12-rust-1.53.0-nightly.so: cannot open shared object file: No such file or directory

That file is in
build/x86_64-unknown-linux-gnu/stage1/lib/libLLVM-12-rust-1.53.0-nightly.so,
which wasn't included in the dynamic path. This adds build/x86_64-unknown-linux-gnu/stage1/lib to the dynamic path for the linkchecker.

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[jyn514]

I think the reason it tries to load LLVM in the first place is because it depends on rustdoc. I wonder if it makes sense to separate out just the markdown module into a separate crate?

[jyn514]

I wonder if it makes sense to separate out just the markdown module into a separate crate?

Oh boo, error_index_generate depends on rustc_span::Edition :(

[GuillaumeGomez]

I tested it locally and it doesn't work (same error).

[GuillaumeGomez]

It works with --stage 2.

[Mark-Simulacrum]

@jyn514 I'm not sure, but my guess is this may have worked already with stage 2 prior to this PR as well? At least, local tests seems to suggest that's true. In that case it's not clear that this is meaningfully changing anything - right?

[jyn514]

Before this PR:

Stage	download-rustc	Link error	Notes
1	true	yes	This behaves exactly the same as stage 0 because bootstrap uses saturating_sub: rust/src/bootstrap/tool.rs Lines 385 to 392 in 8a9fa36 // This uses stage-1 to match the behavior of building rustdoc. // Error-index-generator links with the rustdoc library, so we want to // use the same librustdoc to avoid building rustdoc twice (and to // avoid building the compiler an extra time). This uses // saturating_sub to deal with building with stage 0. (Using stage 0 // isn't recommended, since it will fail if any new error index tests // use new syntax, but it should work otherwise.) let compiler = builder.compiler(builder.top_stage.saturating_sub(1), builder.config.build);
2	true	yes
1	false	yes
2	false	no

After this PR:

Stage	download-rustc	Link error
1	true	yes
2	true	no
1	false	yes
2	false	no

So this doesn't fix all the issues, but it's strictly better than before.

[jyn514]

I think --stage 1 is broken because libLLVM only shows up in the stage1 sysroot for some reason:

$ fd libLLVM-12-rust-1.53.0-nightly.so build/
build/x86_64-unknown-linux-gnu/ci-llvm/lib/libLLVM-12-rust-1.53.0-nightly.so
build/x86_64-unknown-linux-gnu/stage1/lib/libLLVM-12-rust-1.53.0-nightly.so
build/x86_64-unknown-linux-gnu/ci-rustc/lib/libLLVM-12-rust-1.53.0-nightly.so
build/x86_64-unknown-linux-gnu/stage2/lib/libLLVM-12-rust-1.53.0-nightly.so

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 7d4c388 has been approved by Mark-Simulacrum

[jyn514]

I think --stage 1 is broken because libLLVM only shows up in the stage1 sysroot for some reason:

This is really odd - builder.sysroot(), builder.rustc_libdir(), and builder.sysroot_libdir() are all wildly different directories for the same compiler:

[src/bootstrap/tool.rs:396] builder.sysroot(compiler) = "/home/joshua/rustc/build/x86_64-unknown-linux-gnu/stage0-sysroot"
[src/bootstrap/tool.rs:400] builder.sysroot_libdir(compiler, compiler.host) = "/home/joshua/rustc/build/x86_64-unknown-linux-gnu/stage0-sysroot/lib/rustlib/x86_64-unknown-linux-gnu/lib"
[src/bootstrap/tool.rs:401] builder.rustc_libdir(compiler) = "/home/joshua/rustc/build/x86_64-unknown-linux-gnu/stage0/lib"

[jyn514]

Well, I guess rustc_libdir is the only one that looks sketchy.

[Mark-Simulacrum]

Those all look as expected to me, though without download-rustc; I don't really have an intuition for it.

[jyn514]

@Mark-Simulacrum well the reason it causes issues is that I tried this:

diff --git a/src/bootstrap/tool.rs b/src/bootstrap/tool.rs
index 4f2426648fd..42085a1bdae 100644
--- a/src/bootstrap/tool.rs
+++ b/src/bootstrap/tool.rs
@@ -391,6 +391,10 @@ pub fn command(builder: &Builder<'_>) -> Command {
         // use new syntax, but it should work otherwise.)
         let compiler = builder.compiler(builder.top_stage.saturating_sub(1), builder.config.build);
         let mut cmd = Command::new(builder.ensure(ErrorIndex { compiler }));
+        // because rustdoc depends on rustc_driver, error_index transitively depends on libLLVM.so.
+        // by default libLLVM is only copied in the assemble stage, so copy it explicitly here.
+        let sysroot = builder.sysroot(compiler);
+        crate::dist::maybe_install_llvm_runtime(builder, compiler.host, dbg!(&sysroot));
         add_dylib_path(
             vec![
                 PathBuf::from(&builder.sysroot_libdir(compiler, compiler.host)),

but that breaks because stage0-sysroot/lib is not in LD_LIBRARY_PATH, only stage0/lib.

[jyn514]

I don't think download-rustc is particularly relevant for the --stage 1 issue; I would expect it to be the same with or without.

[jyn514]

I really do think making the error-index not depend on LLVM is the simplest solution - happy to work on separating out the markdown part of rustdoc into a (perma-unstable) in-tree crate if you and @GuillaumeGomez are ok with it.

[Mark-Simulacrum]

Hm, but I wouldn't expect a compiler to exist in stage0-sysroot? That directory should only be used for building rustc with beta rustc and in-tree std, freshly built. So we shouldn't be putting things like LLVM in it. stage1/bin/rustdoc should use LLVM in stage1/lib.

Stage 0 rustdoc should use stage0/lib LLVM, I believe.

Likewise for error index built at each of those stages.

[jyn514]

Stage 0 rustdoc should use stage0/lib LLVM, I believe.

Ok, that seems reasonable. How do I get bootstrap to give me stage0 as the sysroot to pass to maybe_install_llvm_runtime? Should I just special-case stage 0 in the ErrorIndex step?

[Mark-Simulacrum]

Having the error index use rustdoc as a binary is likely simpler (maybe with some unstable opts), to the point of just moving its source in, likely would fix this too.

We shouldn't need to install LLVM into stage0/lib, it should already be there.

[jyn514]

Having the error index use rustdoc as a binary is likely simpler (maybe with some unstable opts), to the point of just moving its source in, likely would fix this too.

Ok, I can take a look at that.

We shouldn't need to install LLVM into stage0/lib, it should already be there.

It is not: #84471 (comment)

[jyn514]

Well, I said I'd take a look at it, but this seems to work ok:

diff --git a/src/bootstrap/tool.rs b/src/bootstrap/tool.rs
index 4f2426648fd..a7c7ff9730b 100644
--- a/src/bootstrap/tool.rs
+++ b/src/bootstrap/tool.rs
@@ -391,10 +391,18 @@ pub fn command(builder: &Builder<'_>) -> Command {
         // use new syntax, but it should work otherwise.)
         let compiler = builder.compiler(builder.top_stage.saturating_sub(1), builder.config.build);
         let mut cmd = Command::new(builder.ensure(ErrorIndex { compiler }));
+        // because rustdoc depends on rustc_driver, error_index transitively depends on libLLVM.so.
+        // by default libLLVM is only copied in the assemble stage, so copy it explicitly here.
+        // NOTE: this does *not* use `builder.sysroot(compiler)` because that gives `stage0-sysroot/` for the stage0 compiler,
+        // but we want `stage0/` to be consistent with the dynamic load path.
+        let rustc_libdir = builder.rustc_libdir(compiler);
+        let sysroot = rustc_libdir.parent().unwrap();
+        crate::dist::maybe_install_llvm_runtime(builder, compiler.host, dbg!(&sysroot));
+
         add_dylib_path(
             vec![
-                PathBuf::from(&builder.sysroot_libdir(compiler, compiler.host)),
-                PathBuf::from(builder.rustc_libdir(compiler)),
+                builder.sysroot_libdir(compiler, compiler.host).to_path_buf(),
+                rustc_libdir,
             ],
             &mut cmd,
         );

[jyn514]

We shouldn't need to install LLVM into stage0/lib, it should already be there.

It is not: #84471 (comment)

-beta is though: build/x86_64-unknown-linux-gnu/stage0/lib/libLLVM-12-rust-1.52.0-beta.so. Is there a reason the LLVM .so is named after the release channel instead of the commit it was built from? That would simplify things a lot.

[jyn514]

Is there a reason the LLVM .so is named after the release channel instead of the commit it was built from? That would simplify things a lot.

Oh, well, I don't think that's actually true - you could have an LLVM bump between beta and nightly. Naming the .so after the commit seems like a good cleanup, but I don't think it would actually help anything (other than get rid of a few duplicate .sos).

[bors]

⌛ Testing commit 7d4c388 with merge 7e717e9...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 7e717e9 to master...