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

Make x.py clippy download and use beta clippy #97443

Closed
wants to merge 2 commits into from

Conversation

asquared31415
Copy link
Contributor

@asquared31415 asquared31415 commented May 26, 2022

This is not complete, rustdoc does not work properly

I am working on making x.py download and use the beta clippy. Currently it functions for the most part, however it does not work when trying to run on rustdoc, failing to find some rustc_X extern crates. I was advised by @jyn514 that you might be able to help, @Mark-Simulacrum

I noticed that the command being run does not contain the same --extern rustc_X=path that other commands seem to, I am unsure if that is the issue. More information will be provided in a comment.

Fixes #95988.

@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 26, 2022
@jyn514
Copy link
Member

jyn514 commented May 26, 2022

cc #95988

There are two possible ways to fix the rustdoc issue I can think of:

  • Use stage0-sysroot after all instead of the beta sysroot. I can't remember why we ended using the beta sysroot, can you remind me?
  • Pass --extern rustc_driver=/.../.rlib. This seems like a good idea regardless, actually? It would allow getting rid of the extern crate declarations in rustdoc (although not clippy itself, since it still wants to build out of tree).

@asquared31415
Copy link
Contributor Author

asquared31415 commented May 26, 2022

I can't remember why we ended using the beta sysroot, can you remind me?

The beta clippy needed the beta sysroot to run, it was otherwise complaining that the crates were compiled by different versions of the compiler

@asquared31415
Copy link
Contributor Author

Huh, I'm not sure what this CI error is

src/bootstrap/bootstrap.py Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 29, 2022

☔ The latest upstream changes (presumably #96687) made this pull request unmergeable. Please resolve the merge conflicts.

@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 18, 2022
@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jun 25, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#############################################                             63.4%
######################################################################## 100.0%
extracting /checkout/obj/build/cache/2022-05-20/cargo-beta-x86_64-unknown-linux-gnu.tar.xz
Traceback (most recent call last):
  File "../x.py", line 27, in <module>
    bootstrap.main()
  File "/checkout/src/bootstrap/bootstrap.py", line 1132, in main
    bootstrap(help_triggered)
  File "/checkout/src/bootstrap/bootstrap.py", line 1106, in bootstrap
    build.download_toolchain()
  File "/checkout/src/bootstrap/bootstrap.py", line 498, in download_toolchain
    self._download_component_helper(filename, "clippy-preview", tarball_suffix)
  File "/checkout/src/bootstrap/bootstrap.py", line 526, in _download_component_helper
    verbose=self.verbose,
  File "/checkout/src/bootstrap/bootstrap.py", line 76, in get
    .format(url))
RuntimeError: src/stage0.json doesn't contain a checksum for dist/2022-05-20/clippy-beta-x86_64-unknown-linux-gnu.tar.xz. Pre-built artifacts might not be available for this target at this time, see https://doc.rust-lang.org/nightly/rustc/platform-support.html for more information.

@jyn514
Copy link
Member

jyn514 commented Jun 25, 2022

Ok, I've spent some time today debugging this with @asquared31415. I don't think it can work with clippy's current sysroot detection logic.

  1. If we don't set SYSROOT, clippy falls back to the default sysroot in RUSTUP_HOME, which is wrong.
  2. If we set SYSROOT to stage0, clippy will be unable to compile rustdoc / other tools, because it can't find the rustc_private crates.
  3. If we set SYSROOT to stage0-sysroot, clippy will be unable to compile build scripts. This works ok with normal rustc because it goes through the rustc shim, but clippy actually ignores the shim altogether:
    // Setting RUSTC_WRAPPER causes Cargo to pass 'rustc' as the first argument.
    // We're invoking the compiler programmatically, so we ignore this/
    let wrapper_mode = orig_args.get(1).map(Path::new).and_then(Path::file_stem) == Some("rustc".as_ref());

We need to get clippy to allow one of the following things:

  1. Don't run the wrapper on build scripts at all, always fall back to our shim. This looks non-trivial to do because half the logic is in cargo itself.
  2. Allow setting a SYSROOT that only applies to build scripts and proc_macros (or alternatively, make the current SYSROOT env var only apply to things that aren't build scripts)

@rust-lang/clippy do you have a preference for 1 or 2? or an alternative suggestion for how to get this working?

@jyn514 jyn514 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 Jun 25, 2022
@bors
Copy link
Contributor

bors commented Jun 26, 2022

☔ The latest upstream changes (presumably #98521) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). A-clippy Area: Clippy and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2022
@Mark-Simulacrum
Copy link
Member

I think this is basically waiting on clippy, so marking as such. If you have a question you think I should dig into @jyn514 let me know, I can spend some time thinking about this if needed.

r? @jyn514 though

@jyn514
Copy link
Member

jyn514 commented Jul 3, 2022

@flip1995 came up with a suggestion in Zulip: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/.60x.2Epy.20clippy.60.20needs.20clippy.20changes

@jyn514 jyn514 removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jul 3, 2022
@Dylan-DPC
Copy link
Member

@asquared31415 any updates on this pr?

@jyn514
Copy link
Member

jyn514 commented Jan 18, 2023

I took this over in #106394 - @asquared31415 if you want to work on that just let me know :) but I'm also in comfortable being in charge of getting it merged myself.

@jyn514 jyn514 closed this Jan 18, 2023
@Dylan-DPC
Copy link
Member

Thanks for the update :)

@asquared31415 asquared31415 deleted the bootstrap branch June 22, 2023 01:37
@jyn514 jyn514 mentioned this pull request Nov 5, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 16, 2023
x clippy

thanks to `@asquared31415` `@albertlarsan68` for all their help, most of this pr is their work

note that this also adds x clippy --stage 0 -Awarnings to x86_64-gnu-llvm-15 to make sure it stays working; that won't gate on any clippy warnings, just enforce that clippy doesn't give a hard error. we can't add --stage 1 until clippy fixes its debug assertions not to panic.

note that `x clippy --stage 1` currently breaks when combined with download-rustc.

unlike the previous prs, this doesn't require changes to clippy (it works by using RUSTC_WRAPPER instead), and supports stage 0

read this commit-by-commit

closes rust-lang#107628; see also rust-lang#106394, rust-lang#97443. fixes rust-lang#95988. helps with rust-lang#76495.

r? bootstrap
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 16, 2023
x clippy

thanks to `@asquared31415` `@albertlarsan68` for all their help, most of this pr is their work

note that this also adds x clippy --stage 0 -Awarnings to x86_64-gnu-llvm-15 to make sure it stays working; that won't gate on any clippy warnings, just enforce that clippy doesn't give a hard error. we can't add --stage 1 until clippy fixes its debug assertions not to panic.

note that `x clippy --stage 1` currently breaks when combined with download-rustc.

unlike the previous prs, this doesn't require changes to clippy (it works by using RUSTC_WRAPPER instead), and supports stage 0

read this commit-by-commit

closes rust-lang#107628; see also rust-lang#106394, rust-lang#97443. fixes rust-lang#95988. helps with rust-lang#76495.

r? bootstrap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-clippy Area: Clippy 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix x.py clippy to be less janky
7 participants