Skip to content

x clippy #117595

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 6 commits into from
Dec 16, 2023
Merged

x clippy #117595

merged 6 commits into from
Dec 16, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Nov 4, 2023 •
edited
Loading

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 #107628; see also #106394, #97443. fixes #95988. helps with #76495.

r? bootstrap

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue. labels Nov 4, 2023
jyn514
jyn514 commented Nov 4, 2023
View reviewed changes
@rust-log-analyzer

This comment has been minimized.

# to view
@matthiaskrgr
Copy link
Member

./x.py --stage 1 clippy

Does this build clippy from the repo to then run that clippy on rustc again?
I'm trying to figure out if we still have a way to use an up to date clippy on rustc without having to rebuild everything after each change to rustc/std sources with this pr.
I'm using that regularly to dogfood clippy against rustc after clippy updates.

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023

Does this build clippy from the repo to then run that clippy on rustc again?

yes. --stage 0 is the version that doesn't build from source.

@matthiaskrgr
Copy link
Member

does stage0 always use "beta" or "whatever default toolchain is"?

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023

"beta" (the bootstrap toolchain, sometimes stable)

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023

if you want to test with a custom clippy without building rustc from source you can set build.rustc = "/path/to/custom/clippy/toolchain"

@rust-log-analyzer

This comment has been minimized.

# to view
@matthiaskrgr
Copy link
Member

I did some cursed stuff over at rust-lang/rust-clippy#11235 where I built clippy from the clippy repo, copy that one into the rustup-installed toolchain and then run x.py clippy on rustc to see if the upstream-clippy would crash at some point when checking rustc (for the clippy ci)

I'm curious to see if this will still work wrt this change, when I set the build.rustc to the rustc toolchain that clippy is pinned to, but I think it might...? :D
Anyway, that's something for another day...

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023 •
edited
Loading

I'm curious to see if this will still work wrt this change, when I set the build.rustc to the rustc toolchain that clippy is pinned to, but I think it might...? :D

it should, yes, but i haven't tested

@matthiaskrgr
Copy link
Member

When runnning x.py check it looks like we run with --all-targets(build and tests) do you think it makes sense to actually inherit that for x.py clippy?
I have not looked at the output of running clippy on the in-source tests yet, but I suspect some of the findings may be silly especially around std and core

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023 •
edited
Loading

When runnning x.py check it looks like we run with --all-targets(build and tests) do you think it makes sense to actually inherit that for x.py clippy?

that is what x.py clippy does today yes

i don't want to be the one who makes that decision

@rust-log-analyzer

This comment has been minimized.

# to view
@matthiaskrgr
Copy link
Member

Ah right makes sense that we would also run into some of the issues listed here rust-lang/rust-clippy#11235 (comment) 🥲

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023

yeah i'm gonna remove the stage 1 test until rust-lang/rust-clippy#11230 is fixed
stage 0 has debug assertions disabled so it should be ok to keep

@jyn514 jyn514 force-pushed the x-clippy branch 2 times, most recently from 0b807e2 to 1012698 Compare November 5, 2023 01:09
@rust-log-analyzer

This comment has been minimized.

# to view
@rust-log-analyzer

This comment has been minimized.

# to view
@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 Dec 9, 2023
@rust-log-analyzer

This comment has been minimized.

# to view
jyn514
jyn514 commented Dec 9, 2023
View reviewed changes
src/bootstrap/src/core/builder.rs Outdated
Comment on lines 1159 to 1162
// Set PATH to include the sysroot bin dir so clippy can find cargo.
let path = t!(env::join_paths(
// The sysroot comes first in PATH to avoid using rustup's cargo.
std::iter::once(PathBuf::from(initial_sysroot_bin))
.chain(env::split_paths(&t!(env::var("PATH"))))
Copy link
Member Author

Choose a reason for hiding this comment

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

cc rust-lang/rust-clippy#11943

@bors
Copy link
Collaborator

bors commented Dec 10, 2023

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

@jyn514
Copy link
Member Author

jyn514 commented Dec 10, 2023

won't have a chance to fix the conflicts for at least a week - would appreciate a review of the new changes before i do that if it's not too much trouble

@albertlarsan68
Copy link
Member

LGTM. Can you clean up the commits?
r=me once done.

@albertlarsan68 albertlarsan68 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 Dec 16, 2023
@saethlin
Copy link
Member

Commits look pretty clean to me
@bors r=albertlarsan68

@bors
Copy link
Collaborator

bors commented Dec 16, 2023

📌 Commit a078c3a has been approved by albertlarsan68

It is now in the queue for this repository.

@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 Dec 16, 2023
@bors
Copy link
Collaborator

bors commented Dec 16, 2023

⌛ Testing commit a078c3a with merge 4451777...

@bors
Copy link
Collaborator

bors commented Dec 16, 2023

☀️ Test successful - checks-actions
Approved by: albertlarsan68
Pushing 4451777 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 16, 2023
@bors bors merged commit 4451777 into rust-lang:master Dec 16, 2023
@rustbot rustbot added this to the 1.76.0 milestone Dec 16, 2023
@jyn514 jyn514 deleted the x-clippy branch December 16, 2023 22:59
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4451777): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 1.6% [0.9%, 3.0%] 4
Regressions ❌
(secondary)		 1.9% [1.0%, 2.4%] 3
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.6% [0.9%, 3.0%] 4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)		 -0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.5% [-0.5%, -0.5%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.679s -> 672.517s (-0.02%)
Artifact size: 312.39 MiB -> 312.37 MiB (-0.01%)

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@matthiaskrgr matthiaskrgr matthiaskrgr left review comments

@the8472 the8472 the8472 left review comments

@albertlarsan68 albertlarsan68 albertlarsan68 approved these changes

Assignees

@albertlarsan68 albertlarsan68

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.

Fix x.py clippy to be less janky