Run cargo shear in CI instead of cargo udeps#9638
Conversation
19be1f7 to
7b14e6e
Compare
7b14e6e to
dcfd1e1
Compare
hulthe
left a comment
There was a problem hiding this comment.
@hulthe reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern).
.github/workflows/rust-unused-dependencies.yml line 9 at r1 (raw file):
- '**/*.rs' - '**/Cargo.toml' - 'building/*-container-image.txt'
Doesn't look like it needs to trigger on the container images anymore
hulthe
left a comment
There was a problem hiding this comment.
@hulthe made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faern).
.github/workflows/rust-unused-dependencies.yml line 25 at r1 (raw file):
# By checking out the repo after installing cargo-shear, we will install shear # with the runners default Rust version, and not the one specified in the toolchain file.
But shear is installed using binstall, so there's no rust compiler involved?
ab40308 to
0d7f938
Compare
faern
left a comment
There was a problem hiding this comment.
@faern made 2 comments.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @hulthe).
.github/workflows/rust-unused-dependencies.yml line 9 at r1 (raw file):
Previously, hulthe (Joakim Hulthe) wrote…
Doesn't look like it needs to trigger on the container images anymore
Good catch! Fixed
.github/workflows/rust-unused-dependencies.yml line 25 at r1 (raw file):
Previously, hulthe (Joakim Hulthe) wrote…
But shear is installed using binstall, so there's no rust compiler involved?
Yeah. But I observed that it does download and install Rust 1.91 when installing even via binstall 🤷 As you can see here: https://github.com/mullvad/mullvadvpn-app/actions/runs/21023922140/job/60443936683?pr=9638
From this run, where we check out the repo last, the actual cargo shear command still downloads and installs whatever toolchain we have in rust-toolchain.toml so it's probably unavoidable to pay for that installation time. https://github.com/mullvad/mullvadvpn-app/actions/runs/20997986291/job/60359806372?pr=9638
Given that it seems unavoidable to download the toolchain we have specified in rust-toolchain.toml I guess the order does not matter much. We want to specify whatever makes the job run the most stable and with the best output. I moved the checkout to the first step, just as in most other workflows.
hulthe
left a comment
There was a problem hiding this comment.
@hulthe reviewed 1 file and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved.
.github/workflows/rust-unused-dependencies.yml line 25 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Yeah. But I observed that it does download and install Rust 1.91 when installing even via binstall 🤷 As you can see here: https://github.com/mullvad/mullvadvpn-app/actions/runs/21023922140/job/60443936683?pr=9638
From this run, where we check out the repo last, the actual
cargo shearcommand still downloads and installs whatever toolchain we have inrust-toolchain.tomlso it's probably unavoidable to pay for that installation time. https://github.com/mullvad/mullvadvpn-app/actions/runs/20997986291/job/60359806372?pr=9638Given that it seems unavoidable to download the toolchain we have specified in
rust-toolchain.tomlI guess the order does not matter much. We want to specify whatever makes the job run the most stable and with the best output. I moved the checkout to the first step, just as in most other workflows.
Ah, I see. I suppose cargo itself downloads rust whenever it is invoked, regardless of whether the shear or binstall subcommands need it.
This runs much quicker and finds many more issues
0d7f938 to
0f7ddaf
Compare
hulthe
left a comment
There was a problem hiding this comment.
@hulthe reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved.
cargo shearhas helped identify a bunch of unused dependencies and other tiny problems in the last day. This PR suggests to run it in CI to help battle dependency-rot over time. Here I replace cargo udeps, since they solve basically the same problem but seems to do it way better(?). I cannot think of a single time in the last few years wherecargo udepsactually helped us find an unused dependency. Cargo udeps also takes ages to run (20 min on Windows sometimes!!), since it has to compile the code. In contrastcargo shearis very fast.Is it enough to run
cargo shearon a single platform? Let's see what this issue says: Boshen/cargo-shear#404. Result: As long as we don't use the--expandflag, the platform the tool runs on should not matter for the result, as it does not invoke any compilation, just analyze the source code and metadata.If we have to run it on multiple platforms we currently have the problem that the latest release does not really work on Windows. It gives a huge amount of false positives. This is fixed on theirmain, but not yet released: Boshen/cargo-shear#386This change is