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

Close the front door for clippy but open the back #7533

Merged
merged 4 commits into from Mar 14, 2020

Conversation

@yaahc
Copy link
Contributor

yaahc commented Oct 22, 2019

No description provided.

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Oct 22, 2019

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@yaahc

This comment has been minimized.

Copy link
Contributor Author

yaahc commented Oct 22, 2019

Feeling pretty good about this PR atm, the biggest problem right now is that I broke rustfix isolation, so now its fixing the non relevant crates. I think this is because I didn't scope the fixing to primary units like how I adjusted things in an earlier change, and rustfix probably had some separate hacky way of telling it to only fix primary crates, where as my last changes made it so we just invoke rustc rather than cargo rustc on non primary crates.

I'm also kinda confused on whether or not I should be using the term primary_crates or workspace_crates. We historically use primary but in the recent meeting we used workspace, so I defaulted with the one that aligned with the name we wanted to give the env variable to control the workspace rustc path.

To continue with the pattern we've established here I think we need a WORKSPACE_RUSTFLAGS to allow clippy to still pass args to the driver, originally I expected that the place for this would be in clippy as something like CLIPPYFLAGS or something but @Manishearth informs me that RUSTFLAGS is a cargo thing, so it seems like the sensible thing to do here is be consistent so I'll look into implementing that next.

@ehuss ehuss assigned ehuss and unassigned alexcrichton Oct 22, 2019
@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Oct 22, 2019

I'm also kinda confused on whether or not I should be using the term primary_crates or workspace_crates

Ah, sorry about being vague there. To be clear, "primary packages" are those selected on the command line. For example, cargo build -p foo -p bar, the primary packages are foo and bar, and nothing else.

I think that opens the question on how clippy should work. And maybe how cargo fix works as well, now that I'm looking at it closer (it ignores the target), although it probably isn't too important (nobody has ever complained). Fix should continue to only run against the primary package. As for clippy, I see a few options:

  • Run only against primary packages like fix does. This is how clippy-preview was working.
  • Run against all workspace packages. The reason I was suggesting this is because that is how normal lints work (when you build in a workspace, you get warnings for any workspace member). This is also how the stable cargo clippy works (although it runs on all dependencies, and then the lints are capped on non-workspace members).

So I guess the question boils down to, do you want to see clippy lints for dependencies in a workspace? I don't have a good answer. The primary package seems more useful (think of a workspace like rustc where there are dozens of dependencies). On the other hand, that is inconsistent with how normal lints work.

I have to leave for a while, but I'll try to do some more review here tonight.

@yaahc

This comment has been minimized.

Copy link
Contributor Author

yaahc commented Oct 22, 2019

I definitely prefer it to run against all workspace crates, but not necessarily path dependencies, which is something I think it currently does. I've had cases where I checked out a git repo for an unpublished crate and depended on it with a path dep and it surfaced clippy lints which was not what I wanted.

I'm not actually sure how cargo differentiates between these two cases. Ive only seen things talking about primary crates in the source, so I'll have to figure out how it deliniates crates that are in the workspace specifically.

I'll look into filtering by workspace members for clippy and move forward with the assumption that this is how we want to filter application of the alternate rustc unless I get comments to the contrary.

I'll make sure to keep fix on primaries and try to add some test cases that configure various forms of projects with path deps, workspace members, primary crates specified on the cli when called, etc.

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Oct 22, 2019

Ideally I think the answer would be "all workspace projects, unless you've specified a single one via -p", but "all workspace projects" is fine too.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Oct 23, 2019

all workspace crates, but not necessarily path dependencies

Path dependencies are automatically added as workspace members. This can be overridden by setting the workspace.exclude key to remove the package as a member, which would cap its lints.

how cargo differentiates between these two cases

Workspace members are computed in Workspace::find_members. The primary packages are computed here. The Units to compile is computed in two stages. The first generates the root units in generate_targets, which is where the "primary units" comes from (I would say "root" is synonymous with "primary"). The second phase is computing all dependencies from the roots in unit_dependencies.

