Uh oh!
There was an error while loading. Please reload this page.
fix(linux): give the capture helper's ffmpeg libraries their own directory name - #300
Merged
Merged
Conversation
…ctory name Three artifacts claimed electron/native/bin/linux-x64/ffmpeg: - fetch-ffmpeg.mjs vendors the static ffmpeg EXECUTABLE there (PINNED["linux-x64"].exe) - build-linux-pipewire-helper.mjs mkdir'd it as a DIRECTORY for the helper's unrenamed libav*/libsw*.so, the name its $ORIGIN/ffmpeg RUNPATH is compiled against - audioPeaks.ts resolves that same path expecting a binary to spawn Whoever ran last won, and every loser failed obliquely: `EEXIST: mkdir .../linux-x64/ffmpeg` from the helper build, `spawn … EACCES` from audioPeaks when it found a directory where a binary belonged. CI never saw it because build:linux runs only fetch:ffmpeg:sdk on a fresh checkout — a flag that exists, per its own comment, to dodge this very collision. The helper's directory moves to helper-ffmpeg/ rather than the executable moving, because a bare `ffmpeg` means the executable to every other reader of this tree (win32 has ffmpeg.exe at the same level), and a directory of shared objects under that name is a lie about its contents. RUNPATH follows in build.rs. The staleness/payload guard in before-pack.cjs follows too, and its file-instead-of-directory branch loses the explanation of a collision that can no longer happen. Also corrects the --sdk-only comment, which justified itself with that collision, and the claim in assertLgpl's note that nothing in the app spawns the ffmpeg CLI any more — audioPeaks does, falling back to the browser decode paths when it is absent, which on Linux is always. Whether the CLI should ship on Linux at all is a separate question (~110 MB) and this change does not touch it.
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughLinux FFmpeg shared libraries now use ChangesLinux FFmpeg packaging path
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Follow-up to #298, which added a
beforePackguard that detects this collision. This removes it.Three artifacts claimed
electron/native/bin/linux-x64/ffmpeg, not two:scripts/fetch-ffmpeg.mjsPINNED["linux-x64"].exe)scripts/build-linux-pipewire-helper.mjslibav*/libsw*.so, the name its$ORIGIN/ffmpegRUNPATH is compiled againstelectron/media/audioPeaks.tsWhoever ran last won, and every loser failed obliquely:
Neither message names the real problem. CI never sees it because
build:linuxruns onlyfetch:ffmpeg:sdkon a fresh checkout — and that flag exists, per its own comment, to dodge this very collision.Which side moved, and why
The helper's directory, to
helper-ffmpeg/. A bareffmpegmeans the executable to every other reader of this tree — Windows hasffmpeg.exeat the same level, andaudioPeaks.tsalready carries anisExecutableFile()guard and a test written after this directory fooledexistsSync. A directory of shared objects under that name is a lie about its contents.Changed: the RUNPATH in
build.rs,stageFfmpeg's target, the payload guard inbefore-pack.cjs(its file-instead-of-directory branch loses the explanation of a collision that can no longer happen), and the comments inelectron-builder.json5,fetch-ffmpeg.mjsand bothaudioPeaksfiles.linux.extraResources'filter: ["linux-*/**"]already matches at any depth, so packaging needs no change — only its comment.Two things found while checking the consumers
The
--sdk-onlyrationale is now partly obsolete. It justified itself with this collision. What survives is a size argument: the static CLI is ~110 MB andlinux.extraResourceshas no exclusion for it (unlike Windows'"!win32-*/ffmpeg.exe"). Behaviour is unchanged here — whether the CLI should ship on Linux is a separate call with a real price tag.assertLgpl's note says "nothing in the app spawns ffmpeg any more". That is false.audioPeaks.ts:301resolves and spawns it, ~6× faster than the renderer's own decode. It degrades gracefully —getAudioPeaksreturns null and the renderer falls back to its browser pipelines — so on Linux, where the CLI does not ship, that fallback is always the one taken. Slower on first decode, cached afterwards, not broken. Comment corrected; the shipping decision left alone.Related issue
No issue — follow-up to #298.
Type of change
Release impact
Desktop impact
Screenshots / video
n/a
Testing
Ubuntu 24.04 x86_64. Both orders now work from a clean
electron/native/bin/linux-x64/— each of them used to be a hard failure:fetch:ffmpeg→build:native:linuxEEXIST: mkdir .../ffmpeg{"event":"ready","pipewireVersion":"1.0.5"}build:native:linux→fetch:ffmpegspawnSync … EACCESLGPL verifiedResulting layout, with the two artifacts side by side:
The RUNPATH actually moved and actually resolves — not just the staging path:
npm run build:native:compositor:linuxstill passes its own check (Verified: no unprefixed ffmpeg imports remain in the addon) — the addon reads$ORIGIN, sohelper-ffmpeg/is invisible to it.node scripts/before-pack.cjson the renamed layout: exit 0 when complete, and still exit 1 with the right message whenhelper-ffmpeg/is emptied.npm run test: 1678 passed, 140 files, 0 failed.npx tsc --noEmitclean,biome checkclean.audioPeaks.test.tsis 13/13 — that file previously failed in a worktree with theEACCESabove, because the path it resolves held the helper's libraries instead of a binary.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation