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

Add --no-deps option to avoid running on path dependencies in workspaces #6188

Merged
merged 5 commits into from
Dec 9, 2020
Merged

Add --no-deps option to avoid running on path dependencies in workspaces #6188

merged 5 commits into from
Dec 9, 2020

Conversation

ebroto
Copy link
Member

@ebroto ebroto commented Oct 17, 2020 •
edited
Loading

Since rust-lang/cargo#8758 has hit nightly, this allows us to address the second bullet point and the concern related to --fix in the RUSTC_WORKSPACE_WRAPPER tracking issue.

As a reminder stabilizing that env var will solve #4612 (Clippy not running after cargo check in stable) and would allow to stabilize the --fix option in Clippy.

changelog: Add --no-deps option to avoid running on path dependencies in workspaces

Fixes #3025

@rust-highfive
Copy link

r? @Manishearth

(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 Oct 17, 2020
@ehuss
Copy link
Contributor

ehuss commented Oct 22, 2020

Thanks for moving this forward!

One more thing, I think when running cargo clippy --fix it should also probably imply --no-deps as well, since it can cause problems with how fix works. I'm not sure how easy that would be to wire in, or if it would have any problems, though.

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 22, 2020

📌 Commit 5f51eed has been approved by Manishearth

bors added a commit that referenced this pull request Oct 22, 2020
@bors
Auto merge of #6188 - ebroto:primary_package, r=Manishearth
5139104 
Add --no-deps option to avoid running on path dependencies in workspaces

Since rust-lang/cargo#8758 has hit nightly, this allows us to address the second bullet point and [the concern related to `--fix`](rust-lang/cargo#8143 (comment)) in the [RUSTC_WORKSPACE_WRAPPER tracking issue](rust-lang/cargo#8143).

As a reminder stabilizing that env var will solve #4612 (Clippy not running after `cargo check` in stable) and would allow to stabilize the `--fix` option in Clippy.

changelog: Add `--no-deps` option to avoid running on path dependencies in workspaces

Fixes #3025
@bors
Copy link
Contributor

bors commented Oct 22, 2020

⌛ Testing commit 5f51eed with merge 5139104...

@ebroto
Copy link
Member Author

ebroto commented Oct 22, 2020

Oops I think @ehuss comment is relevant, should I open a followup?

@ebroto

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Oct 22, 2020

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 5139104 (5139104ada0b418dcd596e11ad3f3ff9aa9fa136)

@ebroto ebroto added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 24, 2020
flip1995
flip1995 reviewed Oct 25, 2020
View reviewed changes
src/driver.rs Outdated Show resolved Hide resolved
@Manishearth

This comment has been minimized.

Sign in to view
@flip1995

This comment has been minimized.

Sign in to view
@Manishearth

This comment has been minimized.

Sign in to view
@ebroto ebroto mentioned this pull request Oct 27, 2020
Closed
@ebroto ebroto force-pushed the primary_package branch 2 times, most recently from 95e4028 to 506ae4f Compare November 27, 2020 16:02
@ebroto
Copy link
Member Author

ebroto commented Nov 27, 2020

Sorry to everyone involved for the delay in making this move forward, I didn't have much free time lately and wanted to give priority to some pending reviews.

@flip1995 I've addressed your concern, code is much clearer now, thanks!

@ehuss now --fix implies --no-deps. Thanks for your suggestion!

Regarding the RUSTC_WORKSPACE_WRAPPER tracking issue, I have prepared another PR to make Clippy use RUSTFLAGS for their own arguments so changing those results in a rebuild instead of reusing cached results. It removes a hack (CLIPPY_HACKERY) to add another one, but I think it's better to solve the usability problem. We can solve that in the future (maybe by making cargo take into account another env var to invalidate cached results).

After that, I'm not sure if there would be something else missing to stabilize RUSTC_WORKSPACE_WRAPPER. Maybe the ccache issue, not sure if that would be a blocker? I would be happy to work on that if it is :)

@ebroto

This comment has been minimized.

Sign in to view
flip1995
flip1995 reviewed Nov 27, 2020
View reviewed changes
src/driver.rs Outdated Show resolved Hide resolved
tests/dogfood.rs Show resolved Hide resolved
flip1995
flip1995 approved these changes Nov 27, 2020
View reviewed changes
Copy link
Member

@flip1995 flip1995 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. Don't know if other people want to take a look before merging.

@ebroto
Copy link
Member Author

ebroto commented Dec 9, 2020

I will go ahead and @bors r=flip1995 this. We can always add further changes in a follow-up :)

@bors
Copy link
Contributor

bors commented Dec 9, 2020

📌 Commit 952b731 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Dec 9, 2020

⌛ Testing commit 952b731 with merge a2d9925...

@bors
Copy link
Contributor

bors commented Dec 9, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing a2d9925 to master...

@bors bors merged commit a2d9925 into rust-lang:master Dec 9, 2020
@ebroto ebroto deleted the primary_package branch December 9, 2020 16:08
fpoli added a commit to viperproject/prusti-dev that referenced this pull request Dec 10, 2020
@fpoli
Use CARGO_PRIMARY_PACKAGE to skip dependencies
c3aa699 
See also rust-lang/cargo#8758 and rust-lang/rust-clippy#6188
@fpoli fpoli mentioned this pull request Dec 10, 2020
Merged
fpoli added a commit to viperproject/prusti-dev that referenced this pull request Dec 14, 2020
@fpoli
Use CARGO_PRIMARY_PACKAGE to skip dependencies
e5a4477 
See also rust-lang/cargo#8758 and rust-lang/rust-clippy#6188
@ehuss ehuss mentioned this pull request Dec 14, 2020
Merged
@mleonhard
Copy link

This is super useful. Thank you!

@mleonhard mleonhard mentioned this pull request Feb 26, 2021
Closed
@fpoli fpoli mentioned this pull request Oct 19, 2021
Closed
@Mo-Fatah Mo-Fatah mentioned this pull request Apr 20, 2022
Closed
@EFanZh EFanZh mentioned this pull request Aug 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 approved these changes

Assignees

@flip1995 flip1995

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Limit to specific package in workspace, also excluding its dependencies
7 participants
@ebroto @rust-highfive @ehuss @Manishearth @bors @flip1995 @mleonhard