ci: skip the electron binary download on typecheck and lint - #380
Conversation
Zaldaryon
left a comment
There was a problem hiding this comment.
The change itself is sound. node_modules/electron 44.1.1 ships no scripts in its package.json, so npm ci alone never fetches the platform binary, and the only downloader in the repo is the postinstall calling scripts/fix-native-deps.js, which is exactly where the guard sits. electron's own install.js never reads ELECTRON_SKIP_BINARY_DOWNLOAD, so the comment is right that nothing downstream would honour it. tsc reads electron.d.ts from the npm tarball and .eslintrc.cjs needs nothing from Electron, so neither guarded job loses anything. The five required checks are green on 13de23a, and the npm ci log in the CI evidence section shows the skip firing on both jobs. Local gate here is green too: typecheck, lint:ci (0 errors, 15 pre-existing hook warnings), format:check, test:coverage at 93.45% statements above the 87 floor, build:unpack.
The blocker is the description. It is #385's body, word for word: it describes the Manage Mods bulk-action filter scope, the backup failure cause, and the window-open spread check, and it lists Closes #357, Closes #358, Closes #378, none of which this PR touches. This PR is the #377 CI change. Please replace the body with a description of what this PR does and set Closes #377. dev is not the default branch so nothing auto-closes on merge, but the record should still be right, and a reader arriving here from #377 currently finds a page about three other issues.
One non-blocking note on tests/config/electron-binary-download-skip.test.ts: the ^ {2}(\S+):$ job-block parser also matches push:, pull_request: and workflow_dispatch: under on:, and folds every line between workflow_dispatch: and typecheck:, the new comment block included, into a pseudo-job named workflow_dispatch. It passes today only because that comment does not contain the literal ELECTRON_SKIP_BINARY_DOWNLOAD. Starting the collection after the jobs: line would make it robust, and a second case pinning the negative (variable unset, the download is attempted) would catch an inverted condition.
|
You are right, and it was our slip: the body was #385's, pasted onto the wrong PR. Replaced with a description of this change, |
The typecheck and lint jobs run tsc, eslint and prettier. Neither launches Electron, yet `npm ci` fetched the platform binary for them anyway, so one hiccup on the download mirror failed a job whose only work is static analysis. Run 33969911703 lost typecheck that way while the six other jobs on the same commit passed. Both jobs now set ELECTRON_SKIP_BINARY_DOWNLOAD and fix-native-deps.js returns early when it sees it, logging one line. The guard has to live in the script: electron 44's install.js dropped its own check on that variable and @electron/get 5 never had one, so nothing downstream would honour it. test-matrix, build and sonarcloud are untouched and keep the download. Closes #377
The job-block parser walked the whole workflow, so the top-level `on:` keys
matched `^ {2}(\S+):$` too and `workflow_dispatch` became a pseudo-job holding
every line up to `typecheck:`, the `jobs:` comment block included. It only
passed because that comment does not spell out ELECTRON_SKIP_BINARY_DOWNLOAD.
Collection now starts below `jobs:`.
Added the negative case: with the variable unset the script must fall through
to the installer. The repo's own node_modules already has the binary, so the
script is copied into a sandbox with a stub install.js, which shows the guard
falling through without touching the download mirror.
13de23a to
82d6a3e
Compare
|
Body replaced, and I took the test note too. Rebased on current Two changes in
Mutants for both, on the rebased tree:
Gates on the rebased tree, all local:
Body figures updated to match, and the mutant section now carries these two. Nothing in |
Zaldaryon
left a comment
There was a problem hiding this comment.
Both points from the last round are addressed.
The description is now this PR's: it explains the #369 rebase run that hit the mirror, Closes #377, and it names the two ci.yml jobs and the guard in scripts/fix-native-deps.js with the reason the guard has to live there. The behaviour matrix and the note that #377's own premise was corrected are a good touch.
The new commit 82d6a3e closes the two test gaps. The job-block parser now slices from below jobs:, so the top-level on: keys are no longer mistaken for jobs, and the added "runs the electron installer when the variable is not set" case builds a sandbox with a stub install.js, unsets the variable, and asserts the guard falls through to the installer without touching the mirror. The ci.yml and fix-native-deps.js changes themselves are byte-identical to the head I reviewed before (git range-diff confirms).
Full local gate on 82d6a3e is green: typecheck, lint:ci (0 errors, 15 pre-existing hook warnings), format:check, test:coverage at 93.52% statements and 90.27% branches above the 87/85 floors, build:unpack on Electron 44.1.1. npx vitest run tests/config/electron-binary-download-skip.test.ts: 4 passing. The five required GitHub contexts pass on the head, and the npm ci log shows the skip firing on both guarded jobs.
Approving.
Closes #377.
Run 33969911703 on #369, a pure rebase with no source change, failed its
typecheckjob insidenpm ci: thepostinstallscript (electron-builder install-app-deps && node scripts/fix-native-deps.js) noticed the Electron binary was missing and tried to download it from the mirror, and the mirror answeredTypeError: fetch failed. The job's only work istsc --noEmit, which readselectron.d.tsfrom the npm tarball and needs no binary. Same exposure forlint. Every rebase of every open PR was one mirror hiccup away from a reviewer round trip.What changes
.github/workflows/ci.yml:ELECTRON_SKIP_BINARY_DOWNLOAD: 1in theenvof thetypecheckandlintjobs only, with a comment saying why. The build, test-matrix, sonarcloud and macOS jobs are untouched and keep downloading. Job keys are byte-identical todev, so the five required contexts still report under their own names.scripts/fix-native-deps.js: an early return at the top ofensureElectronBinaryIsDownloaded()when that variable is set, logging one line. The guard has to live here:node_modules/electron44.1.1 ships noscriptsin itspackage.json, sonpm ciitself never fetches the binary, and neither@electron/get5.1.0 nor Electron's owninstall.jsreadsELECTRON_SKIP_BINARY_DOWNLOADany more (the string appears in neither package). Setting the variable in the workflow alone would have changed nothing; CI: typecheck and lint fail when the Electron binary download hiccups, though they never run Electron #377's premise said otherwise and has been corrected there.tests/config/electron-binary-download-skip.test.ts: pins that the two check jobs carry the variable, that the download jobs do not, that the script honours it when set, and that it falls through to the installer when unset. The job-block parser starts below thejobs:line, so the top-levelon:keys are not mistaken for jobs.Behaviour matrix, old script against new, mirror pointed at a dead port
fetch faileddistCI evidence, run 33983272184 on 13de23a
typecheckandlintboth log duringnpm ci:test-matrix (ubuntu-latest)on the same run still logs[fix-native-deps] electron binary missing, running node_modules/electron/install.js. All eight checks pass.Mutants
Guard removed from the script: red on the skip case.
envblock removed from thelintjob: red on the workflow case.Two more after review, on the rebased tree:
ELECTRON_SKIP_BINARY_DOWNLOADwritten into a comment underon:, next toworkflow_dispatch:. The old parser reported["lint", "typecheck", "workflow_dispatch"]and its assertion failed; the new one reported["lint", "typecheck"]and the file stayed green.if (!process.env.ELECTRON_SKIP_BINARY_DOWNLOAD). Red, 2 of 4: the new unset case, which no longer sees the installer run, and the existing set case, which no longer sees the skip line.Left for a follow-up
Caching
~/.cache/electronon the jobs that download.actions/cacheis not used anywhere inci.ymltoday, so it would be a new block in four jobs with a Windows path to verify; it deserves its own PR and its own run.Testing
npm run typecheck: passed (node, web, tests), with no Electron binary in the tree.npm run lint:ci: 0 errors, 15 pre-existing React Hooks warnings.npm run format:check: passed.npm run test:coverage: 167 files, 2150 passed, 2 skipped; 93.52 / 90.27 / 93.06 / 95.09 against floors 87 / 85 / 85 / 89.npm run build:unpack: passed, Electron 44.1.1 Linux.Related issues
Type
Checklist
dev, notmain.npm run typecheckpasses.npm run lint:cipasses.npm run format:checkpasses.npm run test:coveragepasses, coverage at or above the floor invitest.config.ts.npm run build:unpackpasses.