Skip to content

Fix generic Linux runtime deps for RPM packaging#1831

Open
yuzhichang wants to merge 2 commits intogeneralaction:mainfrom
yuzhichang:feature/generic-rpm-packaging
Open

Fix generic Linux runtime deps for RPM packaging#1831
yuzhichang wants to merge 2 commits intogeneralaction:mainfrom
yuzhichang:feature/generic-rpm-packaging

Conversation

@yuzhichang
Copy link
Copy Markdown

Summary

  • extract shared electron-builder config into a single base factory for stable and canary builds
  • inject Linux runtime production dependencies after pack so rpm/deb/appimage builds work with hoisted pnpm node_modules layouts
  • add package homepage metadata required by rpm packaging

Validation

  • mise exec node@24.14.0 -- pnpm run build:main
  • mise exec node@24.14.0 -- pnpm run typecheck
  • mise exec node@24.14.0 -- node --experimental-strip-types scripts/release/rebuild-native.ts --arch x64
  • mise exec node@24.14.0 -- pnpm exec electron-builder --linux rpm --x64 --publish never --config electron-builder.config.ts --config.npmRebuild=false
  • installed rebuilt rpm locally and verified app startup no longer fails with ERR_MODULE_NOT_FOUND for better-sqlite3

Notes

  • local pnpm test is not fully green on current upstream main due to pre-existing baseline issues unrelated to this packaging change, including the pr-merge-line expectation mismatch and an existing legacy-port task import test failure

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 30, 2026

Greptile Summary

This PR fixes Linux RPM packaging by extracting a shared electron-builder.base.ts factory (eliminating duplicated config between stable and canary), adding a homepage field required by RPM metadata, and introducing a new afterPack hook (linux-runtime-node-modules.ts) that re-populates the packaged app's node_modules from production dependencies to work around pnpm's hoisted layout.

  • P1 — copyPackageFiles filter uses wrong parameter: The fs.copy filter receives (src, dest), but the code names the first param _source (ignored) and uses destinationLike (the dest path) for path.relative(sourceDir, destinationLike). Because every destination lives under a path containing 'node_modules', the filter returns false for all items — no package files are actually copied, leaving the injected node_modules empty.

Confidence Score: 3/5

Not safe to merge as-is — the core injection mechanism has a logic bug that prevents any files from being copied into the runtime node_modules.

A single P1 logic bug in the new copyPackageFiles filter would cause the entire node_modules injection to silently produce empty package directories, negating the fix. The author's validation passed, which may mean the current pnpm hoisted layout happens to avoid the code path where the filter fires on real files, but the filter is still wrong by construction and will be fragile across environments. The rest of the PR (config refactor, homepage field) is clean.

scripts/release/linux-runtime-node-modules.ts — specifically the copyPackageFiles filter at lines 88–97

Important Files Changed

Filename Overview
scripts/release/linux-runtime-node-modules.ts New afterPack hook that re-creates Linux runtime node_modules from production deps; contains a P1 filter-parameter bug in copyPackageFiles that causes the wrong path to be used for node_modules exclusion, likely copying no files.
electron-builder.base.ts New shared factory that eliminates duplicated electron-builder config between stable and canary builds; clean refactor with no issues.
electron-builder.config.ts Stable build config simplified to delegate to createElectronBuilderConfig; straightforward and correct.
electron-builder.canary.config.ts Canary build config simplified to delegate to createElectronBuilderConfig; straightforward and correct.
package.json Adds homepage field required by RPM packaging metadata; minimal, correct change.

Sequence Diagram

sequenceDiagram
    participant EB as electron-builder
    participant AP as afterPack hook
    participant LR as injectLinuxRuntimeNodeModules
    participant FS as fs-extra

    EB->>AP: afterPack(context) [Linux x64, per target]
    AP->>LR: isLinuxPack(context)?
    alt not Linux
        LR-->>AP: return (no-op)
    end
    LR->>FS: readJson(projectDir/package.json)
    FS-->>LR: { dependencies, optionalDependencies }
    LR->>FS: remove(runtimeNodeModulesDir)
    LR->>FS: ensureDir(runtimeNodeModulesDir)
    loop each root production dependency
        LR->>LR: resolveInstalledPackageDir(projectDir, pkg)
        LR->>LR: copyRuntimePackageTree(pkg, srcDir, destDir, ancestry)
        note over LR: copyPackageFiles(srcDir, destDir) ⚠ filter uses dest path instead of src path
        LR->>FS: copy(srcDir, destDir, { filter })
        loop each transitive dependency
            LR->>LR: resolveInstalledPackageDir(realPkgDir, dep)
            LR->>LR: copyRuntimePackageTree(dep, depDir, depDest, nextAncestry)
        end
        LR->>FS: writeJson(destDir/package.json) [scripts removed]
    end
    LR-->>AP: done
    AP-->>EB: done
Loading
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
scripts/release/linux-runtime-node-modules.ts:88-97
**`copyPackageFiles` filter uses destination path instead of source path**

The filter function uses `destinationLike` (the second `fs.copy` filter argument — the *destination* path) for computing `rel`, but `sourceDir` is the source root. `path.relative(sourceDir, destinationLike)` will never equal `''` (since `sourceDir ≠ destinationDir`) and will always contain `'node_modules'` in the result (because every `destinationLike` lives under `runtimeNodeModulesDir` which itself includes `'node_modules'` in its absolute path). This causes the filter to return `false` for every item passed to it — effectively preventing any package files from being copied.

The first parameter (`_source`) — the actual source path — is what should be used:

```typescript
filter: (source, _destinationLike) => {
  const rel = path.relative(sourceDir, source);
  if (rel === '') {
    return true;
  }
  return !rel.split(path.sep).includes('node_modules');
},
```

### Issue 2 of 3
scripts/release/linux-runtime-node-modules.ts:152-178
**`afterPack` hook runs for every Linux target, not once per platform**

`afterPack` fires once per (platform × arch × target) combination. With three Linux targets (AppImage, deb, rpm, all x64), `injectLinuxRuntimeNodeModules` will run three times if electron-builder creates a separate `appOutDir` per target. Each invocation removes and recreates the full `node_modules` tree — a potentially slow full copy of all production dependencies. Consider adding an early-exit guard to only inject once, or switch to a platform-level hook if electron-builder supports it.

### Issue 3 of 3
scripts/release/linux-runtime-node-modules.ts:169-177
**Top-level optional dep resolution errors abort the hook**

`resolveInstalledPackageDir` throws if a package cannot be resolved. The `optionalDependencies` loop inside `copyRuntimePackageTree` handles this with a try/catch, but the top-level loop in `injectLinuxRuntimeNodeModules` does not — an unavailable optional dep would abort the entire `afterPack` hook. Consider applying the same try/catch guard for optional deps at the top level.

Reviews (1): Last reviewed commit: "Fix generic Linux runtime deps for RPM p..." | Re-trigger Greptile

Comment thread scripts/release/linux-runtime-node-modules.ts
Comment thread scripts/release/linux-runtime-node-modules.ts
Comment thread scripts/release/linux-runtime-node-modules.ts Outdated
@yuzhichang
Copy link
Copy Markdown
Author

Addressed the open review threads in c02d7f27:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant