fix(build): align Rollup native packages with supported platforms#337
Merged
shantur merged 3 commits intoNeuralNomadsAI:devfrom Apr 19, 2026
Merged
Conversation
Declare @rollup/rollup-win32-x64-msvc as an explicit optional workspace dependency so fresh Windows checkouts and worktrees materialize the native Rollup package at install time instead of failing at runtime.
Contributor
Author
|
I want you to perform the gatekeeper checks for latest PR state check for regressions |
Contributor
|
Gatekeeper review for the latest PR state. What I checked
Findings
Why this matters: the committed lockfile will churn on the next normal lockfile refresh, which is exactly the kind of non-deterministic package metadata we should block in review. Recommendation: regenerate the root lockfile from the updated
Why this matters: if the stated goal is to make "Windows installs" reliable in general, this PR only fixes the x64 MSVC subset. That may be intentional, but then the scope should be described narrowly. Recommendation: either narrow the PR language to explicitly say "Windows x64 MSVC installs" or expand the approach to cover the Windows variants the project intends to support. Notes
-- |
Regenerate the root lockfile after adding the Windows Rollup optional dependency so npm's canonical output keeps the package only under optionalDependencies.
pascalandr
changed the title
Contributor
Author
|
Addressed CodeNomadBot's feedback in
|
Contributor
Author
|
I want you to perform the gatekeeper checks for latest PR state check for regressions |
Contributor
|
Gatekeeper review for the latest PR state. What I understood
What I checked
Findings
Why this now passes gatekeeper review
Non-blocking suggestion
Verdict
-- |
Collaborator
|
Will this optional dependency fail when doing npm i on macOS or Linux as rollup is platform dependent and doesn't have the dependency for macos and Linux |
Contributor
|
What I understood
Answer
Why this is safe
Caveat
Bottom line
-- |
Collaborator
|
Do you think we should add a rollup entry for all platforms and archs to enable building without manual rollup fix? |
Contributor
|
What I understood
My recommendation
Why
What I would do instead
When adding all platforms would make sense
Bottom line
-- |
Collaborator
|
I believe we need to do a manual install step, check .github/workflows for the repo and see how rollup is managed |
Contributor
|
What I understood
What I found in the workflows
Updated recommendation
How I would interpret this PR in that context
Bottom line
-- |
Collaborator
|
We need to be aligned with all the platforms, suggest what needs to be done for that, not just targetted fix. |
Contributor
|
What I understood
Recommended all-platform approach
Why: this gives local installs and worktrees the same explicit platform guardrails instead of only protecting one platform.
Why: root
Why: the real risk is drift. A new supported platform can otherwise be added in one place and missed in another.
Why: if you have already seen macOS exhibit similar issues, this should be treated as a cross-platform reliability problem and verified as such. What I would not do
Suggested end state
Bottom line
-- |
Collaborator
|
Can. you reply with suggested patch to align all platforms for this? |
Contributor
|
What I understood
Suggested patch diff --git a/package.json b/package.json
@@
"devDependencies": {
"baseline-browser-mapping": "^2.9.11"
},
"optionalDependencies": {
+ "@rollup/rollup-darwin-arm64": "^4.52.5",
+ "@rollup/rollup-darwin-x64": "^4.52.5",
+ "@rollup/rollup-linux-arm64-gnu": "^4.52.5",
+ "@rollup/rollup-linux-x64-gnu": "^4.52.5",
"@rollup/rollup-win32-x64-msvc": "^4.52.5"
}
}Then regenerate the lockfile canonically: npm install --package-lock-only --ignore-scriptsOptional follow-up if Windows ARM64 is an actually supported lane and not just a dormant script: diff --git a/package.json b/package.json
@@
"optionalDependencies": {
"@rollup/rollup-darwin-arm64": "^4.52.5",
"@rollup/rollup-darwin-x64": "^4.52.5",
"@rollup/rollup-linux-arm64-gnu": "^4.52.5",
"@rollup/rollup-linux-x64-gnu": "^4.52.5",
+ "@rollup/rollup-win32-arm64-msvc": "^4.52.5",
"@rollup/rollup-win32-x64-msvc": "^4.52.5"
}
}Suggested follow-up patch for script alignment if Windows ARM64 is supported: diff --git a/packages/tauri-app/scripts/prebuild.js b/packages/tauri-app/scripts/prebuild.js
@@
const platformPackages = {
"linux-x64": "@rollup/rollup-linux-x64-gnu",
"linux-arm64": "@rollup/rollup-linux-arm64-gnu",
"darwin-arm64": "@rollup/rollup-darwin-arm64",
"darwin-x64": "@rollup/rollup-darwin-x64",
+ "win32-arm64": "@rollup/rollup-win32-arm64-msvc",
"win32-x64": "@rollup/rollup-win32-x64-msvc",
}Why this patch
What I would not include in this patch yet
Recommended validation after the patch
Bottom line
-- |
Collaborator
|
@pascalandr - Would you be able to implement the suggested fix please? |
Expand the root Rollup optional dependency set to the platforms the repo currently builds in practice: macOS x64/arm64, Linux x64/arm64, and Windows x64, all pinned to the same Rollup version already used by the repo.
pascalandr
changed the title
Contributor
Author
|
Expanded the fix as requested in
|
Contributor
Author
|
I want you to perform the gatekeeper checks for latest PR state check for regressions |
Contributor
|
Gatekeeper review for the latest PR state. What I understood
What I checked
Findings
Why this matters: the PR is now framed as aligning Rollup native packages with supported platforms. That is true for the matrix described in the PR body, but the repo still exposes a Windows ARM64 build entrypoint that is not covered by the new alignment. If someone treats that script as supported, they can still hit the same class of missing-binary problem. Recommendation: either keep treating this as intentionally unsupported and leave it out of scope, or follow up by aligning No blocking findings
Verdict
-- |
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.
Fixes #324
Summary
Validation
package-lock.jsonwithnpm install --package-lock-only --ignore-scriptsoptionalDependenciespackages/tauri-app/scripts/prebuild.js