ci: run dylint as a pnpm pipeline again (#16798)

pnpm runs scripts with the workspace toolchain first on PATH, the way it
puts a pinned Node.js first. cargo-dylint needs rustup itself: it runs the
`rustup` binary and builds its lint library with the nightly the library
pins, which only rustup's cargo selects. That is a requirement of the one
tool, so the dylint script asks for rustup's proxies instead of pnpm
changing how it runs Rust for every script.

pnpm/scripts/cargo-dylint.mjs runs `cargo dylint` from the first PATH
directory that holds both rustup and cargo, with that directory first on
PATH. The ci:rust-dylint script, the rust-dylint pipeline and its cargo
concurrency group come back, and CI runs the pipeline again, which also
installs the esm-loader bundle the crates embed, so its separate install
step goes. The pre-push hook calls the same helper in place of its own
PATH edit.

CONTRIBUTING.md no longer says pnpm install needs cargo on PATH: pnpm runs
cargo metadata from the toolchain it installs for rust-toolchain.toml.

.github/workflows/pacquet-ci.yml

@@ -682,13 +682,7 @@ jobs:
         with:
           install: false
 
-      - name: Bundle the runtime loader the Rust crates embed
-        run: pnpm install --frozen-lockfile --filter @pnpm/esm-loader
-
       - name: Run dylint
-        # Not a `pnpm pipeline`: pnpm runs scripts with the workspace toolchain
-        # first on `PATH`, and cargo-dylint needs the rustup proxies there to
-        # build the lint library with the nightly the library pins.
         # `-D warnings` must come in via `RUSTFLAGS`, not as a trailing
         # `-- -D warnings` after `cargo dylint`'s `--`: `cargo dylint`
         # forwards args after `--` to `cargo check`, which would treat
@@ -696,7 +690,7 @@ jobs:
         # how KSXGitHub/parallel-disk-usage invokes the same lint.
         env:
           RUSTFLAGS: '-D warnings'
-        run: cargo dylint --all -- --all-targets --workspace
+        run: pnpm pipeline rust-dylint --include-workspace-root --full --no-cache
 
   # Single aggregate gate — the only Rust CI context branch protection
   # needs to require. Listing the individual jobs as required checks does

CONTRIBUTING.md

@@ -27,7 +27,7 @@ The repository holds two implementations of the same package manager: the TypeSc
 ### JavaScript and TypeScript CLI
 
 1. Install pnpm using one of the [official installation methods](https://pnpm.io/installation). **Do not use Corepack.** The scripts in this repository invoke pnpm through the `pn` and `pnx` aliases, which the official installation methods create. Corepack only provides the `pnpm` and `pnpx` commands, so with a Corepack-managed pnpm the build fails with errors like `pn: Permission denied` ([pnpm/pnpm#12448](https://github.com/pnpm/pnpm/issues/12448)).
-1. Set up the Rust toolchain first, as described under [Rust toolchain and git hooks](#rust-toolchain-and-git-hooks). `pnpm install` also installs the Rust dependencies and runs `cargo`, so it fails when `cargo` is not on `PATH`.
+1. Set up the Rust tools first, as described under [Rust toolchain and git hooks](#rust-toolchain-and-git-hooks).
 1. Run `pnpm install` in the root of the repository to install all dependencies.
 1. Run `pnpm add ./pnpm11/pnpm/dev -g` to make pnpm from the repository available in the command line via the `pd` command.
 1. Run `pnpm run compile` to create an initial build of pnpm from the source in the repository.
@@ -45,7 +45,7 @@ sudo dnf install make automake gcc gcc-c++ kernel-devel
 
 Rust is now the primary language in this repository, so most contributions need a working Rust toolchain and the Rust developer tools. The Rust workspace (`Cargo.toml`, `rust-toolchain.toml`, `justfile`) lives at the repository root; run `cargo` and `just` from there.
 
-1. Install [`rustup`](https://rustup.rs). You do not need to select a toolchain by hand. `rust-toolchain.toml` pins the version the project builds with, and `rustup` installs it, together with `clippy` from the pinned `default` profile, the first time you run `cargo` inside the repository.
+1. Install [`rustup`](https://rustup.rs). `pnpm install` installs the toolchain `rust-toolchain.toml` pins, and the root `package.json` scripts run it. rustup provides the `cargo` you run outside those scripts, such as through `just`, and the dated nightlies that the [pinned formatter](#rust-formatting) and the dylint check build with.
 
 2. Install [`just`](https://just.systems) (the task runner) and [`cargo-binstall`](https://github.com/cargo-bins/cargo-binstall), then install the task tools from the repository root:
 

package.json

@@ -55,6 +55,7 @@
     "ci:rust-deny": "cargo deny check",
     "ci:rust-fmt": "node pnpm/scripts/rustfmt.mjs --all -- --check",
     "ci:toml-fmt": "taplo format --check",
+    "ci:rust-dylint": "node pnpm/scripts/cargo-dylint.mjs --all -- --all-targets --workspace",
     "ci:build-pnpr": "cargo build --locked --release -p pnpr --bin pnpr -p pnpr-fixtures --bin pnpr-prepare"
   },
   "devDependencies": {

pnpm-workspace.yaml

@@ -83,6 +83,7 @@ pipelines:
   rust-doc: [ci:rust-doc]
   rust-deny: [ci:rust-deny]
   rust-format: [ci:rust-fmt, ci:toml-fmt]
+  rust-dylint: [ci:rust-dylint]
   build-pnpr: [ci:build-pnpr]
 
 # Every entry names a script the root manifest alone defines. A name a
@@ -142,6 +143,8 @@ tasks:
     concurrencyGroup: cargo
   ci:rust-doc:
     concurrencyGroup: cargo
+  ci:rust-dylint:
+    concurrencyGroup: cargo
   ci:rust-test:
     concurrencyGroup: cargo
 

pnpm/scripts/cargo-dylint.mjs

@@ -0,0 +1,28 @@
+// Run `cargo dylint` with rustup's proxies first on `PATH`.
+//
+// cargo-dylint runs `rustup` itself and builds its lint library with the
+// nightly the library pins, which only rustup's `cargo` selects. pnpm runs
+// scripts with the workspace toolchain first on `PATH`, ahead of rustup.
+import { spawnSync } from 'node:child_process'
+import fs from 'node:fs'
+import path from 'node:path'
+import process from 'node:process'
+
+const exe = process.platform === 'win32' ? '.exe' : ''
+
+const pathKey = Object.keys(process.env).find((key) => key.toUpperCase() === 'PATH') ?? 'PATH'
+const searchPath = process.env[pathKey] ?? ''
+const proxies = searchPath
+  .split(path.delimiter)
+  .find((dir) => path.isAbsolute(dir) && ['rustup', 'cargo'].every((name) => fs.existsSync(path.join(dir, name + exe))))
+if (proxies == null) {
+  process.stderr.write('cargo-dylint needs rustup, and no directory on PATH holds both rustup and cargo.\n')
+  process.exit(1)
+}
+
+const result = spawnSync(path.join(proxies, 'cargo' + exe), ['dylint', ...process.argv.slice(2)], {
+  stdio: 'inherit',
+  env: { ...process.env, [pathKey]: [proxies, searchPath].join(path.delimiter) },
+})
+if (result.error != null) throw result.error
+process.exit(result.status ?? 1)

pnpm/scripts/cargo-dylint.test.mjs

@@ -0,0 +1,65 @@
+import assert from 'node:assert/strict'
+import { spawnSync } from 'node:child_process'
+import fs from 'node:fs'
+import os from 'node:os'
+import path from 'node:path'
+import process from 'node:process'
+import { test } from 'node:test'
+import { fileURLToPath } from 'node:url'
+
+const SCRIPT = path.join(path.dirname(fileURLToPath(import.meta.url)), 'cargo-dylint.mjs')
+
+function stubDirs (context, layout) {
+  const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'pnpm-cargo-dylint-')))
+  context.after(() => fs.rmSync(root, { recursive: true, force: true }))
+  const record = path.join(root, 'invocations')
+  const dirs = {}
+  for (const [dir, names] of Object.entries(layout)) {
+    dirs[dir] = path.join(root, dir)
+    fs.mkdirSync(dirs[dir])
+    for (const name of names) {
+      fs.writeFileSync(
+        path.join(dirs[dir], name),
+        `#!/bin/sh\necho "${dir}/${name} $* PATH=$PATH" >> '${record}'\n`,
+        { mode: 0o755 }
+      )
+    }
+  }
+  return { dirs, record }
+}
+
+function runHelper (searchPath) {
+  return spawnSync(process.execPath, [SCRIPT, '--all'], {
+    encoding: 'utf8',
+    env: { PATH: [...searchPath, '/usr/bin', '/bin'].join(path.delimiter) },
+  })
+}
+
+test('runs the cargo beside rustup, ahead of a toolchain cargo earlier on PATH', { skip: process.platform === 'win32' }, (context) => {
+  const { dirs, record } = stubDirs(context, {
+    toolchain: ['cargo'],
+    rustupOnly: ['rustup'],
+    proxies: ['rustup', 'cargo'],
+  })
+
+  const result = runHelper([dirs.toolchain, dirs.rustupOnly, dirs.proxies])
+
+  assert.equal(result.status, 0, result.stderr)
+  const [invocation, ...rest] = fs.readFileSync(record, 'utf8').trim().split('\n')
+  assert.deepEqual(rest, [])
+  assert.ok(invocation.startsWith('proxies/cargo dylint --all PATH='), invocation)
+  assert.equal(invocation.split('PATH=')[1].split(path.delimiter)[0], dirs.proxies)
+})
+
+test('fails without a directory that holds both rustup and cargo', { skip: process.platform === 'win32' }, (context) => {
+  const { dirs, record } = stubDirs(context, {
+    toolchain: ['cargo'],
+    rustupOnly: ['rustup'],
+  })
+
+  const result = runHelper([dirs.toolchain, dirs.rustupOnly])
+
+  assert.equal(result.status, 1)
+  assert.match(result.stderr, /needs rustup/)
+  assert.equal(fs.existsSync(record), false)
+})

pnpm/scripts/pre-push-rust.sh

@@ -36,24 +36,20 @@ if command -v cargo >/dev/null 2>&1; then
         failed=1
     fi
 
-    if command -v cargo-dylint >/dev/null 2>&1; then
+    if command -v cargo-dylint >/dev/null 2>&1 && command -v rustup >/dev/null 2>&1; then
         yellow '▸ RUSTFLAGS="-D warnings" cargo dylint --all -- --all-targets --workspace'
         # Git exports its repository-local variables (GIT_DIR, GIT_INDEX_FILE,
         # ...) to hooks. When dylint builds a driver for a new toolchain it
         # runs `git checkout` in its own clone of rust-clippy, and an inherited
         # GIT_DIR makes that checkout read this repository instead and fail
         # (https://github.com/trailofbits/dylint/issues/2105).
-        # pnpm runs this hook with the workspace toolchain first on PATH.
-        # cargo-dylint builds its lint library with the nightly the library
-        # pins, which only the rustup proxies installed beside `rustup` select.
-        rustup_proxies=$(dirname "$(command -v rustup || command -v cargo)")
         # shellcheck disable=SC2046
-        if ! (unset $(git rev-parse --local-env-vars) && PATH="$rustup_proxies:$PATH" RUSTFLAGS='-D warnings' cargo dylint --all -- --all-targets --workspace); then
+        if ! (unset $(git rev-parse --local-env-vars) && RUSTFLAGS='-D warnings' node pnpm/scripts/cargo-dylint.mjs --all -- --all-targets --workspace); then
             red '✗ cargo dylint reported lints — `just dylint-fix` applies the ones it can, then commit.'
             failed=1
         fi
     else
-        yellow '! cargo-dylint not found on PATH — skipping dylint check (install from source with `cargo install cargo-dylint dylint-link`).'
+        yellow '! cargo-dylint or rustup not found on PATH — skipping dylint check (install rustup, then cargo-dylint from source with `cargo install cargo-dylint dylint-link`).'
     fi
 else
     yellow '! cargo not found on PATH — skipping Rust format, doc, and dylint checks.'

pnpm/scripts/pre-push-rust.test.mjs

@@ -10,7 +10,7 @@ import { temporaryRepo } from './git-fixture.mjs'
 
 const SCRIPT = path.join(path.dirname(fileURLToPath(import.meta.url)), 'pre-push-rust.sh')
 
-function stubbedCheckout (context) {
+function stubbedCheckout (context, { rustup = true } = {}) {
   const dir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'pnpm-pre-push-')))
   context.after(() => fs.rmSync(dir, { recursive: true, force: true }))
   const bin = path.join(dir, 'bin')
@@ -19,10 +19,12 @@ function stubbedCheckout (context) {
   const stub = (name, body) => fs.writeFileSync(path.join(bin, name), `#!/bin/sh\n${body}\nexit 0\n`, { mode: 0o755 })
   stub('cargo', `if [ "$1" = dylint ]; then env | grep '^GIT_' > '${record}'; fi`)
   stub('cargo-dylint', '')
+  if (rustup) stub('rustup', '')
   stub('typos', '')
   stub('taplo', '')
   fs.mkdirSync(path.join(dir, 'pnpm/scripts'), { recursive: true })
   fs.writeFileSync(path.join(dir, 'pnpm/scripts/rustfmt.mjs'), '')
+  fs.copyFileSync(path.join(path.dirname(SCRIPT), 'cargo-dylint.mjs'), path.join(dir, 'pnpm/scripts/cargo-dylint.mjs'))
   return { dir, bin, record }
 }
 
@@ -44,3 +46,17 @@ test('runs cargo dylint without the git variables a hook inherits', { skip: proc
   assert.equal(result.status, 0, result.stderr)
   assert.equal(fs.readFileSync(record, 'utf8'), '')
 })
+
+test('skips cargo dylint when rustup is not on PATH', { skip: process.platform === 'win32' }, (context) => {
+  const { dir, bin, record } = stubbedCheckout(context, { rustup: false })
+
+  const result = spawnSync('bash', [SCRIPT], {
+    cwd: dir,
+    encoding: 'utf8',
+    env: { PATH: [bin, path.dirname(process.execPath), '/usr/bin', '/bin'].join(path.delimiter) },
+  })
+
+  assert.equal(result.status, 0, result.stderr)
+  assert.match(result.stderr, /skipping dylint check/)
+  assert.equal(fs.existsSync(record), false)
+})