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

Don't build the compiler before building rust-demangler #97511

Merged
merged 1 commit into from
Jun 19, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented May 29, 2022

This saves a lot of time compiling, since rust-demangler doesn't actually use any unstable features.

This is not quite ideal because it uses ToolStd, not ToolBootstrap, so rust-demangler would be able to add unstable library features in the future. But it's a lot better than before, and builder.cargo doesn't currently know how to handle stages other than 0.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

At least tests should probably run with the new rustc, to make sure we're not shipping a broken binary. I'm not quite sure how Cargo detects the rustc though, so maybe that'll already happen with this PR - will take a closer look later. I think some Cargo tests exercise unstable rustc features, but those might be only run in their CI, not sure.

I'm not entirely opposed to moving these to bootstrap tools, but at least for Cargo I think it'd be better to build it with latest rustc. It's true it can build with stable, but I'd rather avoid problems during stage0-stepping and get the additional testing from building with a fresh compiler. Plus, per earlier point around tests, we would end up building Cargo twice. I think today RLS also directly links to Cargo and Clippy, so there's a must-build in toolrustc mode there.

@jyn514
Copy link
Member Author

jyn514 commented May 29, 2022

I think today RLS also directly links to Cargo and Clippy, so there's a must-build in toolrustc mode there.

ah, that's a good point - I'll come back to this once we stop shipping RLS in a few cycles. I think the test issues should be avoidable - we may have to pass a different sysroot to cargo or something, but that seems doable.

For now I'll change this to only move rust-demangler to a bootstrap tool.

@rustbot author

@rustbot rustbot 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 May 29, 2022
@bjorn3
Copy link
Member

bjorn3 commented May 29, 2022

Won't this break building cargo and rust-demangler for a target other than the build system?

@jyn514
Copy link
Member Author

jyn514 commented May 29, 2022

@bjorn3 ah, you mean it should be possible to cross-compile rust-demangler? That makes sense, but they still shouldn't be ToolRustc tools, they don't depend on rustc_private. I'll try ToolStd and see if that works.

This saves a lot of time compiling.
@jyn514 jyn514 changed the title Don't build the compiler before building cargo or rust-demangler Don't build the compiler before building rust-demangler Jun 8, 2022
@jyn514
Copy link
Member Author

jyn514 commented Jun 8, 2022

Ok, I've updated this to be a significantly smaller and more targeted patch. It also only affects rust-demangler and not cargo. I think it should be possible to do something similar with cargo, but I'll avoid doing so while RLS depends on it (to avoid building cargo twice).

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 8, 2022
@Mark-Simulacrum
Copy link
Member

r=me, if you don't want to investigate further

I am a little surprised by the need to avoid only compiling via ToolBootstrap -- is the problem with stage0 that we can't cross-compile in that stage? Otherwise it seems to me like it should be fine to be in stage 0 unless we're trying to save compile cycles (e.g., due to shared dep trees with other steps).

@Mark-Simulacrum Mark-Simulacrum 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 Jun 9, 2022
@jyn514
Copy link
Member Author

jyn514 commented Jun 18, 2022

I am a little surprised by the need to avoid only compiling via ToolBootstrap -- is the problem with stage0 that we can't cross-compile in that stage? Otherwise it seems to me like it should be fine to be in stage 0 unless we're trying to save compile cycles (e.g., due to shared dep trees with other steps).

Tried this out - yes, we don't have the target library available in stage 0. So we need to build the standard library anyway, and at that point I think it's equivalent to ToolStd.

I did notice that we currently have ONLY_HOSTS = true for all extended tools - that seems a bit odd? I would expect them to be fine to cross compile; I guess rustc would need to be cross-compiled too but that seems fine.

@bors r=Mark-Simulacrum rollup

@bors
Copy link
Contributor

bors commented Jun 18, 2022

📌 Commit 37f9cdb has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 18, 2022
@jyn514
Copy link
Member Author

jyn514 commented Jun 18, 2022

hmm, cross-compiling in stage 0 with ToolStd seems to also be broken

$ x b src/tools/rust-demangler --target x86_64-unknown-linux-musl --stage 0
thread 'main' panicked at 'src.symlink_metadata() failed with No such file or directory (os error 2)', src/bootstrap/lib.rs:1440:24

but I think that's a pre-existing bug.

never mind, this is because I was trying to compile a musl toolchain without a musl root

@Mark-Simulacrum
Copy link
Member

FWIW, I've been thinking that we might want to push tools we ship to build in stage 1 or later, since that avoids needing to special case or wait a cycle for newly introduced targets. But in practice moving from non-existent to tier 2 in one cycle probably is pretty rare anyway, so it might not be that important.

@jyn514
Copy link
Member Author

jyn514 commented Jun 18, 2022

Yeah, I think there are a lot more obstacles to adding targets than just needing 6 weeks to build the tools. I've been meaning to try and document adding new targets at some point; that, #94829, downloading bootstrap from CI, and #97762 are my biggest priorities at the moment.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 18, 2022
…imulacrum

Don't build the compiler before building rust-demangler

This saves a lot of time compiling, since rust-demangler doesn't actually use any unstable features.

This is not quite ideal because it uses ToolStd, not ToolBootstrap, so rust-demangler would be able to add unstable library features in the future. But it's a lot better than before, and `builder.cargo` doesn't currently know how to handle stages other than 0.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 18, 2022
…imulacrum

Don't build the compiler before building rust-demangler

This saves a lot of time compiling, since rust-demangler doesn't actually use any unstable features.

This is not quite ideal because it uses ToolStd, not ToolBootstrap, so rust-demangler would be able to add unstable library features in the future. But it's a lot better than before, and `builder.cargo` doesn't currently know how to handle stages other than 0.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2022
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#97511 (Don't build the compiler before building rust-demangler)
 - rust-lang#98165 (once cell renamings)
 - rust-lang#98207 (Update cargo)
 - rust-lang#98229 (Add new eslint checks)
 - rust-lang#98230 (Fix weird js condition)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e6d28d2 into rust-lang:master Jun 19, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 19, 2022
@jyn514 jyn514 deleted the faster-cargo-build branch June 19, 2022 04:13
notriddle added a commit to notriddle/rust that referenced this pull request Oct 5, 2022
Don't build the compiler before building rls

The rls stub is a simple stable tool, which doesn't need compiler libs.
(Similar to rust-lang#97511)
notriddle added a commit to notriddle/rust that referenced this pull request Oct 5, 2022
Don't build the compiler before building rls

The rls stub is a simple stable tool, which doesn't need compiler libs.
(Similar to rust-lang#97511)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants