Use spawn rather than exec to support paths with spaces
#539
Conversation
| exec(command, (error, stdout, _) => { | ||
| if (error) { | ||
| reject(error); | ||
| const child = spawn(cmd, args); |
There was a problem hiding this comment.
I think execFile is what we want here https://nodejs.org/api/child_process.html
There was a problem hiding this comment.
Oh yeah, that looks better.
|
I was about to file an issue for that, thanks! Although I’m mystified why “slap a string together and execute it as a shell script” is still a thing people occasionally do. Sorry for the harshness in the preceding sentence, I appreciate all the hard work! |
Our use-case is to find out the |
|
What I’m saying is that there’s a reasonable API to call a binary with optional arguments This PR switched from the latter to the former, for which I’m grateful, but I’m puzzled why anyone would ever default to the latter way. |
## Summary I use ruff for our embedded python interpreter. This interpreter needs to be bootstrapped (env-vars) before be able to get called (technical limit of the environment itself). This is usually done with `.cmd` file (the modern version of `.bat`) on windows which is kind of similar to `.sh` on linux. `.cmd` files are often use on Windows, even vscode itself use it to start vscode on windows. Today I noticed that I get a crash when using the `.cmd` interpreter path. It seems that node require shell mode (in windows calling `cmd.exe` as the shell) to be able to call `.cmd` files correctly. I needed to quote the input filename as well to avoid whitespace issues which looks like a bug in node itself. With that PR I got no crashes anymore when the extension try to run ruff 😄 . Note: The shell mode only get activated when the platform is windows and the file extension is `.cmd`, so users with regular executables should not be affected at all. ## Test Plan Manual testing it locally works great and as expected. I added a utils-test to check the require shell mode flag. The PR is related to the changes of #539 --------- Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
Summary
If the interpreter path contains a space, then the find script fails. We need to escape the arguments that we pass to
exec. But, it seems easier to just usespawn, which doesn't require escaping.Closes astral-sh/ruff#12394.
Test Plan
Created a virtual environment
foo barin a repo; verified that the script failed before but succeeds after this change.