Skip to content

rust:mordant: rebuild the lint libraries when the pinned entries change - #38418

Open
robobun wants to merge 1 commit into
mainfrom
farm/7c65e900/mordant-stale-library
Open

rust:mordant: rebuild the lint libraries when the pinned entries change#38418
robobun wants to merge 1 commit into
mainfrom
farm/7c65e900/mordant-stale-library

Conversation

@robobun

@robobun robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • After the mordant rev under [workspace.metadata.dylint] in Cargo.toml is bumped, bun run rust:mordant on a machine that has run it before keeps linting with the library built from the previous rev. Seen going fc46d7f -> 4e08693: cargo-dylint printed Finished release ... in 0.14s with no Compiling mordant line, target/dylint/libraries/<toolchain>/release/libmordant.so was byte for byte the old build, and the run still reported stored projection at src/bundler/ParseTask.rs:88, a finding the new rev no longer produces. The bump procedure in .github/workflows/CLAUDE.md (change the rev, run the script, triage what it reports) therefore triages against the old lints. CI is unaffected, every run there starts from an empty target dir.
  • cargo-dylint builds a git entry by running cargo build --release --target-dir <target>/dylint/libraries/<toolchain> inside cargo's checkout of the rev, ~/.cargo/git/checkouts/mordant-*/<rev>/ (cargo-dylint 6.0.3, build_library in dylint/src/library_packages/mod.rs). The target dir is the same for every rev, and so is the fingerprint directory (mordant-cd12729808ff36de for both revs): the package is hashed by its path relative to its own workspace root, which is the checkout itself.
  • Cargo's freshness check skips every source file under $CARGO_HOME/git and $CARGO_HOME/registry, which it assumes never change (find_stale_file, cargo src/compiler/fingerprint/mod.rs). The new checkout's files are never looked at, so the old build stays fresh until target/dylint/libraries is deleted by hand. cargo-dylint has no option to force the build; its only knob is --no-build.