Ideally I think the answer would be "all workspace projects, unless you've specified a single one via -p", but "all workspace projects" is fine too.

Cargo doesn't really have this kind of distinction. If you don't specify -p, there is an implied -p that is injected very early. I would also think the inconsistency might be surprising.

I think this is essentially rust-lang/rust-clippy#3025, right? I don't have any good answers on how to address this. Maybe we could set an env var that the clippy wrapper could detect (and the clippy wrapper could maybe have a --no-deps flag to restrict where it runs), but we recently rejected (or postponed) such a thing in #7205 (comment).

(Sorry, running out of energy for today, will try to look closer at the code tomorrow.)

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Oct 23, 2019

I don't think it's a big deal if we can't solve clippy#3025 right now; I'd rather get this to work for workspaces and figure that out later

Copy link
Contributor

ehuss left a comment

Seems pretty close. Just to be clear, I think the precedence of which rustc is used would be:

BuildConfig.primary_unit_rustc
RUSTC_WORKSPACE_WRAPPER
RUSTC_WRAPPER
RUSTC
rustc in path

Where primary_unit_rustc is only used by cargo fix to set cargo itself as the wrapper.

Also, we'll probably want to remove clippy from CI (azure-test-all.yml), as I think it will no longer be needed.

src/cargo/core/compiler/build_config.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/build_context/mod.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/compilation.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/context/compilation_files.rs Outdated Show resolved Hide resolved
src/cargo/ops/fix.rs Outdated Show resolved Hide resolved
src/cargo/util/config/mod.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 28, 2019

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

