-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add support for Windows .cmd
and .bat
files
#570
Add support for Windows .cmd
and .bat
files
#570
Conversation
So, what I understand from this is that a wrapper script is used to call the Python interpreter instead of calling it directory, right? I'm curious as to what are you using the |
Yes. Not all required dll files can be next to the executable so we need to set e.g. the
I don't use it because I expect that ruff-vscode extension will get it from the python-vscode extension. In general the python-vscode extension support interpreters with a
Checkout the mentioned line from above |
I see. Does this mean that the
Is it possible for you to provide some logs around this crash? Sorry to ask these many questions, I just want to understand why this is required. Overall, I think this seems like a reasonable change, I'll review it today. |
Not directly. It support
My config {
"python.defaultInterpreterPath": "C:\\Program Files\\LDS Studio\\bin\\LDSPython.cmd",
"python.autoComplete.extraPaths": [
"C:\\Users\\myusername\\AppData\\Local\\company\\LDS\\pkgmanaged\\DC97AD51-CBBD-5954-9BC2-5124B5C6D82B\\python\\site-packages"
],
"python.analysis.extraPaths": [
"C:\\Users\\myusername\\AppData\\Local\\company\\LDS\\pkgmanaged\\DC97AD51-CBBD-5954-9BC2-5124B5C6D82B\\python\\site-packages"
]
} |
Thanks for the details. Sorry that I wasn't able to get to it today. I plan to review it first thing tomorrow morning. |
The change does make sense to me. The only thing that I find odd is the need for quoting the executable name. It would be nice to understand the reason why it is necessary before merging |
It's "not" a node bug nodejs/node#38490 🥲 |
I think an alternative to this is to call execFile with cmd.exe (may require some arguments) and the bat file as the first argument so that we don't need a shell (and deal with escaping arguments where spaces are just one character that needs escaping) |
@MichaReiser Yes, calling cmd.exe directly is an option, but would need more code in the end. So basically we are talking about reimplementing the node implemention with auto-quoting, because we use the shell on Windows anyway 😀 |
For reference, https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows provides details on what's required for running |
.cmd
and .bat
files
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!
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