Skip to content

Commit 94c13cc

Browse files
zkochanzkochan
authored
ci: run clippy as a single-OS job and add it to the pre-push hook (#12389)
* fix(pnpr): pass batch_publish test request bodies by reference The put_json/put_json_with_token test helpers took the JSON body by value but only borrowed it for serde_json::to_vec, tripping clippy's needless_pass_by_value under --all-targets. Take &Value instead, which also drops an unnecessary body.clone() at one call site. * ci: run clippy as a single-OS job and add it to the pre-push hook Clippy was a step inside the three-OS Lint-and-Test matrix, so it ran once per OS even though it lints the same platform-independent source each time. Move it to its own ubuntu-only job, mirroring the existing single-OS doc, format, and dylint jobs (platform-gated cfg blocks are still type-checked per-OS by the test build). It was also missing from pacquet/scripts/pre-push-rust.sh, so a clippy lint that only fires under --all-targets — like the one that just reached main — slipped past local pushes and surfaced only in CI. Add the same --all-targets workspace clippy gate to the hook.
1 parent f1521cf commit 94c13cc

4 files changed

Lines changed: 69 additions & 27 deletions

File tree

‎.github/workflows/pacquet-ci.yml‎

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,11 @@ jobs:
7070
with:
7171
persist-credentials: false
7272

73-
# The clippy --all-targets pass plus the nextest build hold two
74-
# full sets of debug artifacts in target/, which no longer fits
75-
# in the hosted runner's free disk — the linker dies with ENOSPC
76-
# / SIGBUS mid-build. Drop the preinstalled toolchains this job
77-
# never uses (~25 GB) before compiling anything.
73+
# The nextest build holds a full set of debug artifacts in
74+
# target/, which no longer fits in the hosted runner's free disk —
75+
# the linker dies with ENOSPC / SIGBUS mid-build. Drop the
76+
# preinstalled toolchains this job never uses (~25 GB) before
77+
# compiling anything.
7878
- name: Free up runner disk space
7979
if: runner.os == 'Linux'
8080
run: |
@@ -84,7 +84,6 @@ jobs:
8484
- name: Install Rust Toolchain
8585
uses: ./.github/actions/rustup
8686
with:
87-
clippy: true
8887
save-cache: ${{ github.ref_name == 'main' }}
8988

9089
- name: Install pnpm (for compatibility check)
@@ -105,9 +104,6 @@ jobs:
105104
timeout-minutes: 1
106105
continue-on-error: true
107106

108-
- name: Clippy
109-
run: cargo clippy --locked --workspace --all-targets -- -D warnings
110-
111107
- name: Install just
112108
uses: taiki-e/install-action@873c7452cadb7c034694a1282227095d93fbdf92 # v2.79.14
113109
with:
@@ -131,6 +127,35 @@ jobs:
131127
132128
just test
133129
130+
clippy:
131+
name: Clippy
132+
# Clippy lints the same source on every platform, so one OS is
133+
# enough — running the full `--all-targets` workspace lint across the
134+
# whole test matrix just triples the cost. Mirrors the single-OS doc,
135+
# format, and dylint jobs. (Platform-gated `#[cfg]` blocks are still
136+
# type-checked per-OS by the test job's build.)
137+
runs-on: ubuntu-latest
138+
steps:
139+
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
140+
with:
141+
persist-credentials: false
142+
143+
# `clippy --all-targets` over the whole workspace holds a full set
144+
# of debug artifacts in target/; drop the preinstalled toolchains
145+
# this job never uses (~25 GB) so the linker doesn't hit ENOSPC.
146+
- name: Free up runner disk space
147+
run: |
148+
sudo rm -rf /usr/share/dotnet /usr/local/lib/android /opt/ghc /opt/hostedtoolcache/CodeQL
149+
df -h /
150+
151+
- name: Install Rust Toolchain
152+
uses: ./.github/actions/rustup
153+
with:
154+
clippy: true
155+
156+
- name: Clippy
157+
run: cargo clippy --locked --workspace --all-targets -- -D warnings
158+
134159
doc:
135160
name: Doc
136161
runs-on: ubuntu-latest

‎pacquet/AGENTS.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -380,8 +380,9 @@ are part of the public contract, not implementation detail. See
380380
- Reference the upstream pnpm commit/PR you ported from, when applicable.
381381
- Run `just ready` before pushing.
382382
- The repo-wide husky `pre-push` hook runs `pacquet/scripts/pre-push-rust.sh`,
383-
which checks `rustfmt`, `taplo`, `cargo doc` (with
384-
`RUSTDOCFLAGS=-D warnings`), and `cargo dylint`. Make sure your environment
383+
which checks `rustfmt`, `taplo`, `cargo clippy` (with `--all-targets -D
384+
warnings`), `cargo doc` (with `RUSTDOCFLAGS=-D warnings`), and `cargo
385+
dylint`. Make sure your environment
385386
can run cargo (the hook needs it) before pushing; `cargo-dylint` is
386387
detected at runtime and skipped with a warning if not installed.
387388

‎pacquet/scripts/pre-push-rust.sh‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,16 @@ if command -v cargo >/dev/null 2>&1; then
1515
failed=1
1616
fi
1717

18+
# Mirror the CI clippy gate. `--all-targets` is the load-bearing flag:
19+
# without it clippy skips the test and bench crates, so a lint that
20+
# only fires in an integration test slips past `cargo clippy -p <crate>`
21+
# and surfaces for the first time in CI.
22+
yellow '▸ cargo clippy --all-targets --workspace -- -D warnings'
23+
if ! cargo clippy --all-targets --workspace -- -D warnings; then
24+
red '✗ cargo clippy reported lints — fix the findings (or `just lint`) and commit.'
25+
failed=1
26+
fi
27+
1828
yellow '▸ RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --all-features'
1929
if ! RUSTDOCFLAGS='-D warnings' cargo doc --no-deps --workspace --all-features --quiet; then
2030
red '✗ cargo doc reported warnings — fix the rustdoc diagnostics and commit.'

‎pnpr/crates/pnpr/tests/batch_publish.rs‎

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -31,18 +31,18 @@ async fn body_json(body: Body) -> Value {
3131
serde_json::from_slice(&body_bytes(body).await).expect("body parses as JSON")
3232
}
3333

34-
fn put_json(path: &str, body: Value) -> Request<Body> {
34+
fn put_json(path: &str, body: &Value) -> Request<Body> {
3535
Request::put(path)
3636
.header("content-type", "application/json")
37-
.body(Body::from(serde_json::to_vec(&body).unwrap()))
37+
.body(Body::from(serde_json::to_vec(body).unwrap()))
3838
.unwrap()
3939
}
4040

41-
fn put_json_with_token(path: &str, body: Value, token: &str) -> Request<Body> {
41+
fn put_json_with_token(path: &str, body: &Value, token: &str) -> Request<Body> {
4242
Request::put(path)
4343
.header("content-type", "application/json")
4444
.header("Authorization", format!("Bearer {token}"))
45-
.body(Body::from(serde_json::to_vec(&body).unwrap()))
45+
.body(Body::from(serde_json::to_vec(body).unwrap()))
4646
.unwrap()
4747
}
4848

@@ -56,7 +56,7 @@ async fn add_user_and_get_token(app: axum::Router, username: &str, password: &st
5656
"type": "user",
5757
"roles": [],
5858
});
59-
let response = app.oneshot(put_json(&path, body)).await.unwrap();
59+
let response = app.oneshot(put_json(&path, &body)).await.unwrap();
6060
assert_eq!(response.status(), StatusCode::CREATED);
6161
let payload = body_json(response.into_body()).await;
6262
payload["token"].as_str().expect("token in response").to_string()
@@ -106,8 +106,11 @@ async fn batch_publish_writes_every_package_in_one_request() {
106106
publish_doc("batch-b", "2.0.0", bytes_b),
107107
],
108108
});
109-
let response =
110-
app.clone().oneshot(put_json_with_token("/-/pnpm/v1/publish", body, &token)).await.unwrap();
109+
let response = app
110+
.clone()
111+
.oneshot(put_json_with_token("/-/pnpm/v1/publish", &body, &token))
112+
.await
113+
.unwrap();
111114
assert_eq!(response.status(), StatusCode::CREATED);
112115
let payload = body_json(response.into_body()).await;
113116
assert_eq!(payload["ok"], true);
@@ -175,8 +178,11 @@ async fn batch_publish_supports_scoped_packages_with_libnpmpublish_attachment_na
175178
},
176179
}],
177180
});
178-
let response =
179-
app.clone().oneshot(put_json_with_token("/-/pnpm/v1/publish", body, &token)).await.unwrap();
181+
let response = app
182+
.clone()
183+
.oneshot(put_json_with_token("/-/pnpm/v1/publish", &body, &token))
184+
.await
185+
.unwrap();
180186
assert_eq!(response.status(), StatusCode::CREATED);
181187

182188
// On disk: canonical `<basename>-<version>.tgz` under the scope dir.
@@ -215,7 +221,7 @@ async fn anonymous_batch_publish_is_rejected() {
215221
let app = router(static_config(storage.clone()));
216222

217223
let body = json!({ "packages": [publish_doc("anon-batch", "1.0.0", b"bytes")] });
218-
let response = app.oneshot(put_json("/-/pnpm/v1/publish", body)).await.unwrap();
224+
let response = app.oneshot(put_json("/-/pnpm/v1/publish", &body)).await.unwrap();
219225
assert_eq!(response.status(), StatusCode::UNAUTHORIZED);
220226
assert!(!storage.join("anon-batch").exists());
221227
}
@@ -236,7 +242,7 @@ async fn batch_publish_rolls_back_every_package_when_one_fails_integrity() {
236242

237243
let body = json!({ "packages": [good, bad] });
238244
let response =
239-
app.oneshot(put_json_with_token("/-/pnpm/v1/publish", body, &token)).await.unwrap();
245+
app.oneshot(put_json_with_token("/-/pnpm/v1/publish", &body, &token)).await.unwrap();
240246
assert_eq!(response.status(), StatusCode::BAD_REQUEST);
241247
let body_text = String::from_utf8(body_bytes(response.into_body()).await).unwrap();
242248
assert!(body_text.contains("EINTEGRITY"), "error should carry EINTEGRITY: {body_text}");
@@ -270,7 +276,7 @@ async fn batch_publish_rejects_duplicate_package_names() {
270276
],
271277
});
272278
let response =
273-
app.oneshot(put_json_with_token("/-/pnpm/v1/publish", body, &token)).await.unwrap();
279+
app.oneshot(put_json_with_token("/-/pnpm/v1/publish", &body, &token)).await.unwrap();
274280
assert_eq!(response.status(), StatusCode::BAD_REQUEST);
275281
let body_text = String::from_utf8(body_bytes(response.into_body()).await).unwrap();
276282
assert!(body_text.contains("duplicate package"), "got: {body_text}");
@@ -286,7 +292,7 @@ async fn batch_publish_rejects_bodies_without_a_packages_array() {
286292
for body in [json!({}), json!({ "packages": [] }), json!({ "packages": "nope" }), json!([])] {
287293
let response = app
288294
.clone()
289-
.oneshot(put_json_with_token("/-/pnpm/v1/publish", body.clone(), &token))
295+
.oneshot(put_json_with_token("/-/pnpm/v1/publish", &body, &token))
290296
.await
291297
.unwrap();
292298
assert_eq!(response.status(), StatusCode::BAD_REQUEST, "body: {body}");
@@ -301,7 +307,7 @@ async fn batch_publish_rejects_entries_without_a_name() {
301307

302308
let body = json!({ "packages": [{ "versions": {} }] });
303309
let response =
304-
app.oneshot(put_json_with_token("/-/pnpm/v1/publish", body, &token)).await.unwrap();
310+
app.oneshot(put_json_with_token("/-/pnpm/v1/publish", &body, &token)).await.unwrap();
305311
assert_eq!(response.status(), StatusCode::BAD_REQUEST);
306312
}
307313

@@ -317,14 +323,14 @@ async fn batch_publish_merges_with_previously_published_versions() {
317323
let first = json!({ "packages": [publish_doc("merge-pkg", "1.0.0", b"v1")] });
318324
let response = app
319325
.clone()
320-
.oneshot(put_json_with_token("/-/pnpm/v1/publish", first, &token))
326+
.oneshot(put_json_with_token("/-/pnpm/v1/publish", &first, &token))
321327
.await
322328
.unwrap();
323329
assert_eq!(response.status(), StatusCode::CREATED);
324330

325331
let second = json!({ "packages": [publish_doc("merge-pkg", "2.0.0", b"v2")] });
326332
let response =
327-
app.oneshot(put_json_with_token("/-/pnpm/v1/publish", second, &token)).await.unwrap();
333+
app.oneshot(put_json_with_token("/-/pnpm/v1/publish", &second, &token)).await.unwrap();
328334
assert_eq!(response.status(), StatusCode::CREATED);
329335

330336
let packument: Value =

0 commit comments

Comments
 (0)