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

x.py test linkchecker builds rustc twice #76371

Closed
jyn514 opened this issue Sep 5, 2020 · 13 comments
Closed

x.py test linkchecker builds rustc twice #76371

jyn514 opened this issue Sep 5, 2020 · 13 comments
Labels
C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@jyn514
Copy link
Member

jyn514 commented Sep 5, 2020

Found in https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/tool.20linkchecker.20needs.20stage.202.20compiler.20.3F: Currently, x.py doc uses --stage 0, but x.py linkchecker uses --stage 2. This causes enormous recompiles for no reason; the two should match.

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-bug Category: This is a bug. labels Sep 5, 2020
@ehuss
Copy link
Contributor

ehuss commented Sep 5, 2020

I believe x.py test src/tools/linkchecker defaults to runs at stage 1 like all other tests. Is there some config or something causing it to run at stage 2?

I'd be slightly concerned running it at stage 0, since some doc tools need to link with the compiler, they would be using an old compiler in that case. For quick checks, it should be OK, but like running any tests at stage 0, it is incomplete.

Also, I believe not all docs can be built at stage 0 (for example, if you have compiler-docs enabled), which could cause confusing errors. That should probably have some check to skip or error with compiler docs at stage 0.

@jyn514
Copy link
Member Author

jyn514 commented Sep 5, 2020

Hmm, this is a good point. @lzutao what do you think?

@jyn514
Copy link
Member Author

jyn514 commented Sep 5, 2020

I'd be slightly concerned running it at stage 0, since some doc tools need to link with the compiler, they would be using an old compiler in that case. For quick checks, it should be OK, but like running any tests at stage 0, it is incomplete.

In particular, they'd be missing recent fixes to intra-doc links which would fix broken links, yeah.

@tesuji
Copy link
Contributor

tesuji commented Sep 5, 2020

I don't actually recall the stage 2 std or stage 2 compiler was built with linkchecker,
it actually took long enough that I don't want to run it again.

My config.toml:

ccache = true
ninja = true
targets = "AArch64;ARM;Mips;X86"
cargo = "/home/lzutao/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo"
rustc = "/home/lzutao/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rustc"
submodules = false
local-rebuild = true
codegen-units = 0
incremental = true
llvm-config = "/home/lzutao/.local/bin/llvm-config"

@ehuss
Copy link
Contributor

ehuss commented Sep 11, 2020

After looking at this in more detail, I think the issue is that rustc is built twice. The second build is due to the error-index-generator, which links to rustc and rustdoc in order to reuse the markdown renderer. I'm not sure if there's a particularly good way to work around that. AFAIK, linking to rustc must use a stage1 build. The only alternative I can think of is using a different renderer (either mdbook, or something custom using pulldown-cmark?). I'm not sure that would be really worth it, but something to consider.

@Mark-Simulacrum
Copy link
Member

You can definitely link to rustc libraries in any stage - not sure what you mean by stage 1 there.

In the long run I think mdbook is probably a good idea for the error index though.

@ehuss
Copy link
Contributor

ehuss commented Sep 11, 2020

Yea, I was thinking about it more, and I was mistaken. I was thinking about ABI issues (with stage1-tools built with a stage1 compiler linking against stage0-rustc built with a stage0 compiler). But I guess it should use the same trick that rustdoc does to handle this (I think error-index-generator would need to replicate most of this logic to use stage-1).

A problem with mdbook is that it can't output to a single file. The URL https://doc.rust-lang.org/error-index.html might need to change to something like https://doc.rust-lang.org/error-index/index.html, which could be a little disruptive (at least needing a redirect). I'm also curious if it would be better to have each error on a separate page, or continue with one large page. (I'm uncertain about how slow a single page would be, considering mdbook does client-side syntax highlighting.) I'm also uncertain if anyone ever visits the error-index page at all (seems a bit silly to put effort into something nobody uses).

@Mark-Simulacrum do either of those approaches seem preferable (or neither)?

@jyn514
Copy link
Member Author

jyn514 commented Sep 11, 2020

I'm also uncertain if anyone ever visits the error-index page at all (seems a bit silly to put effort into something nobody uses).

Maybe a simpler solution would be to make the error-index opt-in instead of opt-out, so that it's only generated by CI? Then you wouldn't have to mess with it to get x.py test linkchecker --stage 0 working.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Sep 11, 2020

I think the redirect shouldn't be too hard, I wouldn't see that as a problem. Distinct pages are a bit annoying, though I imagine that 99% of the time people are looking at just one error of the error index rather than all of them at the same time.

I think personally I'd prefer to make the error index generator not use rustdoc/rustc as a library. Maybe an easy way of doing that is to just roll the single file into rustdoc itself behind a forever unstable -Z flag or something, rather than trying to re-engineer a bunch of mdbook stuff.

@jyn514 jyn514 changed the title Default stage for x.py test linkchecker should match doc x.py test linkchecker builds rustc twice Sep 30, 2020
@ehuss
Copy link
Contributor

ehuss commented Jan 31, 2021

I posted #81603 to avoid the second build with --stage=1. I think there is still value to re-architect how error-index-generator works, but I wanted to do a smaller change for now rather than invest time in redesigning the tool.

jackh726 added a commit to jackh726/rust that referenced this issue Feb 2, 2021
…ulacrum

rustbuild: Don't build compiler twice for error-index-generator.

When using `--stage=1`, the error-index-generator was forcing the compiler to be built twice.  This isn't necessary; the error-index-generator just needs the same unusual logic that rustdoc uses to build with stage minus one.

`--stage=0` and `--stage=2` should be unaffected by this change.

cc rust-lang#76371
jackh726 added a commit to jackh726/rust that referenced this issue Feb 2, 2021
…ulacrum

rustbuild: Don't build compiler twice for error-index-generator.

When using `--stage=1`, the error-index-generator was forcing the compiler to be built twice.  This isn't necessary; the error-index-generator just needs the same unusual logic that rustdoc uses to build with stage minus one.

`--stage=0` and `--stage=2` should be unaffected by this change.

cc rust-lang#76371
@jyn514
Copy link
Member Author

jyn514 commented May 1, 2021

@ehuss this should be fixed since #81603, right?

@ehuss
Copy link
Contributor

ehuss commented May 1, 2021

The original issue should be fixed, so I think this can be closed. I kept it open for the idea of rewriting error-index-generator to use a different technique. However, I personally have no plans to do that.

@jyn514
Copy link
Member Author

jyn514 commented May 1, 2021

Ok, I'll close this issue then - the linkchecker is already somewhat tracked by #80096.

@jyn514 jyn514 closed this as completed May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

No branches or pull requests

4 participants