@yaahc yaahc force-pushed the yaahc:clippy-banishment branch from 97b2a4f to 175238d Nov 20, 2019
.filter(|wrapper| !wrapper.get_program().is_empty())
.map(|wrapper| {
let mut cmd = wrapper.clone();
// TODO: FIX HACK

This comment has been minimized.

Copy link
@yaahc

yaahc Nov 21, 2019

Author Contributor

This is where having the primary_unit_rustc exist as a wrapper becomes a problem. cargo fix doesn't really want to use the rustc_wrapper either, it's only supposed to run fix for primary units so it makes sense to invoke it via the primary wrapper, but when you do that it turns it into a wrapper by adding the path (rustc) as an arg, but for fix we already added the path to the wrapper so you end up with cargo clippy-driver rustc ... at the end. This check prevents that extra incorrect insert from happening but we need to find a better way to do this.

@yaahc yaahc marked this pull request as ready for review Nov 21, 2019
src/cargo/util/rustc.rs Outdated Show resolved Hide resolved
@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Nov 22, 2019

IIUC, this is moving in a slightly different direction where it is only running against primary units, not all workspace packages, correct? If that is true, calling it a "workspace wrapper" may not be entirely correct. I'm also not sure if that is what @Manishearth wanted (the message at #7533 (comment) seemed to imply all workspace crates is preferred). I'd also be a little concerned this is encroaching on the concerns from #7205 (comment), where "primary" isn't well established.

primary packages are strictly a subset of the workspace ones

Not really. -p can be specified for any package.

Whichever approach is taken, it would be good to have some tests for the new behavior.

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Nov 22, 2019

@yaahc

This comment has been minimized.

Copy link
Contributor Author

yaahc commented Nov 22, 2019

Okay, yea I wasn't sure if we decided that primary crates were close enough to workspace crates or if we wanted to intentionally aim at workspace crates. In that case I shall figure out an is_workspace_unit function to replace the is_primary and rename all instances of primary that I've added to workspace.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 25, 2019

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

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 16, 2019

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

@yaahc yaahc force-pushed the yaahc:clippy-banishment branch from 7caefed to 4d999a6 Dec 16, 2019
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 8, 2020

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

@yaahc yaahc force-pushed the yaahc:clippy-banishment branch from 4d999a6 to 4fc11a4 Jan 13, 2020
rustc-echo-wrapper Outdated Show resolved Hide resolved
tests/testsuite/check.rs Outdated Show resolved Hide resolved
tests/testsuite/check.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/mod.rs Outdated Show resolved Hide resolved
rustc-echo-wrapper Outdated Show resolved Hide resolved
tests/testsuite/check.rs Outdated Show resolved Hide resolved
tests/testsuite/check.rs Outdated Show resolved Hide resolved
@yaahc yaahc force-pushed the yaahc:clippy-banishment branch from cc44e98 to 1208667 Feb 11, 2020
@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Feb 12, 2020

So some general guidance:

cargo fix plus clippy

For the case of cargo fix with clippy, I think it would be easiest to keep things relatively the same as they are today. That is, the primary wrapper is:
/path/to/cargo /path/to/rustc …rest of args

And then inside ops::fix, it can catch the WORKSPACE_RUSTC_WRAPPER env var and execute WORKSPACE_RUSTC_WRAPPER /path/to/rustc …rest of args. The reason for doing that is FixArgs has some pretty simplistic arg parsing, and I wouldn't want to make it more complicated. This might require changing FixArgs::rustc to a ProcessBuilder or something like that just to keep it simple.

Otherwise, we would need to execute /path/to/cargo /path/to/clippy /path/to/rustc …rest of args, and that gets kinda tricky to extract the proper arguments.

cargo clippy plus sccache

With both RUSTC_WRAPPER and WORKSPACE_RUSTC_WRAPPER, I would expect the command to execute would be:
$RUSTC_WRAPPER $WORKSPACE_RUSTC_WRAPPER /path/to/rustc …

Because clippy is expecting to ignore the first argument. I would check to see if sccache handles this correctly. It seems to be doing the opposite right now, and I'm curious if there's a reason you picked that order?

Other notes

  • Can you change the env var name to RUSTC_WORKSPACE_WRAPPER per #7533 (review). I think it is better to keep a common prefix.
  • I forgot to mention that this should be placed behind an unstable flag. Add a flag to CliUnstable, look at the add method below to parse it. In load_global_rustc, check self.cli_unstable() for that flag when checking for the workspace wrapper.
@yaahc

This comment has been minimized.

Copy link
Contributor Author

yaahc commented Feb 12, 2020

@ehuss

the ordering was happenstance for how things just shook out with when process builders get insantiated and when the wrappers get applied. It was accidental and is the thing I need to fix, but your comment gives me ideas so I'm gonna go ahead and try to get it working RQ and ill msg you when im done / if I hit a wall.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 20, 2020

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

src/cargo/ops/fix.rs Outdated Show resolved Hide resolved
Copy link
Contributor

ehuss left a comment

Thanks! I think this is getting pretty close. Some notes:

  • There should be some kind of test in testsuite/cache_messages.rs that verifies when the workspace wrapper is set that it rebuilds compared to when it is not set (essentially the same as the old clippy test). The wrapper will just need to emit some extra output.
  • Windows build is broken.
  • The notes at the bottom of #7533 (comment) still need to be addressed.

Also, have you tested with sccache to see if it works with clippy?

src/cargo/ops/fix.rs Outdated Show resolved Hide resolved
src/cargo/util/process_builder.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/compilation.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/context/compilation_files.rs Outdated Show resolved Hide resolved
src/cargo/util/rustc.rs Outdated Show resolved Hide resolved
tests/testsuite/fix.rs Outdated Show resolved Hide resolved
src/cargo/ops/fix.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 12, 2020

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

@yaahc yaahc force-pushed the yaahc:clippy-banishment branch from c53f61a to b0351e4 Mar 13, 2020
yaahc added 3 commits Mar 13, 2020
@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Mar 14, 2020

OK! I think this looks good overall. Thanks so much for sticking with it @yaahc!

Do you want to work on the clippy side to enable this? I think there are two things to do:

  • With -Zunstable-options set, use RUSTC_WORKSPACE_WRAPPER here (if running on nightly channel).
  • Add a --fix flag to cargo-clippy that will change it to use "fix" here instead of "check". At least, that's how I think it should work, but of course converse with the clippy team what they think.

Unfortunately I don't see a way to check if clippy is on the nightly channel in the clippy codebase, but maybe it can access is_nightly_build directly from the rustc_session crate? I'm not too familiar with that.

There's no rush. The rust PR queue is somewhat backed up, so it may take a couple weeks for this to land on nightly.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 14, 2020

📌 Commit d9a77ce has been approved by ehuss

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 14, 2020

⌛️ Testing commit d9a77ce with merge 7302186...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 14, 2020

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 7302186 to master...

@bors bors merged commit 7302186 into rust-lang:master Mar 14, 2020
11 checks passed
11 checks passed
homu Test successful
Details
rust-lang.cargo Build #20200313.6 succeeded
Details
rust-lang.cargo (Linux beta) Linux beta succeeded
Details
rust-lang.cargo (Linux nightly) Linux nightly succeeded
Details
rust-lang.cargo (Linux stable) Linux stable succeeded
Details
rust-lang.cargo (Windows x86_64-msvc) Windows x86_64-msvc succeeded
Details
rust-lang.cargo (build_std) build_std succeeded
Details
rust-lang.cargo (docs) docs succeeded
Details
rust-lang.cargo (macOS) macOS succeeded
Details
rust-lang.cargo (resolver) resolver succeeded
Details
rust-lang.cargo (rustfmt) rustfmt succeeded
Details
@ehuss ehuss mentioned this pull request Mar 18, 2020
bors added a commit to rust-lang/rust that referenced this pull request Mar 18, 2020
Update cargo

Update cargo

21 commits in bda50510d1daf6e9c53ad6ccf603da6e0fa8103f..7019b3ed3d539db7429d10a343b69be8c426b576
2020-03-02 18:05:34 +0000 to 2020-03-17 21:02:00 +0000
- Run through clippy (rust-lang/cargo#8015)
- Fix config profiles using "dev" in `cargo test`. (rust-lang/cargo#8012)
- Run CI on all PRs. (rust-lang/cargo#8011)
- Add unit-graph JSON output. (rust-lang/cargo#7977)
- Split workspace/validate() into multiple functions (rust-lang/cargo#8008)
- Use Option::as_deref (rust-lang/cargo#8005)
- De-duplicate edges (rust-lang/cargo#7993)
- Revert "Disable preserving mtimes on archives" (rust-lang/cargo#7935)
- Close the front door for clippy but open the back (rust-lang/cargo#7533)
- Fix CHANGELOG.md typos (rust-lang/cargo#7999)
- Update changelog note about crate-versions flag. (rust-lang/cargo#7998)
- Bump to 0.45.0, update changelog (rust-lang/cargo#7997)
- Bump libgit2 dependencies (rust-lang/cargo#7996)
- Avoid buffering large amounts of rustc output. (rust-lang/cargo#7838)
- Add "Updating" status for git submodules. (rust-lang/cargo#7989)
- WorkspaceResolve: Use descriptive lifetime label. (rust-lang/cargo#7990)
- Support old html anchors in manifest chapter. (rust-lang/cargo#7983)
- Don't create hardlink for library test and integrations tests, fixing rust-lang/cargo#7960 (rust-lang/cargo#7965)
- Partially revert change to filter debug_assertions. (rust-lang/cargo#7970)
- Try to better handle restricted crate names. (rust-lang/cargo#7959)
- Fix bug with new feature resolver and required-features. (rust-lang/cargo#7962)
@ehuss ehuss mentioned this pull request Mar 29, 2020
bors added a commit that referenced this pull request Mar 30, 2020
Remove clippy tests.

Clippy was removed in #7533, but a stale file was left behind.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.