Stop apt-installing Playwright's system deps in Software Factory CI - #6015
Stop apt-installing Playwright's system deps in Software Factory CI#6015backspace wants to merge 2 commits into
Conversation
`playwright install-deps` fetches 182 packages (114 MB) from Ubuntu's mirrors, and that half of the browser install is the one that stalls. It has held a runner for as long as 170 minutes; bounded by the five-minute timeout added to contain that, it still burned five Software Factory shards in a single day, each dying before a single test ran and each passing on a plain re-run. The packages are fonts and GStreamer media codecs. These specs use Playwright's default Chromium, play no media and render no non-Latin text, and the runner image already carries the libraries Chromium links against — the Matrix suite in ci.yaml has always run Chromium with no deps step at all. Measured rather than assumed: with the step removed, all three shards pass 62 tests, the same count as with it, and no missing-library error appears. The install step goes from a five-minute timeout to about 22 seconds. Reinstating it is the fix if a future spec needs media playback or CJK glyphs; that would present as a missing codec or tofu boxes, not as a browser that cannot launch.
Bare `playwright install` fetches Chromium, Firefox, WebKit and FFmpeg. The Software Factory config declares no projects, so Playwright's default applies and every spec runs on Chromium; Firefox and WebKit are downloaded and never launched. Removing the apt step made that visible. Playwright's host validation warns about libraries missing for the browsers it has downloaded, so a run now prints a boxed "Host system is missing dependencies to run browsers" listing libgtk-4, libgraphene, libgst* and libflite* — all of them WebKit and Firefox dependencies. The suite passes regardless, but the warning reads like a broken install rather than two unused browsers, and the next person to see it will reasonably suspect the missing apt step. Naming chromium drops both the download and the warning.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review went after one question: with the apt step gone and the install narrowed to Chromium, does anything this job launches lose a binary or a library it needs — and did the narrowing land on both workflows that install Playwright browsers. I took the stability argument as carried by the passing shards rather than re-litigating it.
No blocking issues. playwright install chromium still lands the headless shell that preflight.ts demands and that everything in this job actually launches, and the harness's puppeteer path takes the runner image's system Chrome through PUPPETEER_EXECUTABLE_PATH, so it never leaned on the deps step either.
Recommendations:
- The Matrix job in
ci.yamlis the twin of this change and hasn't moved. It runs barepnpm exec playwright installagainst a config whose only project ischromium(packages/matrix/playwright.config.ts, andscripts/test.shpasses no--project), so it downloads Firefox and WebKit that no spec there launches — and having no deps step either, it meets the same conditions your second commit names as producing the missing-libgtk/libgstwarning. Either the same one-word narrowing there, or an explicit "not this PR", so the next person reading that warning isn't sent hunting for a missing apt step. --only-shellwould drop the full Chrome for Testing build as well, and would make CI name the same target asfactory-setup.tsandpreflight.ts— see the comment on therun:line.- The step comment's measurements compare against a step this PR deletes and rot on the next spec added — see the comment on the comment block.
- The ten-minute ceiling lost the sentence that explained it — see the comment on
timeout-minutes.
Generated by Claude Code
| # Ubuntu's mirrors, and that half is what stalls: it has held a runner | ||
| # for as long as 170 minutes, and even bounded by a timeout it burned | ||
| # five Software Factory shards in a single day, each failing before any | ||
| # test ran. | ||
| # | ||
| # A healthy install of both takes about a minute, so five is generous. | ||
| # The deps half is still required: without it Playwright reports "Host | ||
| # system is missing dependencies to run browsers". | ||
| # What it installs is fonts and GStreamer media codecs. These specs use | ||
| # Playwright's default Chromium and neither play media nor render | ||
| # non-Latin text, and the runner image already carries what Chromium | ||
| # links against — the Matrix suite in ci.yaml has always run Chromium | ||
| # with no deps step. Measured over all three shards without it: 62 | ||
| # tests pass, the same as with it, and the step takes ~22s rather than | ||
| # timing out at five minutes. |
There was a problem hiding this comment.
[Claude Code 🤖] Non-blocking, this PR.
Roughly half of this comment measures the change rather than stating the contract the step keeps, and it rots on the next spec added. 62 tests pass, the same as with it and ~22s rather than timing out at five minutes both compare against a step that stops existing when this lands, so a reader a year out has nothing to anchor them against — and the count is wrong the moment a spec is added. burned five Software Factory shards in a single day and has always run Chromium are the same class.
The durable half is already written: what install-deps pulls (fonts and GStreamer codecs), that these specs neither play media nor render non-Latin text, that the runner image carries the libraries Chromium links against with the Matrix job in ci.yaml as the standing demonstration, and the symptom that would justify reinstating the step. Suggest keeping that and cutting the before/after numbers — the incident count and the measurements are what the commit message is for, per the evergreen-comments skill.
Generated by Claude Code
| # If a future spec needs media playback or CJK glyphs, the failure will | ||
| # be a missing codec or tofu boxes rather than a browser that cannot | ||
| # launch — reinstate the deps step for that, ideally cached. | ||
| timeout-minutes: 10 |
There was a problem hiding this comment.
[Claude Code 🤖] Non-blocking, this PR.
The sentence that justified this ceiling left with the deps half ("a healthy install of both takes about a minute, so five is generous"), leaving an unexplained ten minutes bounding a step whose only remaining work is a CDN fetch that the previous comment recorded as never having been implicated in a stall. In a file where every other knob carries its rationale, this one now reads as arbitrary — either tighten it to something near the observed cost so a wedged download fails fast rather than holding a runner, or add the clause saying what the ten minutes is protecting against.
Generated by Claude Code
| # libraries (libgtk-4, libgst*, libflite*) that Playwright's host | ||
| # validation then warns are missing, which reads alarmingly like a | ||
| # broken install rather than two unused browsers. | ||
| run: pnpm exec playwright install chromium |
There was a problem hiding this comment.
[Claude Code 🤖] Non-blocking; this PR or a follow-up, your call.
The argument that drops Firefox and WebKit also drops the full Chrome for Testing build. In playwright-core's registry, the Chromium browser type resolves to chromium-headless-shell whenever no channel is set and headless is true, so every launch in this job lands on the shell: the Playwright specs, boxel test's chromium.launch({ headless: true }) in packages/boxel-cli/src/lib/test-engine.ts, and screenshot-execution.ts. Nothing under packages/software-factory or packages/boxel-cli sets a channel or headless: false, so the full browser this step downloads is never launched here — the headed script runs on a developer's machine, not on this runner.
pnpm exec playwright install --only-shell chromium installs the shell alone, and it is also what the local path already asks for: scripts/factory-setup.ts installs chromium-headless-shell and preflight.ts refuses to run the factory unless a chromium_headless_shell-* build is present. As written, CI and the factory's own setup name different install targets for the same requirement.
Generated by Claude Code
2 participants
While this doesn’t noticeably improve software factory job run times, it does reduce dependency installation, which should help with stability. See the installation on
mainvs on this branch for comparison: 1200+ lines vs 80 lines.Claude:
playwright install-depsfetches 182 packages (114 MB) from Ubuntu's mirrors, and that half of the browser install is the one that stalls. It has held a runner for as long as 170 minutes; bounded by the five-minute timeout added to contain that, it still burned five Software Factory shards in a single day, each dying before a single test ran and each passing on a plain re-run.The packages are fonts and GStreamer media codecs. These specs use Playwright's default Chromium, play no media and render no non-Latin text, and the runner image already carries the libraries Chromium links against — the Matrix suite in ci.yaml has always run Chromium with no deps step at all.
Measured rather than assumed: with the step removed, all three shards pass 62 tests, the same count as with it, and no missing-library error appears. The install step goes from a five-minute timeout to about 22 seconds.
Reinstating it is the fix if a future spec needs media playback or CJK glyphs; that would present as a missing codec or tofu boxes, not as a browser that cannot launch.