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

chore(ci): add cargo machete #5323

Merged

Conversation

SWvheerden
Copy link
Collaborator

@SWvheerden SWvheerden commented Apr 13, 2023

Description

Adds cargo machete to check for dependencies

Motivation and Context

Helps keep the dependencies clean

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Apr 13, 2023
@CjS77
Copy link
Collaborator

CjS77 commented Apr 13, 2023

Does this action just report, or actually change Cargo.toml?

If former, will it cause a PR to fail checks if there are unused dependencies?

It adds ±2.5 minutes to CI; this is something we could also run nightly as long as it reports properly / creates an issue.

@SWvheerden
Copy link
Collaborator Author

This just runs a check on the cargo.toml files in the repo.
But it does fail the ci
Here is a run of me testing it: https://github.com/tari-project/tari/actions/runs/4689245804/jobs/8310763738

I prefer it running on the pr itself and making sure the offending PR fixes its cargo file. Machete can give false positives, and you need to fix those with exceptions in the cargo files. I prefer the users checking those first before just fixing them.

As to the time, I can add it directly to the clippy fix, that might be much quicker run, but will fail as part of clippy and not on its own?

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Apr 18, 2023
@stringhandler stringhandler merged commit f9ff526 into tari-project:development Apr 18, 2023
@CjS77
Copy link
Collaborator

CjS77 commented Apr 18, 2023

CON: Since unused dependencies aren't a major issue, it could become annoying to do a push-commit-test cycle to GH for every PR (I have had several complaints about this for clippy & cargo fmt already). Do we really want to add more friction, vs run nightly and (potentially even) automatically create a new issue / new PR (GHP could do this)

PRO: On the plus side, I guess we don't add deps that often, so maybe it doesn't come up that much

tl;dr - If you include it in the clippy test, I think we can go ahead

@stringhandler
Copy link
Collaborator

IMO it actually could save on build time because we are not downloading and building unnecessary crates

@SWvheerden SWvheerden deleted the sw_add_cargo_machete branch May 3, 2023 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants