Fix GitHub Actions#86
Open
JustLikeCheese wants to merge 6 commits into
Open
Conversation
The Windows MSIX is extracted via 7z, which preserves URL-encoded names like %40worklouder/ for the @worklouder scoped npm package. The asar header still references the originals, so the subsequent asar extract crashes with ENOENT when resolving unpacked .node binaries. Walk app.asar.unpacked/ after extraction and decodeURIComponent every %XX-encoded entry name. Logs how many it renamed. Also pulls package-lock.json up to the current package.json version so the lockfile doesn't churn on every npm install.
Electron 42 ships a v8 that changed v8::External::Value() to require an ExternalPointerTypeTag argument. better-sqlite3 <= 12.9.0 calls it 0-arg and fails to compile during electron-rebuild on Linux: ../src/util/macros.cpp:30:76: error: no matching function for call to 'v8::External::Value()' 12.10.1 fixes it (release notes: "Fix V8 external API usage for Electron 42 by @tstone-1 in #1475"). No code changes here — just a caret bump, lockfile follows.
Linux build was silently passing with zero artifacts: electron-forge make
exits 0 after the Packaging stage's "Finalizing package" line without ever
reaching the Make stage, and upload-artifact@v7 defaults if-no-files-found
to warn so the job stays green. We can't reproduce the forge silent-exit
locally on Windows, so this commit ships the minimum changes needed to
make the next CI run produce actionable evidence and refuse to ship broken
packages:
- DEBUG=electron-forge:*,electron-packager,@electron/packager* on the Linux
build step so the next run logs what forge is actually doing between
package and make.
- Post-build sanity step that lists out/ and out/make/ contents and fails
the job hard if zero .deb/.rpm/.zip were produced (replacing the silent
upload warning with a loud, structured ::error).
- if-no-files-found: error on every upload-artifact step (Linux, macOS,
Windows) so any future zero-files scenario fails red.
- Expand Linux apt install with build-essential, python3, libudev-dev so
the better-sqlite3 / node-pty source-compile fallback works if
prebuild-install ever misses.
- Split rebuild:native into per-arch scripts (rebuild:native:x64 /
:arm64) and wire build:linux-{x64,arm64} to the matching one. Defensive
on x64; correctness fix for the arm64 leg, which previously ran
electron-rebuild without --arch on an x64 host and produced wrong-arch
native .node files inside the arm64 package.
- Harden prepare-src.js: on Linux, refuse to fall through when the
@cometix/codex or rg vendor binary can't be resolved. Previously we
logged "[!] keeping upstream" and shipped the macOS Mach-O binary
inside a Linux .deb/.rpm.
…cript
electron-forge make exits cleanly with status 0 in CI between the Packaging
stage's "Finalizing package" listr line and ever reaching the Make stage —
no error, no unhandledRejection, no stack trace. The previous commit's
DEBUG output confirmed @electron/packager logs "Extracting electron.zip"
then the process exits 40ms later without writing anything to out/. Two
runs reproduced this deterministically. Rather than continue chasing a
silent listr2/packager interaction, switch Linux to the same pattern
mac/win already use: a custom build script with explicit promises and
loud error handling.
- New scripts/build-from-upstream-linux.js calls @electron/packager
directly (no listr2 wrapper, no hidden lifecycle), then invokes
MakerDeb / MakerRpm / MakerZip via their programmatic API. Per-maker
errors are isolated and reported; partial-success still exits 1.
- The afterCopy hook inlines forge.config.js's packageAfterCopy logic so
the same Linux-only file/dir skip rules apply (no Mach-O native dir,
no app.asar, no .lproj).
- Wires build:linux-{x64,arm64} npm scripts to call the new script
instead of electron-forge make.
- Drops the post-make diagnostic step from build.yml now that the new
script's own output is structured and loud. if-no-files-found:error
on upload-artifact is still the final safety net.
forge.config.js is left untouched — mac/win still go through their
existing build-from-upstream.js path and don't touch forge make either,
so the file only matters for `npm run forge:package` / `forge:make`
direct invocations which we don't use in CI.
… + asar
The previous attempt's @electron/packager call exits the Node process
silently with code 0 in the middle of extractElectronZip() — extract-zip
2.0.1 (the unmaintained upstream archived in 2021) is the culprit. CI
log shows packager logs "Packaging app for platform linux x64 using
electron v42.0.1" then the process ends 37 ms later with no error, no
unhandled rejection, no maker output. Reproduced on three runs.
Rather than fight extract-zip on Node 24, bypass it: download the
Electron Linux template zip via @electron/get directly, extract with
the system unzip CLI (preinstalled on ubuntu-latest), repack src/ to
app.asar via @electron/asar, and copy Linux resources from src/mac-{arch}/
inline. Then invoke maker-deb/rpm/zip programmatically as before. Each
phase logs its progress so any future regression is loud, and `unzip` is
a battle-tested POSIX tool with no Node interop surprises.
No changes to forge.config.js — it's unused on the Linux path now.
apt deps added in the previous commit (build-essential, python3,
libudev-dev) stay for the maker-deb/rpm peer-package compile path. The
`unzip` CLI is preinstalled on ubuntu-latest.
Last run: arm64 RPM is green (3/3), x64 RPM fails on rpmbuild's brp-strip
pass at:
resources/cua_node/lib/node_modules/@oai/sky/bin/linux/sky_linux_arm64
/usr/bin/strip: Unable to recognise the format of the input file ...
Upstream Codex ships both sky_linux_x64 and sky_linux_arm64 side-by-side.
The host's x86_64 strip can parse x64 ELF (so arm64 RPMs build fine on
the x64 runner — confirmed in the last run), but not arm64 ELF — so
the x64 RPM blows up. deb and zip don't run a strip pass so they were
fine all along.
Before any maker runs, delete the cross-arch sky binary from
<packageDir>/resources/. The runtime selector only ever invokes
sky_linux_${process.arch}, so the other-arch copy is dead weight on the
end-user machine; removing it also slims the .deb and .zip slightly.
Symmetric: arm64 build drops sky_linux_x64, x64 build drops sky_linux_arm64.
Reviewer's GuideRefactors Linux build to bypass @electron/packager on CI, tightens Linux packaging safety, fixes Windows MSIX percent-encoded paths, hardens Windows version detection, and makes the GitHub Actions workflows fail-fast when artifacts or dependencies are missing. Sequence diagram for Windows syncWin MSIX path decodingsequenceDiagram
participant syncWin
participant decodePercentNames
participant assembleOutput
syncWin->>decodePercentNames: decodePercentNames(app.asar.unpacked)
decodePercentNames-->>syncWin: [percent-encoded paths decoded]
syncWin->>assembleOutput: assembleOutput(resourcesDir, destDir, Windows)
assembleOutput-->>syncWin: info
syncWin-->>syncWin: return info
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
decodePercentNames, consider handling the case where the decoded name already exists in the target directory before callingfs.renameSync, otherwise a collision between encoded/decoded names could throw and abort processing of the rest of the tree. - In
build-from-upstream-linux.js, it might be safer to explicitly check for the presence of theunzipbinary (or wrapexecFileSync('unzip', ...)with a clearer error) so that CI or local environments withoutunzipfail with a more actionable message.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `decodePercentNames`, consider handling the case where the decoded name already exists in the target directory before calling `fs.renameSync`, otherwise a collision between encoded/decoded names could throw and abort processing of the rest of the tree.
- In `build-from-upstream-linux.js`, it might be safer to explicitly check for the presence of the `unzip` binary (or wrap `execFileSync('unzip', ...)` with a clearer error) so that CI or local environments without `unzip` fail with a more actionable message.
## Individual Comments
### Comment 1
<location path="scripts/sync-upstream.js" line_range="220-229" />
<code_context>
return info;
}
+function decodePercentNames(root) {
+ if (!fs.existsSync(root)) return;
+ let renamed = 0;
+ const walk = (dir) => {
+ for (const e of fs.readdirSync(dir, { withFileTypes: true })) {
+ const cur = path.join(dir, e.name);
+ let next = cur;
+ if (/%[0-9A-Fa-f]{2}/.test(e.name)) {
+ try {
+ const decoded = decodeURIComponent(e.name);
+ if (decoded !== e.name) {
+ next = path.join(dir, decoded);
+ fs.renameSync(cur, next);
+ renamed++;
+ }
+ } catch {
+ /* keep as-is on malformed sequences */
+ }
+ }
+ if (e.isDirectory()) walk(next);
+ }
+ };
+ walk(root);
+ if (renamed) console.log(` [decode] renamed ${renamed} percent-encoded path(s)`);
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle potential name collisions when decoding percent-encoded filenames.
On Windows, `fs.renameSync` will throw if the decoded target already exists, which will abort `syncWin`. Consider checking `fs.existsSync(next)` before renaming and either skipping, logging, or resolving the conflict (e.g., by appending a suffix) so a single collision doesn’t stop the whole sync.
</issue_to_address>
### Comment 2
<location path="scripts/build-from-upstream-linux.js" line_range="50-59" />
<code_context>
+function copyLinuxResources(srcDir, destDir) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Preserve original file permissions instead of chmod 755 for every copied file.
`chmodSync(..., 0o755)` is being applied to all copied files, which incorrectly makes non-executables (e.g., images, text files) executable and may conflict with upstream packaging expectations. Please preserve the source file mode (e.g., via `fs.statSync(srcPath).mode`) and only enforce 755 for known executables, or at least restrict chmod to files that were already executable.
Suggested implementation:
```javascript
const srcStat = fs.statSync(srcPath);
let mode = srcStat.mode;
// If the source file was executable, normalize to 755.
// Otherwise, preserve the original (non-executable) mode.
if ((mode & 0o111) !== 0) {
mode = 0o755;
}
fs.chmodSync(destPath, mode);
```
If there are other places in this file (or nearby scripts) that use `fs.chmodSync(..., 0o755)` for copies performed by `copyLinuxResources` or similar helpers, you should apply the same pattern there: compute `srcStat = fs.statSync(srcPath)`, derive `mode` from `srcStat.mode`, and only enforce `0o755` when the source file was already executable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+220
to
+229
| function decodePercentNames(root) { | ||
| if (!fs.existsSync(root)) return; | ||
| let renamed = 0; | ||
| const walk = (dir) => { | ||
| for (const e of fs.readdirSync(dir, { withFileTypes: true })) { | ||
| const cur = path.join(dir, e.name); | ||
| let next = cur; | ||
| if (/%[0-9A-Fa-f]{2}/.test(e.name)) { | ||
| try { | ||
| const decoded = decodeURIComponent(e.name); |
There was a problem hiding this comment.
issue (bug_risk): Handle potential name collisions when decoding percent-encoded filenames.
On Windows, fs.renameSync will throw if the decoded target already exists, which will abort syncWin. Consider checking fs.existsSync(next) before renaming and either skipping, logging, or resolving the conflict (e.g., by appending a suffix) so a single collision doesn’t stop the whole sync.
Comment on lines
+50
to
+59
| function copyLinuxResources(srcDir, destDir) { | ||
| if (!fs.existsSync(srcDir)) { | ||
| throw new Error(`Source platform dir not found: ${srcDir}`); | ||
| } | ||
| const skip = new Set(["_asar"]); | ||
| for (const f of MACOS_ONLY_FILES) skip.add(f); | ||
| for (const d of MACOS_ONLY_DIRS) skip.add(d); | ||
|
|
||
| let copied = 0; | ||
| const copyDir = (s, d) => { |
There was a problem hiding this comment.
suggestion (bug_risk): Preserve original file permissions instead of chmod 755 for every copied file.
chmodSync(..., 0o755) is being applied to all copied files, which incorrectly makes non-executables (e.g., images, text files) executable and may conflict with upstream packaging expectations. Please preserve the source file mode (e.g., via fs.statSync(srcPath).mode) and only enforce 755 for known executables, or at least restrict chmod to files that were already executable.
Suggested implementation:
const srcStat = fs.statSync(srcPath);
let mode = srcStat.mode;
// If the source file was executable, normalize to 755.
// Otherwise, preserve the original (non-executable) mode.
if ((mode & 0o111) !== 0) {
mode = 0o755;
}
fs.chmodSync(destPath, mode);If there are other places in this file (or nearby scripts) that use fs.chmodSync(..., 0o755) for copies performed by copyLinuxResources or similar helpers, you should apply the same pattern there: compute srcStat = fs.statSync(srcPath), derive mode from srcStat.mode, and only enforce 0o755 when the source file was already executable.
|
I tested a very similar fix set on a native Ubuntu 24.04 arm64 host and wanted to confirm that the overall direction here is correct. What matched the root causes we reproduced locally:
What we validated locally:
Two follow-ups I would still recommend before merge:
Overall: this PR is moving in the right direction and it matches the failures we reproduced locally on arm64. |
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
2 participants
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.
修复 Linux 平台的 GitHub Actions
修复 Windows MSIX %-encoded 路径解码问题
增强 getWindowsVersion()，加了 4 次重试
Ciallo～(∠・ω< )⌒★!
Linux 的平台喔还没测试，Linux 佬可以试试
https://github.com/JustLikeCheese/CodexDesktop-Rebuild/actions/runs/27545680480
Summary by Sourcery
Fix CI build failures on Linux and Windows by replacing the Linux packaging pipeline with a custom Electron-based builder and improving Windows packaging and version checks.
New Features:
Bug Fixes:
Enhancements:
CI: