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

Add sysroot detection in rustdoc #78926

Closed
wants to merge 1 commit into from

Conversation

robinmoussu
Copy link

When building rustdoc without using x.py, the sysroot is not going
to be detected automatically. It wasn't an issue when using x.py, and
this commit should not have any effect when building rustdoc as part
of the rustc build.

Note: if you want to build rustdoc out-of-tree, you need to install the
rustup components rustc-dev and llvm-tools-preview.

r? @simulacrum
@jyn514

Comment on lines +146 to +153
.map(|mut args: Vec<String>| {
// Make sure we use the right default sysroot. The default sysroot is wrong,
// because `get_or_default_sysroot` in `librustc_session` bases that on `current_exe`.
//
// Make sure we always call `compile_time_sysroot` as that also does some sanity-checks
// of the environment we were built in.
// FIXME: Ideally we'd turn a bad build env into a compile-time error via CTFE or so.
if let Some(sysroot) = compile_time_sysroot() {
let sysroot_flag = "--sysroot";
if !args.iter().any(|e| e == sysroot_flag) {
// We need to overwrite the default that librustc_session would compute.
args.push(sysroot_flag.to_owned());
args.push(sysroot);
}
}

args
Copy link
Author

Choose a reason for hiding this comment

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

I'm not 100% sure that it's the most readable way to update the Vec<String> in this Option.

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 10, 2020
@Mark-Simulacrum Mark-Simulacrum self-assigned this Nov 10, 2020
src/librustdoc/lib.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 11, 2020
@Mark-Simulacrum
Copy link
Member

r? @jyn514

I think this is probably "fine" though I'm not personally really all that happy to see it. It seems like it may be worthwhile to consider putting this logic in rustc_driver (or similar), though, to avoid duplicating it into all the tools.

src/librustdoc/lib.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Nov 12, 2020

At least for Miri, I don't see how to easily move this logic to rustc_driver. We specifically want to capture environment variables set during the Miri build, not when the librustc_driver for the sysroot gets built.

What would help, however, is if whoever instantiates the driver could affect get_or_default_sysroot to use a different default sysroot.

@Mark-Simulacrum
Copy link
Member

Well, we could macro-ize it or something to get the expansion to be in the miri build I guess.

I imagine it should be possible to provide a global static of some kind that drivers can set to affect get_or_default_sysroot though.

When building `rustdoc` without using `x.py`, the sysroot is not going
to be detected automatically. It wasn't an issue when using `x.py`, and
this commit should not have any effect when building `rustdoc` as part
of the `rustc` build.

Note: if you want to build rustdoc out-of-tree, you need to install the
rustup components `rustc-dev` and `llvm-tools-preview`.
@robinmoussu
Copy link
Author

Where should this code be moved? In a macro in rustc_driver?

@RalfJung
Copy link
Member

This code is a hack needed to avoid changing the code in rustc itself. IMO it makes no sense to move it to move it to the rustc repo. If rustc wants to support this usecase, instead get_or_default_sysroot should be changed to allow drivers to overwrite the default sysroot.

@RalfJung
Copy link
Member

Specifically, this code hard-codes the default sysroot to be "one path above the binary":

let mut p = canonicalize(exe);
p.pop();
p.pop();
p

That doesn't work for Miri, which is why it needs that hack which this PR copies to rustdoc.

@flip1995
Copy link
Member

FYI: Clippy does this too: https://github.com/rust-lang/rust-clippy/blob/408b615d3451d00c21b303fd8aeb74b77970a4a9/src/driver.rs#L301-L344

Summary:

        // Get the sysroot, looking from most specific to this invocation to the least:
        // - command line
        // - runtime environment
        //    - SYSROOT
        //    - RUSTUP_HOME, MULTIRUST_HOME, RUSTUP_TOOLCHAIN, MULTIRUST_TOOLCHAIN
        // - sysroot from rustc in the path
        // - compile-time environment
        //    - SYSROOT
        //    - RUSTUP_HOME, MULTIRUST_HOME, RUSTUP_TOOLCHAIN, MULTIRUST_TOOLCHAIN

The compile time sysroot is only the fallback for us. This is because we need clippy_driver to also work on single files, just like rustc, IIRC. (The discussion about this starts here: rust-lang/rust-clippy#5071 (comment))

@robinmoussu
Copy link
Author

I am not sure that I understand why I need to do to make this issue progress forward. Could you guide me?

@jyn514
Copy link
Member

jyn514 commented Nov 17, 2020

@robinmoussu right now the default sysroot is hard-coded in rustc. It should be configurable by tools (or any program using rustc_private) so they don't need to add the hack you're adding here. #78926 (comment) has some suggestions on what code to change.

@crlf0710 crlf0710 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 Dec 4, 2020
@crlf0710
Copy link
Member

crlf0710 commented Dec 4, 2020

@robinmoussu Ping from triage, any updates on this?

@jyn514
Copy link
Member

jyn514 commented Dec 5, 2020

@crlf0710 this is a good refactor to have, but it's not really what @robinmoussu signed up for when he made the PR. Also it will probably not be used by rustdoc even if merged, since #79540 is more the approach I see the team taking, we are not currently planning to switch rustdoc to building out of tree.

So if @robinmoussu wants to keep working on this more power to him, but I think it makes more sense for @RalfJung or @flip1995 to pick it up since they're the ones that will use it (or we can just make this an issue since the PR implementation is not on the right track).

@robinmoussu
Copy link
Author

I agree with what @jyn514 said. Given that my bandwidth is more limited that what I'd like, I will not be able to work on it more than that. You can totally close this PR if you think it's not useful as-it.

@RalfJung
Copy link
Member

RalfJung commented Dec 6, 2020

Okay, let's close it then. Thanks @robinmoussu for exploring this solution, even if it did not work out!

@RalfJung RalfJung closed this Dec 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants