Lint all Rust projects in pre-commit
ClosedPublic
Actions

Authored by bartek on Jan 19 2023, 9:00 AM.

Details

Reviewers

varun
• jon
ashoat

Commits

rCOMMacfb4e0126e8: Lint all Rust projects in pre-commit

Summary

Resolves ENG-2314.

Added a get_cargo_paths.js - it contains a helper function that looks for Cargo project directory for given .rs file path.
Modified .lintstaged.js to collect a set of modified cargo projects and pass this list to rust_pre_commit.sh script.
Modified rust_pre_commit.sh script to accept Cargo project paths as command line arguments.

This way, cargo fmt + cargo check is run once per modified cargo project.

Test Plan

# do some changes in any .rs file e.g. in 2+ services/ projects 
# lint should run once per cargo project
# it's best to do some incorrect formatting
git add .

# or git commit, command below runs only the hook, without commiting
lint-staged

# linter should fail on your formatting

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Jan 19 2023, 9:00 AM

bartek held this revision as a draft.

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptJan 19 2023, 9:00 AM

Renamed plural get_cargo_paths.js -> singular get_cargo_path.js

Harbormaster completed remote builds in B15462: Diff 21084.Jan 19 2023, 9:14 AM

bartek added inline comments.Jan 19 2023, 9:19 AM

scripts/rust_pre_commit.sh
8–9 ↗	(On Diff #21085)	I assume that paths won't contain spaces, but this assumption may be wrong? Using `"$@"` triggers shellcheck SC2124. What's the correct way of iterating over these paths?
20–23 ↗	(On Diff #21085)	Should I run `cargo test` too?

Harbormaster completed remote builds in B15463: Diff 21085.Jan 19 2023, 9:27 AM

bartek published this revision for review.Jan 19 2023, 9:27 AM

ashoat added inline comments.Jan 19 2023, 9:45 AM

scripts/get_cargo_path.js
3 ↗	(On Diff #21085)	We shouldn't rely on a transitive dependency to have this library... can you make it explicit by adding to the root `package.json`?

• jon added inline comments.Jan 19 2023, 10:59 AM

scripts/rust_pre_commit.sh
8–18 ↗	(On Diff #21085)	This is how I would do it.
20–23 ↗	(On Diff #21085)	We should probably fix the build of everything first. I think the old blob client doesn't build right now.

bartek added inline comments.Jan 19 2023, 12:10 PM

scripts/get_cargo_path.js
3 ↗	(On Diff #21085)	Yes, I'll do it
scripts/rust_pre_commit.sh
20–23 ↗	(On Diff #21085)	Sounds reasonable. Old blob client is about to be removed in D6312. But I found another blocker - tests in `commtest` shouldn't be run without special configuration so they'd always fail when linted this way.