[wip] Fix `x test clippy --stage 0` #96798

jyn514 · 2022-05-07T01:37:04Z

This makes a lot of changes to bootstrap, and introduces some new requirements for clippy. The basic idea is that clippy cannot depend on the sysroot it is
built with to be the same as the sysroot it is run with. This assumption is hard-coded in lots
of places throughout clippy, which is why the changes are so extensive.

Change clippy to use the sysroot of the run compiler (stage1, not stage0-sysroot) when running tests. This fixes an issue where clippy would try to load libraries from the beta compiler, then complain about a version mismatch.
Change clippy to be able to find host macros. Previously it assumed macros and dependencies were stored in the same target directory, which is not true for stage 0 (macros are in release, but other dependencies are in release/x86_64-unknown-linux-gnu).
Don't build rustc twice. The naive way to get dependencies into the stage1/ sysroot is to just compile everything we were doing before with a later compiler, but that takes a very long while to run. Instead:
- Add a new crate in the clippy repo that has only the dependencies needed for UI tests. That allows compiling those crates without also compiling clippy and the whole compiler.
- Don't require clippy_utils to be present when running tests in rust-lang/rust. That crate is only used for dogfood lints, which aren't even run in-tree.
- Change various tests to remove usage of rustc_* crates. In all cases they were not testing the rustc crates themselves, they just happened to be conveniently in the sysroot.

This currently breaks --bless because clippy_dev requires rustc_lexer.
I hope to fix this shortly when the "build a single crate" PR lands (#95503); this PR is a WIP until then.

I'm opening this early to get feedback, particularly from the clippy team. This introduces two new maintenance burdens on clippy:

ui-test-dependencies/Cargo.toml has to be kept up to date. Right now this is only enforced in rust-lang/rust, but I can investigate enforcing it in rust-clippy if you think it will be a burden to only see errors on syncs.
UI tests can't both depend on rustc internals and run in rust-lang/rust. Either they need to use other dependencies (regex, alloc, etc) or they need to be disabled by switching on option_env!("RUSTC_TEST_SUITE").is_some().

Fixes #78717. Helps with #76495. Blocked on #95503 and #90244.

@rustbot label +A-rustbuild +A-clippy +S-blocked -S-waiting-on-review

rust-highfive · 2022-05-07T01:37:07Z

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

rust-highfive · 2022-05-07T01:37:08Z

r? @Mark-Simulacrum

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

flip1995

Thanks for working on this.

Keeping those toml files in sync shouldn't be too much of a hassle during the sync.
That's fine.

Some NIT picking about PR body wording:

Don't require clippy_lints to be present when running tests in rust-lang/rust. That crate is only used for dogfood lints, which aren't even run in-tree.

Should be:

Don't require ~~clippy_lints~~ clippy_utils to be present when running tests in rust-lang/rust. That crate is only used for ~~dogfood lints~~ internal UI tests, which aren't even run in-tree.

However, I would really like to run them in rustc to avoid major sync headaches.

flip1995 · 2022-05-07T09:35:36Z

src/tools/clippy/Cargo.toml

@@ -39,6 +39,7 @@ filetime = "0.2"
 rustc-workspace-hack = "1.0"

 # UI test dependencies
+ui-test-dependencies = { path = "ui-test-dependencies" }
 clippy_utils = { path = "clippy_utils" }


Does Clippy still need all of those dependencies in the top level Cargo.toml or is it enough to have them all in ui-test-dependencies?

Good callout :) I think it's enough to only have them in ui-test-dependencies? but there's a comment in compiletest that makes me wary:

// Test dependencies may need an `extern crate` here to ensure that they show up // in the depinfo file (otherwise cargo thinks they are unused) #[allow(unused_extern_crates)] extern crate derive_new;

I just tested it and with binary-dep-info they indeed need to stay in the top-level Cargo.toml

flip1995 · 2022-05-07T09:39:20Z

src/tools/clippy/ui-test-dependencies/Cargo.toml

+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+# clippy_utils = { path = "../clippy_utils" }


This crate is required if Clippy internal lints should be tested in the Rust repo. Those are just UI tests as well and unrelated to dogfood.

Those internal UI tests are currently not run, because the internal feature has to be enabled during testing. I have a patch to do this, because this caused headaches during the sync multiple times in the past.

That being said, I think being able to test Clippy with --stage 0 is more important for now.

Got it. I don't think we can run those lints without building the compiler twice, but we could add some way to opt-in to the internal tests so that they still run in CI (maybe something with --test-args?).

Worth looking into. But nothing to deal with in this PR.

src/tools/clippy/tests/compile-test.rs

src/bootstrap/test.rs

jyn514 · 2022-05-07T17:07:19Z

Building stage2 tool clippy-driver (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.18s
thread 'main' panicked at 'fs::read(stamp) failed with No such file or directory (os error 2) ("/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-rustc/x86_64-unknown-linux-gnu/release/.librustc.stamp")', src/bootstrap/lib.rs:1375:24
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This is coming from trying to uplift stage3 compiler artifacts. I had a feeling the stage numbering for clippy was weird - I think the proper fix is to make test --stage 0 clippy run the beta clippy, just like test --stage 0 ui. I can either work around it by passing test --stage 1 in CI or fixing #90244. @Mark-Simulacrum are you ok with test --stage 1 for clippy? Or should I work on a more permanent fix (either fixing the assemble step for stage 3, or changing the stage numbering)?

cc #92538, https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/Idea.20for.20explaining.20stages.20better/near/212230695

bors · 2022-05-08T00:58:01Z

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

This makes a lot of changes. The basic idea is that clippy cannot depend on the sysroot it is *built* with to be the same as the sysroot it is *run* with. This assumption is hard-coded in lots of places throughout clippy, which is why the changes are so extensive. - Change clippy to use the sysroot of the run compiler (stage1, not stage0-sysroot) when running tests. This fixes an issue where clippy would try to load libraries from the beta compiler, then complain about a version mismatch. - Change clippy to be able to find host macros. Previously it assumed macros and dependencies were stored in the same target directory, which is not true for stage 0 (macros are in `release`, but other dependencies are in `release/x86_64-unknown-linux-gnu`). - Don't build rustc twice. The naive way to get dependencies into the stage1/ sysroot is to just compile everything we were doing before with a later compiler, but that takes a very long while to run. Instead: - Add a new crate in the clippy repo that has only the dependencies needed for UI tests. That allows compiling those crates without also compiling clippy and the whole compiler. - Don't require `clippy_lints` to be present when running tests in rust-lang/rust. That crate is only used for dogfood lints, which aren't even run in-tree. - Change various tests to remove usage of `rustc_*` crates. In all cases they were not testing the rustc crates themselves, they just happened to be conveniently in the sysroot. This currently breaks `--bless` because clippy_dev requires `rustc_lexer`. I hope to fix this shortly when the "build a single crate" PR lands; this PR is a WIP until then.

rust-log-analyzer · 2022-06-18T12:39:12Z

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

  IMAGE: x86_64-gnu-tools
##[endgroup]
From https://github.com/rust-lang/rust
 * branch              master     -> FETCH_HEAD
Searching for toolstate changes between 0182fd99afaf4a3d602de6b88506edaf6043c125 and add69f2576b823894cc1829cfaccbd1d8d306554
Clippy or rustfmt subtrees were updated
##[group]Run src/ci/scripts/verify-channel.sh
src/ci/scripts/verify-channel.sh
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
---
Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
Building stage2 tool clippy-driver (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.24s
thread 'main' panicked at 'fs::read(stamp) failed with No such file or directory (os error 2) ("/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-rustc/x86_64-unknown-linux-gnu/release/.librustc.stamp")', src/bootstrap/lib.rs:1398:24
Assembling stage3 compiler (x86_64-unknown-linux-gnu)
Build completed unsuccessfully in 0:00:02

bors · 2022-06-19T03:34:41Z

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

RalfJung · 2022-08-31T13:56:33Z

I had a feeling the stage numbering for clippy was weird

I'd say it is perfectly consistent with this image -- "stage N clippy" depends on the rustc crate and hence is using "stage N compiler artifacts".

JohnCSimon · 2023-06-17T14:19:43Z

@jyn514

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

jyn514 · 2023-06-17T15:29:21Z

yeah that sounds good, this is blocked on #110854 anyway

rust-highfive assigned Mark-Simulacrum May 7, 2022

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

This comment has been minimized.

Sign in to view

jyn514 force-pushed the test-stage0-clippy branch 2 times, most recently from 64422f6 to 7342286 Compare May 7, 2022 01:41

jyn514 force-pushed the test-stage0-clippy branch from 7342286 to 680ec80 Compare May 7, 2022 03:05

This comment has been minimized.

Sign in to view

flip1995 reviewed May 7, 2022

View reviewed changes

jyn514 force-pushed the test-stage0-clippy branch from 680ec80 to f90eb62 Compare May 7, 2022 15:44

This comment has been minimized.

Sign in to view

This was referenced May 9, 2022

x.py panics when trying to build "stage3" compiler #90244

Closed

Fix spurious error when running build --stage 2 compiler/rustc #96859

Closed

jyn514 force-pushed the test-stage0-clippy branch from 0fa8c83 to f90eb62 Compare June 18, 2022 11:55

jyn514 added 2 commits June 18, 2022 07:05

Try removing UI test dependencies from clippy itself and see what breaks

8dcc51c

jyn514 force-pushed the test-stage0-clippy branch from f90eb62 to 8dcc51c Compare June 18, 2022 12:06

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jun 18, 2022

jyn514 mentioned this pull request Nov 5, 2022

Enable internal feature when testing Clippy #104007

Closed

jyn514 mentioned this pull request May 20, 2023

Improve the rustc-side clippy development experience #76495

Open

4 tasks

JohnCSimon closed this Jun 17, 2023

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jun 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] Fix `x test clippy --stage 0` #96798

[wip] Fix `x test clippy --stage 0` #96798

jyn514 commented May 7, 2022 •

edited

Loading

rust-highfive commented May 7, 2022

rust-highfive commented May 7, 2022

This comment has been minimized.

This comment has been minimized.

flip1995 left a comment •

edited

Loading

flip1995 May 7, 2022

jyn514 May 7, 2022

flip1995 May 12, 2022

flip1995 May 7, 2022

jyn514 May 7, 2022

flip1995 May 9, 2022

This comment has been minimized.

jyn514 commented May 7, 2022 •

edited

Loading

bors commented May 8, 2022

rust-log-analyzer commented Jun 18, 2022

bors commented Jun 19, 2022

RalfJung commented Aug 31, 2022 •

edited

Loading

JohnCSimon commented Jun 17, 2023

jyn514 commented Jun 17, 2023

[wip] Fix x test clippy --stage 0 #96798

[wip] Fix x test clippy --stage 0 #96798

Conversation

jyn514 commented May 7, 2022 • edited Loading

rust-highfive commented May 7, 2022

rust-highfive commented May 7, 2022

This comment has been minimized.

This comment has been minimized.

flip1995 left a comment • edited Loading

Choose a reason for hiding this comment

flip1995 May 7, 2022

Choose a reason for hiding this comment

jyn514 May 7, 2022

Choose a reason for hiding this comment

flip1995 May 12, 2022

Choose a reason for hiding this comment

flip1995 May 7, 2022

Choose a reason for hiding this comment

jyn514 May 7, 2022

Choose a reason for hiding this comment

flip1995 May 9, 2022

Choose a reason for hiding this comment

This comment has been minimized.

jyn514 commented May 7, 2022 • edited Loading

bors commented May 8, 2022

rust-log-analyzer commented Jun 18, 2022

bors commented Jun 19, 2022

RalfJung commented Aug 31, 2022 • edited Loading

JohnCSimon commented Jun 17, 2023

jyn514 commented Jun 17, 2023

[wip] Fix `x test clippy --stage 0` #96798

[wip] Fix `x test clippy --stage 0` #96798

jyn514 commented May 7, 2022 •

edited

Loading

flip1995 left a comment •

edited

Loading

jyn514 commented May 7, 2022 •

edited

Loading

RalfJung commented Aug 31, 2022 •

edited

Loading