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.

Sign up for GitHub

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

Jump to bottom

x.py clippy: don't run with --all-targets by default #88511

Merged
merged 1 commit into from
Sep 5, 2021
Merged

x.py clippy: don't run with --all-targets by default #88511

merged 1 commit into from
Sep 5, 2021

Conversation

matthiaskrgr
Copy link
Member

this caused a lot of noise because benchmarks and tests were also checked

before:
./x.py clippy |& wc -l
74026

with change:
./x.py clippy |& wc -l
43269

Zulip thread: https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/x.2Epy.20check.20default.20targets

r? @jyn514 or @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2021
@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Aug 30, 2021
jyn514
jyn514 reviewed Aug 30, 2021
View reviewed changes
src/bootstrap/check.rs Outdated
Comment on lines 123 to 128
if builder.kind != Kind::Clippy {
cargo.arg("--all-targets");
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this makes it so it's no longer possible to pass --all-targets to clippy - is that intentional? I agree it doesn't seem super useful, but it seems weird to remove the option altogether ... although I guess you can still add it back with CARGOFLAGS=--all-targets.

Copy link
Member

Choose a reason for hiding this comment

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

Also, for this step specifically it should just be an early return above, because we've already run on the standard library; this invocation is only for the other targets. The other steps are correct though.

Copy link
Member Author

Choose a reason for hiding this comment

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

How did that work previously? When I check ./x.py clippy --all-targets pre-branch, I just get a

Unrecognized option: 'all-targets'

Usage: x.py <subcommand> [options] [<paths>...]

error...

Copy link
Member

Choose a reason for hiding this comment

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

I think at least a comment as to why this isn't being specified is a good idea. We can consider the defaults (or ability to provide it) later.

Copy link
Member

Choose a reason for hiding this comment

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

How did that work previously?

See the changes in https://github.com/rust-lang/rust/pull/88011/files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, added some comments.

@jyn514 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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 30, 2021
@matthiaskrgr matthiaskrgr force-pushed the xpyclippydefaulttarget branch from 55ef046 to 75ed79a Compare August 30, 2021 21:53
jyn514
jyn514 suggested changes Aug 30, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

r=me with https://github.com/rust-lang/rust/pull/88511/files#r698829310 done.

src/bootstrap/check.rs Outdated Show resolved Hide resolved
@matthiaskrgr matthiaskrgr force-pushed the xpyclippydefaulttarget branch from 75ed79a to 732f700 Compare August 30, 2021 22:14
@jyn514
Copy link
Member

jyn514 commented Aug 31, 2021

@matthiaskrgr did you miss https://github.com/rust-lang/rust/pull/88511/files#r698829310 ?

@matthiaskrgr
Copy link
Member Author

Ah yes, I missed that.
I excluded an early return to run() of Std, I hope that's what you ment`:

        if builder.kind != Kind::Clippy {
            return;
        }

@matthiaskrgr matthiaskrgr force-pushed the xpyclippydefaulttarget branch from 732f700 to e7c15b9 Compare August 31, 2021 16:45
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@matthiaskrgr
Copy link
Member Author

Ok that does not seem to work... 😅

@jyn514
Copy link
Member

jyn514 commented Aug 31, 2021

@matthiaskrgr I meant here: https://github.com/rust-lang/rust/blob/e7c15b95174e5bc78b51a7887ac9e6db3468d65b/src/bootstrap/check.rs#L114

The idea is to avoid running cargo twice, not to skip it altogether.

@jyn514
Copy link
Member

jyn514 commented Aug 31, 2021

(or in other words, your "handled elsewhere" comment is incorrect)

@matthiaskrgr
x.py clippy: don't run with --all-targets by default
5538be5 
this caused a lot of noise because benchmarks and tests were also checked
@matthiaskrgr matthiaskrgr force-pushed the xpyclippydefaulttarget branch from e7c15b9 to 5538be5 Compare August 31, 2021 20:56
@matthiaskrgr
Copy link
Member Author

@jyn514 I think this is ready to be reviewed again..? :)

@jyn514
Copy link
Member

jyn514 commented Sep 5, 2021

Oh sorry, didn't realize this was waiting on me. Could you switch the labels next time?

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 5, 2021

📌 Commit 5538be5 has been approved by jyn514

@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 Sep 5, 2021
@m-ou-se m-ou-se mentioned this pull request Sep 5, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2021
@bors
Auto merge of rust-lang#88662 - m-ou-se:rollup-h6lgp7k, r=m-ou-se
7e1e3eb 
Rollup of 4 pull requests

Successful merges:

 - rust-lang#88257 (Provide more context on incorrect inner attribute)
 - rust-lang#88432 (Fix a typo in raw_vec)
 - rust-lang#88511 (x.py clippy: don't run with --all-targets by default)
 - rust-lang#88657 (Fix 2021 `dyn` suggestion that used code as label)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5fdc8eb into rust-lang:master Sep 5, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@jyn514 jyn514 jyn514 requested changes

Assignees

@jyn514 jyn514

Labels
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)
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.
7 participants
@matthiaskrgr @jyn514 @rust-log-analyzer @bors @Mark-Simulacrum @rust-highfive @rustbot