microsoft / vscode-cmake-tools

CMake integration in Visual Studio Code
https://marketplace.visualstudio.com/items?itemName=vector-of-bool.cmake-tools
MIT License
1.46k stars 449 forks source link

CMake executable cannot be a script #4037

Open JVApen opened 1 week ago

JVApen commented 1 week ago

Brief Issue Summary

The option "cmake.cmakePath" cannot refer to a script. For example: (cmake.bat)

C:\path\to\cmake.exe $*

(In the real case, this is more complex as this triggers some remote execution)

Output:

[cmakeExecutable] CMake executable not found in cache. Checking again.
[proc] Executing command: C:/path/to/cmake.cmd --version
[proc] Executing command: C:/path/to/cmake.cmd -E capabilities

CMake Tools Diagnostics

No response

Debug Log

No response

Additional Information

I already did some reverse engineering on the code. proc.ts has a method called execute which accepts some options. If options.shell is set to true, it should enable the shell option as described in https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows However cmakeExecutable.ts in the method getCMakeExecutableInformation doesn't pass this option. const execOpt: proc.ExecutionOptions = { showOutputOnError: true };

emmenlau commented 1 week ago

This is a relevant issue for us too, and a regression over some previous version (unsure which version).

gcampbell-msft commented 1 week ago

@JVApen @emmenlau If you could provide some more information about this issue, that would be great. For example, repro steps to reproduce this, or a small exacmple of a script you're using.

Looking at the code, nothing between version 1.18.44 and 1.19.50 was changed when checking the cmake.exe, so I wouldn't expect that it's causing an issue. Therefore, more investigation may be needed and any repro steps can help with that. Thanks!

JVApen commented 1 week ago

@gcampbell-msft : see my first event, simple CMD file with c:\path\to\cmake.exe $* should be sufficient to reproduce

emmenlau commented 1 week ago

Dear @gcampbell-msft , indeded, @JVApen is correct. A reproducer can be achieved with a simple CMD script like:

@ECHO OFF

REM We do other steps here as required, for example modify cmake settings, or change PATH, or change directory, ...
REM Then execute cmake:

C:\path\to\cmake.exe %*
gcampbell-msft commented 1 week ago

@JVApen When trying to reproduce with these steps: https://github.com/microsoft/vscode-cmake-tools/issues/4037#issuecomment-2330984376, I notice that the "Bad CMake executable" popup appears in 1.18.44 as well. Do you also see this?

gcampbell-msft commented 1 week ago

@emmenlau @JVApen I actually see this issue as far back as 1.14.34. Is there something I'm missing in the repro? Or is this correct?

emmenlau commented 1 week ago

Dear @gcampbell-msft , sadly I can not say which version worked for me before. We have an isolated system that is not updated too frequently. I would think that the last working version was from about 6 months ago (both VSCode and all extensions), but I may be off a bit there.

In case that helps, we're using the same scripting "trick" on Linus and MacOS every day, and there it works flawless. These systems are updated very frequently, and used more than our Windows environment. So the problem is Windows-only, and I would think it has to do with not allowing nodejs to spawn bat files. I can only say this worked at some point, for at least a few months, maybe the better part of a year.

Does the suggestion from @JVApen help, about https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows?

gcampbell-msft commented 1 week ago

@JVApen @emmenlau I've tracked the issue down. This didn't come from a CMake Tools update, but rather a VS Code update. In July, VS Code updated to 1.92 which now uses Node 20. This contains a breaking change for spawn, which we weren't aware of.

Could you please test the vsix (after modifying the file extension from .zip to .vsix) from here? https://github.com/microsoft/vscode-cmake-tools/pull/4044#issue-2507517383

emmenlau commented 1 week ago

Dear @gcampbell-msft , awesome!!! This indeed fixes the issue!!!

Just to confirm, the linked extension will identify itself as v1.13.0 (pre-release) which is a bit surprising because VSCode will complain that a newer version was already installed. But aside from this small detail, the linked cmake tools extension works and successfully executes by wrapper bat script on Windows!

Regression fixed!

kuch3n commented 5 days ago

This fix would allow converting windows paths to cygwins posix paths, too: #4038 Or even load shells for Cygwin, MSys etc.

dbaeumer commented 2 days ago

Small hint in case it help. There was a breaking change in NodeJS that it requires to pass shell: true if you want to execute a command script. If not pass the execution fails.

emmenlau commented 2 days ago

Small hint in case it help. There was a breaking change in NodeJS that it requires to pass shell: true if you want to execute a command script. If not pass the execution fails.

Thanks @dbaeumer for the hint! However the regression has already been fixed. Do you think that the fix requires more attention?