Fix rustdoc execution on multiple targets and custom libdir #58947
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Could you specify exactly what commands you're running (and relevant configuration)? I'm pretty sure that the current PR as-is shouldn't land due to reverting the behavior change in #58238. We expect that |
|
config.toml: Environment: Commands: Actual build script: https://github.com/gentoo/gentoo-rust/blob/master/dev-lang/rust/rust-9999.ebuild |
|
Tried to restore stage 0 behavior. |
|
Removed |
|
Reverted back |
|
And it looks like |
|
I'll try to find some time for another review soon. |
|
Looks like target compiler doesn't required but host compiler libraries are required to document something. |
|
Add condition so required reverts will be applied only for other than host targets. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Still failed: build-with-cond-ensure.log |
|
Looks like it never true |
| @@ -926,7 +926,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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
@Mark-Simulacrum What if use here stage2 compile for stage2 rustdoc? Then correct librustc_driver library should match with RUSTDOC_LIBDIR.
| // 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) | ||
| }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Because without this logic it fails on documenting alloc for wasm32-unknown-unknown, with this logic it successfully compiles and installs rust.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Looks like target_compiler.stage - 1 causes stage2 rustdoc called with stage1 library path.
| builder.ensure(compile::Rustc { | ||
| compiler: build_compiler, | ||
| target: builder.config.build, | ||
| }); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Maybe move this ensure into Documenting stage2 std (wasm32-unknown-unknown) step because this step fails?
There was a problem hiding this comment.
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.
|
Rebased and squashed commits. Added output of |
|
I've just reapplied #58238 and it built: build-successfull-2.log |
Mark-Simulacrum
added
S-waiting-on-author
|
Second try with the same patches failed: build-unsuccessfull-3.log |
|
And there successful build with the same patches: |
|
Unsuccessful: rust-fail.log |
|
Is some step could delete Any librustc_driver-* are LD_LIBRARY_PATH'es are : And there it fails |
|
There two variants:
|
|
I've managed to fix first path in LD_LIBRARY_PATH but cann't found where last one generated: @Mark-Simulacrum could you say source of last path in LD_LIBRARY_PATH ? |
|
Second commit cause librustc_driver-.so and libstd-.so missing in |
|
Issue was fixed with #58897 |
…ulacrum Fix custom relative libdir While working on rust-lang#58947 I found out relative libdir ignored during setting LD_LIBRARY_PATH.
…ulacrum Fix custom relative libdir While working on rust-lang#58947 I found out relative libdir ignored during setting LD_LIBRARY_PATH.
…ulacrum Fix custom relative libdir While working on rust-lang#58947 I found out relative libdir ignored during setting LD_LIBRARY_PATH.
Fixes #58587
Reverts #58238
tool.rschanges in #58238 works if there is a one build.target in config but if I add wasm target build breaks as well.