Keep the CI package cache out of the packaged app - #267
Merged
Conversation
The artifact build kept the npm package cache inside the project directory, and electron-builder had no `files` filter, so it treated that cache as application content. A cache directory changes while the build runs: a log file that was present when the walk started and gone by the time it was read aborted the whole build, which is why every branch and trunk alike stopped producing signed macOS and Windows artifacts. Exclude `vendor` from packaging, and clear npm's logs on both sides of the CI cache so no build inherits another build's diagnostics. Verified by packaging locally with a stand-in cache in place: without the filter `vendor/npm/**` lands inside `app.asar`, logs and all; with it the directory is absent and `src/main.js`, `src/preload.js`, the bundled renderer and all 28085 `node_modules` entries are still there. Fixes #265 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
No branch has produced a signed macOS or Windows artifact since the last version bump. The build dies during packaging:
Trunk fails the same way, re-running a job does not help, and reviewers have had no way to test a pull request on a real machine — the only reason that build layer exists. Linux is unaffected, which is the first clue: it is the one job that does not use this cache.
Details in #265.
What changes
Root cause. The artifact build keeps npm's package cache at
vendor/npm, inside the project directory, and npm writes its own debug logs into it.package.jsonhad nofilesfilter, so electron-builder's default of "everything in the project directory is application content" swept that cache into the app. A cache directory is not static while a build runs: a log file present when the walk begins and gone by the time it is read takes the whole build down.Two changes, addressing two separate mistakes:
package.json— excludevendorfrom packaging. This is the fix. A directory that mutates during the build must not be a packaging input. Thefilesarray holds only a negation, which electron-builder handles by prepending its own**/*default (fileMatcher.js→containsOnlyIgnore()), so nothing else about what gets packaged changes. The!x{,/**/*}form is the one electron-builder uses internally for the same job.npm ciwrites a fresh log during the build regardless, so only the exclusion above closes the failure..gitignoregainsvendor/npmalongside the existingvendor/bundle, so the cache cannot be committed by anyone who runs the CI script locally.Why the version bump was the trigger. The cache key hashes
package-lock.json. The bump minted a fresh cache entry, and the build that created it archived its own debug log into it — the timestamp in the error is that build'snpm ci. Every build since has restored the poisoned entry.How to test this
Platforms: macOS and Windows. These are the two jobs that use this cache; Linux never had the problem.
The end-to-end test is this PR's own artifact build. Locally, packaging can be driven directly:
Starting state: a clean checkout with dependencies installed.
Stage a stand-in for the CI cache:
CSC_IDENTITY_AUTO_DISCOVERY=false npx electron-builder --dir --publish nevernpx asar list "dist/mac-arm64/WordPress Contributor Toolkit.app/Contents/Resources/app.asar" | grep '^/vendor'Expected: no output. On
trunkthe same command lists/vendor,/vendor/npm,/vendor/npm/_logsand the log file itself — that is the bug, visible without needing to reproduce the race.Same command without the
grep, confirming the app is still whole:/src/main.js,/src/preload.js,/src/renderer/index.js,/package.jsonand thenode_modulestree are all present.What must not have happened: the app must not have got smaller in any way other than losing
vendor. A wrongfilespattern would silently dropsrcornode_modulesand the packaging step would still report success — the app would only fail when a contributor launches it. Step 4 is what catches that. Also check the CI log still shows the npm cache being restored and saved: the cleanup must not have removed_cacache, or every build re-downloads every package.Risks and limitations
vendoris excluded; other non-app directories are not.docs,test,scriptsandfastlanestill get packaged, but they are ~217 entries against 28085 fornode_modules, so there is nothing to win there. The real artifact bulk is the dependency tree — see Stop shipping build-only deps in the app #23, which measured it and is a separate concern.node --test; a test asserting the string"!vendor{,/**/*}"is present would restate the diff rather than test it. Verified by actually packaging instead, twice, and the assertion is written into the testing steps above so it can be re-run.Related
Fixes #265. Adjacent to #23 (packaged app carries more than the app), which attacks artifact size through the dependency tree and is stale and conflicting — it needs its own decision.
Design decisions and alternatives considered
Moving npm's cache out of the project directory would have fixed the crash at its root, and was the first instinct. Rejected because the cache path is load-bearing for the CI cache save/restore, which archives that directory by relative path — relocating it means reworking the caching, on a pipeline that is currently red. Excluding it from packaging is the smaller change and the correct invariant regardless of where the cache lives.
Setting npm's
logs-diroutside the project is the tidier version of the log cleanup and would make the deletions unnecessary. Not done here because it needs a path that is valid on both the macOS and Windows agents, which is a second decision to get wrong while the pipeline is broken. Worth revisiting once builds are green.An inclusive
fileslist naming what to package would be more explicit than a lone negation, and electron-builder's own docs argue against it: an inclusive list silently drops anything added later. The negation keeps the default and states only the exception.Review outcome (required — see AGENTS.md)
Reviewed against the five dimensions in
.github/instructions/code-review.instructions.md.npm run lintclean.npm test— 780 passing, 0 failing.1 [fix here] · 1 [follow-up] — the [fix here] was fixed before this PR was opened.
_logsonly beforesave_cache. That leaves the already-poisoned cache entry in place, since the save does not overwrite an existing key, so builds would have stayed red until the lockfile changed again. Moved the primary cleanup to just afterrestore_cache.vendorstays out of the asar. A packaging assertion would need a build in CI rather than a unit test; noted in Risks above rather than filed, as it is one assertion on a directory that no longer exists in the app.Considered and cleared: no new dependency, no host binary, nothing crossing IPC, no spawn, no path composition, and
rm -rftargets a literal quoted path set two lines above underset -u. The bash script runs on both the macOS and Windows agents and uses nothing platform-specific.Implementation notes
The negation-only
filesbehaviour is not folklore —app-builder-lib/out/fileMatcher.js:containsOnlyIgnore()is true when every pattern starts with!, so the default include is prepended and the exclusion is layered on top. The same function adds electron-builder's own exclusions in exactly the!${dir}{,/**/*}form used here.Local verification, on this branch versus without the
fileskey, with a stand-in cache staged:/vendorentries inapp.asarfilesfilesThe 6-entry difference is exactly the staged cache. The byte difference is trivial here only because the stand-in cache is a few hundred bytes; on a CI agent it is the full package cache.