tools: add linter to validate Rust dependencies#63284
Conversation
Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
|
Review requested:
|
| exit "$EXIT_CODE" | ||
| fi | ||
|
|
||
| lint-rust: |
There was a problem hiding this comment.
This does not lint rust sources. A better name would be lint-crates.
| lint-rust: | |
| lint-crates: |
|
This only really comes up at the time of a V8 update, right? Would it make more sense just to add this step to the V8 upgrade checklist, rather than perpetually checking with every commit? I think we are going to have to look at upgrading to temporal_rs 0.2.x before v26.x hits LTS, if the build environment allows (there are a lot of conformance updates for the stage 4 spec), but we can't update the crates in alignment with the equivalent upstream DEPS update (chromium/chromium@387bc69) because 0.2.0 has been yanked from crates.io, so this script would potentially block such a move. |
If the run time is pretty short (which it seems to be), I don't think it's necessarily bad to run on every commit.
Do you think this kind of issue will be repeating or one-off? If one-off, we could just hold off on merging until this is resolved or we could merge with the acceptance that there'll be some wonkiness around that while it gets resolved. |
It's not very expensive in machine time indeed, on the other hand it's not exactly light-weight (downloads 165.3 MiB of software + clones Google's repo) for something that will so rarely change, so I get René's point. There would be solutions to avoid that (e.g. move this to its own workflow with trigger only for the specific files, and/or leverage Nix capabilities to cache result on Cachix), but I feel they all come at a price of making this less straightforward to maintain. My goal with this PR was to not waste that script as manually check those deps is error prone, but as long as this PR is enough to let folks approve #63281, I guess that goal is fulfilled. That being said, the fact that we didn't detect the drift is probably indicative some automation is in order. |
It probably doesn't really make sense to run this on all PRs (as opposed to e.g. only those who touch V8 and/or the crates), but it's true for other linters as well so 🤷
I expect this to be red until #63281 is merged