Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix spawn EINVAL error on windows #881

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

AhmedBaset
Copy link
Contributor

Fix #853

This PR fixes a node.js breaking security batch for windows devices nodejs/node#52554 when runnint bubblewrap build

The original error was

>> bubblewrap build
,-----.        ,--.  ,--.  ,--.
|  |) /_,--.,--|  |-.|  |-.|  |,---.,--.   ,--,--.--.,--,--.,---.
|  .-.  |  ||  | .-. | .-. |  | .-. |  |.'.|  |  .--' ,-.  | .-. |
|  '--' '  ''  | `-' | `-' |  \   --|   .'.   |  |  \ '-'  | '-' '
`------' `----' `---' `---'`--'`----'--'   '--`--'   `--`--|  |-'
                                                           `--'    
Please, enter passwords for the keystore A:\projects\55-sanaye\pwa\android\android.keystore and alias android.

? Password for the Key Store: ********
? Password for the Key: ********
? Password for the Key: ********

Building the Android App...


cli ERROR spawn EINVAL

I patched the logger to log the trace:

Building the Android App...

cli ERROR spawn EINVAL

cli Error: spawn EINVAL
    at ChildProcess.spawn (node:internal/child_process:421:11)
    at spawn (node:child_process:760:9)
    at execFile (node:child_process:350:17)
    at node:child_process:242:21
    at executeFile (C:\Users\A7med\AppData\Local\pnpm\global\5\.pnpm\@bubblewrap+core@1.22.0_encoding@0.1.13\node_modules\@bubblewrap\core\dist\lib\util.js:68:18)
    at GradleWrapper.executeGradleCommand (C:\Users\A7med\AppData\Local\pnpm\global\5\.pnpm\@bubblewrap+core@1.22.0_encoding@0.1.13\node_modules\@bubblewrap\core\dist\lib\GradleWrapper.js:59:38)
    at GradleWrapper.assembleRelease (C:\Users\A7med\AppData\Local\pnpm\global\5\.pnpm\@bubblewrap+core@1.22.0_encoding@0.1.13\node_modules\@bubblewrap\core\dist\lib\GradleWrapper.js:51:20)
    at Build.buildApk (C:\Users\A7med\AppData\Local\pnpm\global\5\.pnpm\@bubblewrap+cli@1.22.0_encoding@0.1.13\node_modules\@bubblewrap\cli\dist\lib\cmds\build.js:108:34)
    at Build.build (C:\Users\A7med\AppData\Local\pnpm\global\5\.pnpm\@bubblewrap+cli@1.22.0_encoding@0.1.13\node_modules\@bubblewrap\cli\dist\lib\cmds\build.js:171:20)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

How this was tested?

I patched the dist to pass shell: true as proposed on nodejs/node#52554 and it worked as expected

@demns
Copy link

demns commented Nov 23, 2024

if for the approver's review evidence needed, before:
image

after:
image

@davidjandrey-sv
Copy link

@andreban This affects many users, please review asap

@andreban
Copy link
Member

CC @ibrahimkarahan

@ibrahimkarahan
Copy link
Collaborator

CC @OlegNitz
I'd rather replace this call with child_process.spawn and avoid shell: true for safety if possible.

Copy link

@gerkim62 gerkim62 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please accept this pr. it is clean just 2 lines changed and can't proceed with my proj without

@OlegNitz OlegNitz merged commit 4a24e83 into GoogleChromeLabs:main Dec 16, 2024
1 check passed
OlegNitz added a commit that referenced this pull request Dec 19, 2024
Partial rollback of PR #881: exec() cannot have shell:true
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.

Unable to use Bubblewrap Build
8 participants