Make build-for-windows.ps1 fail if install-node-dependencies.sh fails
#2120
Conversation
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
AliSoftware
reviewed
Nov 25, 2025
Contributor
There was a problem hiding this comment.
What do you think about instead removing the -eu from the shebang of the install-node-dependencies.sh and replacing it with an explicit call to set -eu in that same file right after that shebang?
That way the onus would still be on the install-node-dependencies.sh to set those flags itself, rather than expecting the caller (that one .ps1 script for the time being but maybe more scripts would call it in the future) to remember to use bash -eu when calling it.
Contributor
📊 Performance Test ResultsComparing e702179 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
Contributor
Author
|
@AliSoftware, good call. I've made that change 👍 |
AliSoftware
approved these changes
Nov 25, 2025
wojtekn
approved these changes
Nov 25, 2025
fredrikekelund
deleted the
f26d/exit-flag-for-node-dependencies-script-on-windows
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Related issues
Proposed Changes
The Windows production build was borked in the recent 1.6.4 release because the
postinstallscript inpackage.jsonfailed (specifically,scripts/download-wp-server-files.tsfailed due to GitHub rate limits on the SQLite plugin download). This should have caused the entire build to fail, but it didn't.The reason it didn't is that we don't call
set -eininstall-node-dependencies.sh. Instead, we use the shebang line at the top of that script to set the-eflag. However, because we specify that thebashexecutable should be used to run the script inbuild-for-windows.ps1, the shebang line is ignored, and the-eflag is never set.This PR fixes the problem by setting the
-euflags ininstall-node-dependencies.shwith an actualsetcommand, and not in the shebang comment.Testing Instructions
Build for Windowsstep should pass on CIPre-merge Checklist