-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(cli): spawning node fails when node path contains spaces #24001
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
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! Nothing wrong with this change from first glance, but we do already have a solution for this problem. We have files called os.ts that do this so I'd prefer to reuse one of those than rewrite this work again.
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
|
The pull request linter fails with the following errors: PRs must pass status checks before we can provide a meaningful review. If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing |
When spawning a Node.js child process, if the
nodeexecutable is under a path that contains spaces or other characters that need to be escaped, the spawn fails. This is due to how Node.js handleschild_process.spawnwhenshellis set totrue.The fix is to (1) revert to using an array to pass in the command and arguments (initially changed in #17176) and (2) quote the command. The change for (1) is a refactor so that we may allow an array OR a string to be passed into
exec.I've verified that this change fixes the reported issue by modifying the bundled
index.jslocally. It's not clear how to best test this withinexec.test.jssince the Node.js process path is progmatically extracted from the app context. I'm happy to adjust the PR if the maintainers have a suggestion here.Closes aws/aws-cdk-cli#636.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license