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

bootstrap: add rust-toolchain to the root directory #94830

Closed
wants to merge 1 commit into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 11, 2022

Helps with #94829.

This doesn't solve "bootstrap uses beta rustc but nightly rustfmt",
but in practice there should be very little difference between two.
Users can still override the toolchain used with cargo +nightly or similar.

Helps with rust-lang#94829.

This doesn't solve "bootstrap uses beta rustc but nightly rustfmt",
but in practice there should be very little difference between two.
Users can still override the toolchain used with `cargo +nightly` or similar.
@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 Mar 11, 2022
@Mark-Simulacrum
Copy link
Member

I think src/tools/bump-stage0 should also adjust this file before we introduce it -- hopefully not too difficult to implement, I think we already have a toml library there. (And just hard-coding string formatting would also be fine).

Can you say a little more about the expected effects of adding this? I imagine that editor tooling for example (e.g., rust-analyzer) might be better off if it was using a nightly rather than beta, since cfg(bootstrap) is typically not set in the environment and so nightly will likely work but beta may not (leading to worse autocompletion). Obviously that particular side effect, if real, is fixable by flipping the bootstrap cfg (i.e., making it cfg(next) or something).

Given that we're not fully ready for users to make use of it yet (right?), it seems like adding it might be a little premature. I'm particularly uncertain about beta rustfmt - I would expect that to not work at all, as our rustfmt.toml uses unstable features (or x.py fmt, I forget).

@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 Mar 13, 2022
@bjorn3
Copy link
Member

bjorn3 commented Mar 14, 2022

I would expect that to not work at all, as our rustfmt.toml uses unstable features

Indeed. Some files get formatted differently when trying to use a non-nightly rustfmt.

@jyn514
Copy link
Member Author

jyn514 commented Mar 14, 2022

I think src/tools/bump-stage0 should also adjust this file before we introduce it -- hopefully not too difficult to implement, I think we already have a toml library there. (And just hard-coding string formatting would also be fine).

👍

Can you say a little more about the expected effects of adding this? I imagine that editor tooling for example (e.g., rust-analyzer) might be better off if it was using a nightly rather than beta, since cfg(bootstrap) is typically not set in the environment and so nightly will likely work but beta may not (leading to worse autocompletion). Obviously that particular side effect, if real, is fixable by flipping the bootstrap cfg (i.e., making it cfg(next) or something).

You're right, that's indeed an issue currently. Flipping the cfgs is one possible fix; another is to add --cfg=bootstrap to the recommended options in https://rustc-dev-guide.rust-lang.org/building/suggested.html#configuring-rust-analyzer-for-rustc.

I don't think there should be a ton of other side-effects - the only other workflow I've heard mentioned (by @ehuss) that doesn't go through x.py is running cargo tree and similar things in the root, which should still work fine with a beta cargo.

I'm particularly uncertain about beta rustfmt - I would expect that to not work at all, as our rustfmt.toml uses unstable features (or x.py fmt, I forget).

bootstrap completely ignores rust-toolchain. This only affects people using cargo fmt instead of x.py fmt, which was already pretty broken.

@jyn514
Copy link
Member Author

jyn514 commented Mar 14, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 14, 2022
@jyn514
Copy link
Member Author

jyn514 commented Mar 14, 2022

err wait I missed the part where I implement the parsing in bump-stage0 🤦

@rustbot label: -S-waiting-on-review +S-waiting-on-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 Mar 14, 2022
@bjorn3
Copy link
Member

bjorn3 commented Mar 14, 2022

bootstrap completely ignores rust-toolchain. This only affects people using cargo fmt instead of x.py fmt, which was already pretty broken.

In that case rust-toolchain shouldn't list rustfmt, right?

@jyn514
Copy link
Member Author

jyn514 commented Mar 14, 2022

In that case rust-toolchain shouldn't list rustfmt, right?

oh, good point - I'll remove that :)

I think I may still need to add some logic to rustbuild to download nightly rustfmt in that case actually. So @Mark-Simulacrum is right and this is premature until rustbuild manages rustfmt.

@bjorn3
Copy link
Member

bjorn3 commented Mar 14, 2022

I imagine that editor tooling for example (e.g., rust-analyzer) might be better off if it was using a nightly rather than beta,

Editor tooling should still use ./x.py check. cargo check --workspace (which is what r-a uses) doesn't work at all even with RUSTFLAGS="--cfg bootstrap" AFAIK.

@jyn514 jyn514 marked this pull request as draft March 14, 2022 12:04
@Mark-Simulacrum
Copy link
Member

FWIW, my primary worry is in-editor rustfmt invocations. I think today people need to mostly manually override that, which should still work even after we add rust-toolchain, but it would be amazing if that just magically did the right thing. Those typically don't invoke cargo fmt I think, instead invoking rustfmt directly.

@ehuss
Copy link
Contributor

ehuss commented Mar 14, 2022

I use tools that call things like rustfmt, cargo update, and cargo metadata where this would be a bit annoying. It would also cause the rustup folder to grow toolchains that are only temporarily used. I can live with it, but this would be a net drawback for me and a bit frustrating.

@jyn514
Copy link
Member Author

jyn514 commented Mar 20, 2022

FWIW, my primary worry is in-editor rustfmt invocations. I think today people need to mostly manually override that, which should still work even after we add rust-toolchain, but it would be amazing if that just magically did the right thing.

I agree, and I've opened #95136 to support that in bootstrap without having to wait for rustup support. I don't think that particular issue should block this PR, since it doesn't regress anything for rustfmt. I promise to get that working before I start recommending cargo run or changing any documentation :)

I use tools that call things like rustfmt, cargo update, and cargo metadata where this would be a bit annoying.

@ehuss can you talk a little bit more about that? What tools are you using? If they only need cargo update and cargo metadata, maybe they could call out to rustup which cargo --toolchain $(rustup default) instead? If you do use rustfmt, I think it's actually a good thing it uses the specified toolchain, since otherwise it could format the tree differently and cause unnecessary changes.

@ehuss
Copy link
Contributor

ehuss commented Mar 20, 2022

What tools are you using? If they only need cargo update and cargo metadata, maybe they could call out to rustup which cargo --toolchain $(rustup default) instead?

When I do weekly updates, I use a tool that calls cargo metadata to collect information about what is being updated, and cargo update to update Cargo.lock as needed.

This will come up for anyone who uses cargo update to bump something in the lockfile.

I also use cargo as-is for simple tools in the src/tools directory.

I can set a directory override to work around this. I'm just pointing out that this may be a new source of friction for some people.

If you do use rustfmt, I think it's actually a good thing it uses the specified toolchain, since otherwise it could format the tree differently and cause unnecessary changes.

In 99.9% of situations it isn't important which rustfmt is being used. I can't recall a bootstrap bump causing format changes happening within the past few years.

@jyn514
Copy link
Member Author

jyn514 commented May 25, 2022

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants