Avoid unconditional bash execution on Windows#1417
Avoid unconditional bash execution on Windows#1417kiritocode1 wants to merge 4 commits intogoogle:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
updated the cla lol . |
|
The main question of #1331: why |
|
@antongolub thank you for bringing it to my attention , I just did a workaround ,
ill update the pr with the fixes . im thinking of a. either validate that the resolved bash is actually invocable by doing something like this : try {
const { shell, prefix, postfix } = $
if (process.platform === 'win32') {
try {
useBash()
// Validate that the resolved bash is actually invocable (not a broken WSL ref)
cp.execSync(`"${$.shell}" -c "echo ok"`, {
stdio: 'ignore',
timeout: 5000,
})
} catch {
$.shell = true
$.prefix = ''
$.postfix = ''
$.quote = quote
}
} else {
useBash()
}which adds a small latency to every import on windows while keeping the performance the same on unix and macos. or which option will work better in the long run? happy to implement either. |
|
The problem is broader: |
I see, then what could be a better approach? |
|
I think we also need a comprehensive, reliable, and safe guide to installing Bash for different versions of Windows. |
sounds good. who do we reach out to for collaborating on this or should I take the lead? |
Fixes #1331
This PR resolves a sudden crash on Windows environments when importing
zx. Previously,useBash()was being called unconditionally at the module level. This checks if the platform is Windows (win32) and defaults.shelltotruewith the standardquote, circumventing the exception when a user on Windows doesn't easily havebashavailable.Changes:
src/core.ts: Safely detectwin32platform before applying shell defaults.test/init-win32.test.js: Added unit test to verify that loading the module onwin32correctly assigns$.shell = true..size-limit.json: Accommodated the tiny bundle size increase caused by the bugfix check.build/*: Rebuilt and committed the compiled node modules.npm buildbefore committing and verified the bundle updates correctly.run testand confirmed all tests succeed.