Skip to content

Fix rustdoc execution on multiple targets and custom libdir #58947

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix rustdoc execution on multiple targets and custom libdir #58947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6def641
Fix rustdoc execution on multiple targets and custom libdir.
o01eg Mar 6, 2019
d8bd9d3
Fix custom relative libdir.
o01eg Mar 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/bootstrap/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,11 @@ impl<'a> Builder<'a> {
if compiler.is_snapshot(self) {
self.rustc_snapshot_libdir()
} else {
self.sysroot(compiler).join(libdir(&compiler.host))
match self.config.libdir_relative() {
Some(relative_libdir) if compiler.stage >= 1
=> self.sysroot(compiler).join(relative_libdir),
_ => self.sysroot(compiler).join(libdir(&compiler.host))
}
}
}

Expand Down Expand Up @@ -927,7 +931,8 @@ impl<'a> Builder<'a> {
cargo.env("RUSTC_ERROR_FORMAT", error_format);
}
if cmd != "build" && cmd != "check" && cmd != "rustc" && want_rustdoc {
cargo.env("RUSTDOC_LIBDIR", self.rustc_libdir(compiler));
// sysroot libdir required for case with custom libdir
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems suspicious. I would expect that we'd need to do a similar modification for RUSTC_LIBDIR, but that doesn't seem to be the case AFAICT.

A comment explaining why rustdoc should be different than rustc here would be good; it might be best to discuss that reasoning before adding the comment though since I'm not sure what it could be.

To clarify why this concerns me: I expect rustdoc and rustc to need/require the same libraries, so if one needs the sysroot/lib directory then the other should too, rather than rustc using sysroot/lib and rustdoc using sysroot/lib/rustlib//lib. The libraries in both places are the same in stage 2 currently so this might "work" in that stage on accident, but I wouldn't expect it to in stage 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't first time when I have to use this path for rustdoc library: #52439
#46592
Without it build fails even for host target.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I understand that this fixes it in your case (and maybe on CI). However, without understanding what goes wrong with your machine/config that causes that failure, and why this fixes it, I don't feel comfortable landing such a change. At the very least because it would seem reasonable to apply the same change to RUSTC_LIBDIR, too...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mark-Simulacrum What if use here stage2 compile for stage2 rustdoc? Then correct librustc_driver library should match with RUSTDOC_LIBDIR.

cargo.env("RUSTDOC_LIBDIR", self.sysroot_libdir(compiler, self.config.build));
}

if mode.is_tool() {
Expand Down
8 changes: 4 additions & 4 deletions src/bootstrap/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use filetime::FileTime;
use serde_json;

use crate::dist;
use crate::util::{exe, libdir, is_dylib};
use crate::util::{exe, is_dylib};
use crate::{Compiler, Mode, GitRepo};
use crate::native;

Expand Down Expand Up @@ -988,13 +988,13 @@ impl Step for Assemble {

// Link in all dylibs to the libdir
let sysroot = builder.sysroot(target_compiler);
let sysroot_libdir = sysroot.join(libdir(&*host));
t!(fs::create_dir_all(&sysroot_libdir));
let rustc_libdir = builder.rustc_libdir(target_compiler);
t!(fs::create_dir_all(&rustc_libdir));
let src_libdir = builder.sysroot_libdir(build_compiler, host);
for f in builder.read_dir(&src_libdir) {
let filename = f.file_name().into_string().unwrap();
if is_dylib(&filename) {
builder.copy(&f.path(), &sysroot_libdir.join(&filename));
builder.copy(&f.path(), &rustc_libdir.join(&filename));
}
}

Expand Down
28 changes: 16 additions & 12 deletions src/bootstrap/tool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,18 +426,22 @@ impl Step for Rustdoc {
return builder.initial_rustc.with_file_name(exe("rustdoc", &target_compiler.host));
}
let target = target_compiler.host;
// Similar to `compile::Assemble`, build with the previous stage's compiler. Otherwise
// we'd have stageN/bin/rustc and stageN/bin/rustdoc be effectively different stage
// compilers, which isn't what we want. Rustdoc should be linked in the same way as the
// rustc compiler it's paired with, so it must be built with the previous stage compiler.
let build_compiler = builder.compiler(target_compiler.stage - 1, builder.config.build);

// The presence of `target_compiler` ensures that the necessary libraries (codegen backends,
// compiler libraries, ...) are built. Rustdoc does not require the presence of any
// libraries within sysroot_libdir (i.e., rustlib), though doctests may want it (since
// they'll be linked to those libraries). As such, don't explicitly `ensure` any additional
// libraries here. The intuition here is that If we've built a compiler, we should be able
// to build rustdoc.
let build_compiler = if target_compiler.stage >= 2 {
// Past stage 2, we consider the compiler to be ABI-compatible and hence capable of
// building rustdoc itself.
builder.compiler(target_compiler.stage, builder.config.build)
} else {
// Similar to `compile::Assemble`, build with the previous stage's compiler. Otherwise
// we'd have stageN/bin/rustc and stageN/bin/rustdoc be effectively different stage
// compilers, which isn't what we want.
builder.compiler(target_compiler.stage - 1, builder.config.build)
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we complicate the build_compiler logic here? It seems like we would already have the stage - 1 compiler anyway so it seems simpler to leave this as is? We should have justification here for anything we do differently from compile::Assemble.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because without this logic it fails on documenting alloc for wasm32-unknown-unknown, with this logic it successfully compiles and installs rust.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I understand that this fixes your use case. That's not what I'm trying to ask for, though; I want to understand why it doesn't work for rustdoc but does for rustc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like target_compiler.stage - 1 causes stage2 rustdoc called with stage1 library path.


// require host compiler so documentation of other targets won't fail on missing libraries
builder.ensure(compile::Rustc {
compiler: build_compiler,
target: builder.config.build,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fairly certain this shouldn't be here; if we're going to build rustdoc successfully then we already have the relevant libraries for it to build. If it's being used to build documentation (e.g. with --target) then the step that calls that should be calling this compile.

Basically, the question we should always ask is "does everything which uses this step need this built?" and in this case that question (I think) should be answered with a no -- e.g. documenting core shouldn't need any target libraries (or, indeed, any libraries).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this ensure into Documenting stage2 std (wasm32-unknown-unknown) step because this step fails?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely shouldn't need librustc and document std if we've built rustdoc successfully. I suspect this change being required is indicative of other problems.


let mut cargo = prepare_tool_cargo(
builder,
Expand Down