Patch fs-ext-extra-prebuilt to remove other-platform binaries on install#2615
Conversation
Applies upstream fix (adamziel/fs-ext-prebuilt#6) via patch-package to remove binaries for other platforms after a successful prebuilt load. This prevents Windows code-signing tools from encountering Linux/macOS .node files they cannot process.
| "start": "npm -w studio-app run start", | ||
| "start-wayland": "npm -w studio-app run start-wayland", | ||
| "postinstall": "patch-package --patch-dir apps/cli/patches && patch-package --patch-dir apps/studio/patches && ts-node ./scripts/download-wp-server-files.ts && node ./scripts/download-available-site-translations.mjs", | ||
| "postinstall": "patch-package && patch-package --patch-dir apps/cli/patches && patch-package --patch-dir apps/studio/patches && ts-node ./scripts/download-wp-server-files.ts && node ./scripts/download-available-site-translations.mjs", |
There was a problem hiding this comment.
@fredrikekelund this packages is dependency for @studio/common, benchmark-site-editor and studio-cli. With the monorepo setup, is applying that in the prokject root's node_modules/ a way to go?
There was a problem hiding this comment.
Good question. This is probably one of the biggest gotchas with our monorepo setup now.
As part of the packaging flow, we now run the install:bundle npm scripts in apps/cli and apps/studio. They do this:
npm install [--omit=dev] --no-package-lock --no-progress --install-links --no-workspaces
The goal is to produce self-contained apps/cli/node_modules and apps/studio/node_modules directories, because those are copied into the packaged output.
This goal conflicts with how dependency management typically works with our npm workspaces setup, which is that most (not all) dependencies are hoisted to the root node_modules directory.
So, how does this affect patches? Well, patch-package fails if it finds a patch without a matching dependency. That's why we have separate apps/cli/patches and apps/studio/patches directories, because otherwise the install:bundle npm scripts would fail.
To answer your question directly: just put the patch in apps/cli/patches. fs-ext-extra-prebuilt is a dev dependency of @studio/common, so it won't be included in the self-contained packaged node_modules directories.
There was a problem hiding this comment.
Thanks for the clarification. I moved it, and it works fine.
fredrikekelund
left a comment
There was a problem hiding this comment.
LGTM 👍 I've triggered a Windows dev build on Buildkite. Let's see how that fares before we land this
📊 Performance Test ResultsComparing cef34dc vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
…cript The patch-package approach patched install.js but that script had already run before patch-package executes, so no binaries were actually removed. Instead, run a dedicated script from postinstall that deletes non-current- platform binaries from fs-ext-extra-prebuilt/binaries directly, preventing Windows code-signing from encountering darwin/linux .node files.
| "start": "npm -w studio-app run start", | ||
| "start-wayland": "npm -w studio-app run start-wayland", | ||
| "postinstall": "patch-package --patch-dir apps/cli/patches && patch-package --patch-dir apps/studio/patches && ts-node ./scripts/download-wp-server-files.ts && node ./scripts/download-available-site-translations.mjs", | ||
| "postinstall": "patch-package --patch-dir apps/cli/patches && patch-package --patch-dir apps/studio/patches && node ./scripts/remove-fs-ext-other-platform-binaries.mjs && ts-node ./scripts/download-wp-server-files.ts && node ./scripts/download-available-site-translations.mjs", |
There was a problem hiding this comment.
It didn't work with the patch, as the patch was applied after the package was installed. I moved the logic to custom scripts as a hotfix.
We can remove those when the library is fixed.
|
|
||
| # Rename NuGet package files with generic name | ||
| $artifactsPath = Get-Item ".\out" | Select-Object -ExpandProperty FullName | ||
| $artifactsPath = Get-Item ".\apps\studio\out" | Select-Object -ExpandProperty FullName |
There was a problem hiding this comment.
We missed that during the monorepo change.
| - apps\studio\out\**\studio-setup.exe | ||
| - apps\studio\out\**\studio-update.nupkg | ||
| - apps\studio\out\**\RELEASES | ||
| - apps\studio\out\**\*.appx |
There was a problem hiding this comment.
Those are the remaining changes that follow the monorepo change.
Related issues
Proposed Changes
.nodebinaries for other platforms during postinstall and bundle stepsout/pathsTesting Instructions
npm installand verify no errors.nodefiles fromfs-ext-extra-prebuiltPre-merge Checklist