Fix

  • scripts/rust-mordant.ts asks cargo metadata --no-deps for the target directory and the [workspace.metadata.dylint] table (the two things cargo-dylint itself reads), keeps the table's JSON in <target>/dylint/libraries/built-from.json, and removes that directory before running cargo-dylint when the file is missing or differs. When it matches nothing happens and cargo reuses the build as before.
  • This is the right key because the entries are the one input to the library build that cargo does not track: the checkout's contents are fixed by the rev, the toolchain is in the directory name, and everything else is in cargo's fingerprint. Only libraries/ is discarded. The check of the workspace itself (<target>/dylint/target/) needs no help, dylint's driver hashes the metadata and the library bytes into rustc's dependency tracking, which is also why the stale library was visibly re-linting the workspace in the first place.
  • The file is written before the build rather than after a successful run: it records what the directory is for, and whether the builds in it finished is cargo's own bookkeeping. Writing it after success would rebuild the libraries on every run for as long as the lint pass exits non-zero, which is the whole time a bump is being triaged.
  • cargo metadata --no-deps is about 400 KB here and spawnSync stops capturing at 1 MB by default, so the cap is raised.
  • Test: test/internal/rust-mordant-pin-bump.test.ts runs the script against stand-in cargo and bun executables and checks that a directory with no record is discarded, that the same pin keeps it, that a changed pin discards it and leaves the check cache alone, and that a failing cargo metadata stops the run. The script's change is outside src/, so the fail-before run is git stash push -- scripts/ && bun bd test test/internal/rust-mordant-pin-bump.test.ts: the directory is kept and no record is written. Passes with the change.
  • Also run for real: with target/dylint/libraries holding the fc46d7f build and Cargo.toml at the current pin (18a5734), the script printed its rebuilding line, cargo printed Compiling mordant v0.1.0 (.../18a5734), the workspace pass finished clean (exit 0, as Bump mordant to 18a5734 #38384 says it should), and a second run reused the build (Finished ... in 0.08s, no rebuilding line).

Background

  • dylint is a cargo plugin that loads lint libraries (cdylibs built against the nightly named in their own repo) into a rustc driver and runs them over a workspace, the way clippy runs its built-in lints. Mordant is one such library; [workspace.metadata.dylint] in Cargo.toml tells cargo-dylint which git rev of it to build and load.
  • A cargo fingerprint is the per-crate record under .fingerprint/ in a target dir that cargo compares against to decide whether to recompile. For a package built from a local path it includes the list of source files whose mtimes are compared against it; the $CARGO_HOME exemption above removes the checkout's files from that comparison.
  • cargo-dylint keeps three things: <target>/dylint/libraries/<toolchain>/ with the library builds (what this change manages), <target>/dylint/target/<toolchain>/ with the check of this workspace, and ~/.dylint_drivers/<toolchain>/ with the driver. The last two are already keyed by what invalidates them.
Reproduction at the previous two pins

Cargo.toml at fc46d7f, empty target/dylint:

$ cargo dylint list --all
Building workspace metadata entry `mordant`
   ...
   Compiling mordant v0.1.0 (/root/.cargo/git/checkouts/mordant-f8af09eb0c74a22a/fc46d7f)
    Finished `release` profile [optimized] target(s) in 43.17s
$ sha256sum target/dylint/libraries/*/release/libmordant.so
fb399c9443ae4ad454d2124e03476b61b241bbff46d27e4e95b5dd433c691304

Cargo.toml moved to 4e08693:

$ cargo dylint list --all
Building workspace metadata entry `mordant`
    Finished `release` profile [optimized] target(s) in 0.14s
$ sha256sum target/dylint/libraries/*/release/libmordant.so
fb399c9443ae4ad454d2124e03476b61b241bbff46d27e4e95b5dd433c691304
$ ls ~/.cargo/git/checkouts/mordant-f8af09eb0c74a22a/
4e08693  fc46d7f

The 4e08693 checkout was created after the build above (its files are newer than the fingerprint), and the fingerprint's dep-info lists the sources as src/lib.rs, src/stored_projection.rs, ... relative to whichever checkout is being built, so without the $CARGO_HOME exemption cargo would have rebuilt. rm -rf target/dylint/libraries followed by the same command compiles mordant from the 4e08693 checkout.

cargo-dylint builds each [workspace.metadata.dylint] entry out of cargo's
git checkout of the pinned rev into target/dylint/libraries/<toolchain>/,
one target dir shared by every rev. Cargo skips freshness checks on files
under $CARGO_HOME/git and the rev is not part of the fingerprint, so after
a rev bump the library built from the previous rev is reused and the bump
gets triaged against the old lints. Record the entries the directory was
built from in it and discard it when they differ.
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b6df94f1-f2a2-464f-9d5a-03da38121edf

📥 Commits

Reviewing files that changed from the base of the PR and between 01aa7cd and 1e5d44f.

📒 Files selected for processing (2)
  • scripts/rust-mordant.ts
  • test/internal/rust-mordant-pin-bump.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced and fixed, self-review in progress.

  • Reproduced with cargo-dylint 6.0.3 by building the library at fc46d7f, moving Cargo.toml to 4e08693 and running again: Finished release ... in 0.14s, no Compiling mordant, same libmordant.so sha256 as before. Mechanism in the description (cargo exempts $CARGO_HOME/git from freshness checks, and the fingerprint does not depend on the rev).
  • test/internal/rust-mordant-pin-bump.test.ts fails against the unmodified script (git stash push -- scripts/) and passes with it.
  • The script was also run for real against a target/dylint/libraries built from fc46d7f with Cargo.toml at 18a5734: rebuilt from the new checkout, clean workspace pass, second run reused the build.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — self-contained dev-tooling fix with a hermetic test.

What was reviewed:

  • The invalidation key (JSON of [workspace.metadata.dylint]) and that only <target>/dylint/libraries/ is removed, leaving the workspace check cache and driver intact.
  • Write-before-build ordering of built-from.json — sound, avoids a rebuild loop while a bump is being triaged with non-zero lint exits.
  • cargo metadata handling: maxBuffer raised, non-zero status exits before any rmSync; the test's 2 MB packages field exercises the buffer cap.
  • Test hermeticity: stand-in cargo/bun on PATH, skipIf(isWindows) for the sh stubs, concurrent stdout/stderr/exited drain, and the dylint --version probe handled by the stub.
Extended reasoning...

Overview

This PR touches two files: scripts/rust-mordant.ts (a developer/CI wrapper around cargo dylint) and a new test at test/internal/rust-mordant-pin-bump.test.ts. The script gains a ~30-line stanza that reads cargo metadata --no-deps, compares the [workspace.metadata.dylint] table against a marker file in <target>/dylint/libraries/built-from.json, and removes that directory when they differ so cargo-dylint rebuilds the lint pack from the newly pinned rev instead of reusing a stale build. Nothing under src/ changes; no shipped runtime behavior is affected.

Security risks

None. The script is local dev tooling invoked via bun run rust:mordant. The only destructive operation is rmSync on <target_directory>/dylint/libraries, where target_directory comes from cargo metadata for this workspace — a build-artifact directory. Worst-case failure mode is an unnecessary ~1-minute lint-library rebuild. No user input, no network, no auth/crypto/permissions.

Level of scrutiny

Low. This is a build-script quality-of-life fix with a clearly stated root cause (cargo's fingerprint check skips sources under $CARGO_HOME/git, so a rev bump doesn't invalidate the shared libraries/ target dir). The blast radius is confined to a regenerable cache directory. CI already starts from an empty target dir, so this only changes behavior on developer machines that have run the script before. No CODEOWNERS cover the changed paths.

Other factors

The test is well-constructed per the repo's review guidance: it uses tempDir/bunEnv/bunExe from harness, drains stdout/stderr/exited concurrently, uses Buffer.alloc(n, fill) rather than .repeat(), asserts full result objects with toEqual, covers all four branches (no record, same pin, changed pin, cargo metadata failure), verifies the check cache is not discarded, and is properly gated with skipIf(isWindows) since the stubs are sh scripts. The stub cargo correctly short-circuits the dylint --version probe so it isn't counted as a run. Test placement in test/internal/ matches existing precedent for build-script tests (build-rust-toolchain-probe.test.ts, build-download-retry.test.ts, etc.). The PR description includes a real reproduction and a manual verification against the current pin. No outstanding reviewer comments.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Nothing to change from the review above; it covers the points the change rests on (the key is the metadata table, only libraries/ is removed, the record is written before the build). A second pass over the diff is still running; I will update the status comment if it turns anything up.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 2:05 AM PT - Aug 14th, 2026

@robobun, your commit 1e5d44f has some failures in Build #95665 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 38418

That installs a local version of the PR into your bun-38418 executable, so you can run:

bun-38418 --bun

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 5:05 AM PT - Aug 14th, 2026

@robobun, your commit 1e5d44f has some failures in Build #95665 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 38418

That installs a local version of the PR into your bun-38418 executable, so you can run:

bun-38418 --bun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants