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

Fix cargo run tidy #94806

Merged
merged 1 commit into from
Mar 31, 2022
Merged

Fix cargo run tidy #94806

merged 1 commit into from
Mar 31, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 10, 2022

When I implemented rust-only bootstrapping in #92260,
I neglected to test stage0 tools - it turns out they were broken because
they couldn't find the sysroot of the initial bootstrap compiler.

This fixes stage0 tools by using rustc --print sysroot instead of assuming rustc is already in a
sysroot and hard-coding the relative directory.

Fixes #94797 (properly, without having to change rustup).

@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 10, 2022
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

The rustfmt initialization (for x.py fmt) may also need similar-ish treatment in config.rs -- I suspect the current logic there will just replace ~/.cargo/bin/rustc with ~/.cargo/bin/rustfmt which will be similarly ineffective.

src/bootstrap/lib.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

But I'm a little confused: does this actually work? ~/.cargo/bin/rustc --print sysroot would give you the default toolchains sysroot, not the one that you were actually running in the parent process, no? Or is that somehow magically being handled already?

@jyn514
Copy link
Member Author

jyn514 commented Mar 10, 2022

Or is that somehow magically being handled already?

It's magically handled by the rustup proxy - it sets RUSTUP_TOOLCHAIN in the environment to make sure rustc does the right thing when cargo calls it. See around https://github.com/rust-lang/rustup/blob/d080411224dca896dbff18b02feabea16a95d9ed/src/toolchain.rs#L402.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Mar 10, 2022

Ok, great. @rustbot author. once the two nits are fixed, likely r=me

@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2022

Error: Parsing shortcut command in comment failed: ...'bot author' | error: expected end of command at >| ' -- once t'...

Please let @rust-lang/release know if you're having trouble with this bot.

@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 10, 2022
When I implemented rust-only bootstrapping in rust-lang#92260,
I neglected to test stage0 tools - it turns out they were broken because
they couldn't find the sysroot of the initial bootstrap compiler.

This fixes stage0 tools by using `rustc --print sysroot` instead of assuming rustc is already in a
sysroot and hard-coding the relative directory.
@jyn514
Copy link
Member Author

jyn514 commented Mar 10, 2022

I suspect the current logic there will just replace ~/.cargo/bin/rustc with ~/.cargo/bin/rustfmt which will be similarly ineffective.

This should be ok I think - it's just the sysroot detection that was broken, using the rustup proxy instead of rustfmt itself should work fine.

@jyn514
Copy link
Member Author

jyn514 commented Mar 10, 2022

Yup, looks fine:

$ cargo run -q fmt src/bootstrap
skip untracked path config.toml.bak during rustfmt invocations
skip untracked path empty.rs during rustfmt invocations
skip untracked path fds-local-no-requests.txt during rustfmt invocations
skip untracked path fds-local.txt during rustfmt invocations
skip untracked path src/build/ during rustfmt invocations
skip untracked path src/test/rustdoc/intra-doc/private-macro.rs during rustfmt invocations

@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 10, 2022
jyn514 added a commit to jyn514/rust that referenced this pull request Mar 10, 2022
…otstrap`

While working on rust-lang#94806, I was very confused
because the `trim()` fix didn't actually do anything and still errored out.
It turns out the issue was that bootstrap had been rebuilt, but not fake rustc.
Rebuild all the fake binaries automatically from within bootstrap to avoid issues like this in the future.
@Mark-Simulacrum
Copy link
Member

Presumably that will not work with RUSTUP_TOOLCHAIN, since while Cargo and rustc are sourced from the same sysroot (beta) rustfmt needs to be from a different one presumably (nightly-XXXX) - doesn't necessarily need to block this PR, but I would like to understand if this will be a problem down the line or not.

@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
@jyn514
Copy link
Member Author

jyn514 commented Mar 20, 2022

while Cargo and rustc are sourced from the same sysroot (beta) rustfmt needs to be from a different one presumably (nightly-XXXX) - doesn't necessarily need to block this PR, but I would like to understand if this will be a problem down the line or not.

Yes, this popped up in another PR, it's an existing issue: #94830 (comment)

Do you mind if I leave it for later? I think the logic for fixing this will be very similar to what's needed for download-rustc (i.e. fairly involved, using curl and such to download/unpack things) so I would rather wait until that infrastructure is in place.

@jyn514
Copy link
Member Author

jyn514 commented Mar 20, 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 20, 2022
@Mark-Simulacrum
Copy link
Member

Sure, that works for me.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 30, 2022

📌 Commit 25a7d2d 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 30, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 30, 2022
…acrum

Fix `cargo run tidy`

When I implemented rust-only bootstrapping in rust-lang#92260,
I neglected to test stage0 tools - it turns out they were broken because
they couldn't find the sysroot of the initial bootstrap compiler.

This fixes stage0 tools by using `rustc --print sysroot` instead of assuming rustc is already in a
sysroot and hard-coding the relative directory.

Fixes rust-lang#94797 (properly, without having to change rustup).
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 30, 2022
…acrum

Fix `cargo run tidy`

When I implemented rust-only bootstrapping in rust-lang#92260,
I neglected to test stage0 tools - it turns out they were broken because
they couldn't find the sysroot of the initial bootstrap compiler.

This fixes stage0 tools by using `rustc --print sysroot` instead of assuming rustc is already in a
sysroot and hard-coding the relative directory.

Fixes rust-lang#94797 (properly, without having to change rustup).
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 30, 2022
…acrum

Fix `cargo run tidy`

When I implemented rust-only bootstrapping in rust-lang#92260,
I neglected to test stage0 tools - it turns out they were broken because
they couldn't find the sysroot of the initial bootstrap compiler.

This fixes stage0 tools by using `rustc --print sysroot` instead of assuming rustc is already in a
sysroot and hard-coding the relative directory.

Fixes rust-lang#94797 (properly, without having to change rustup).
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#93901 (Stabilize native library modifier syntax and the `whole-archive` modifier specifically)
 - rust-lang#94806 (Fix `cargo run tidy`)
 - rust-lang#94869 (Add the generic_associated_types_extended feature)
 - rust-lang#95011 (async: Give predictable name to binding generated from .await expressions.)
 - rust-lang#95251 (Reduce max hash in raw strings from u16 to u8)
 - rust-lang#95298 (Fix double drop of allocator in IntoIter impl of Vec)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 943ea7f into rust-lang:master Mar 31, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 31, 2022
@jyn514 jyn514 deleted the cargo-run-tidy branch March 31, 2022 12:53
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 7, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#93901 (Stabilize native library modifier syntax and the `whole-archive` modifier specifically)
 - rust-lang#94806 (Fix `cargo run tidy`)
 - rust-lang#94869 (Add the generic_associated_types_extended feature)
 - rust-lang#95011 (async: Give predictable name to binding generated from .await expressions.)
 - rust-lang#95251 (Reduce max hash in raw strings from u16 to u8)
 - rust-lang#95298 (Fix double drop of allocator in IntoIter impl of Vec)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo run -p bootstrap -- test tidy fails because it can't find the sysroot
6 participants