microsoft / vscode-cmake-tools

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

Bug with cmake.buildDirectory #4038

Closed sabudilovskiy closed 1 month ago

sabudilovskiy commented 2 months ago

Brief Issue Summary

cmake.buildDirectory override build directory from preset. This behavior is most annoying. I would like an option so that everything is taken only from the preset.

CMake Tools Diagnostics

No response

Debug Log

No response

Additional Information

No response

kuch3n commented 2 months ago

Related issue: #1376

gcampbell-msft commented 2 months ago

@kuch3n @sabudilovskiy Could you help us understand where this issue reproduces? Does it reproduce in v1.18.44? Thanks.

gcampbell-msft commented 2 months ago

@sabudilovskiy Also, could you share a repro for this? I don't see this reproducing, if I define a binaryDir in my preset, then that directory is used rather than any cmake.buildDirectory from the settings.

Thanks.

kuch3n commented 2 months ago

Currently i assume the described bug is mainly caused by #1376

Additionaly it makes a difference if you call the same command from the vscode console or via the plugin itself. E.g., the odd c? directory is missing. Maybe it's a problem how the executable is run from the cmake-tools plugin.

As for reproducability, you just need a cygwin environment and use CMake from cygwin. A plain CMakeList.txt is enough.

For now i'm trying to run CMake via a hook, but the command output is missing if run via the plugin:

@echo off
setlocal enableextensions
set TERM=
set CYGWIN=winsymlinks:sys
cd /d "%~dp0bin" && .\bash -c "cmake %*"

EDIT: maybe CMake behaviour is different if stdout is not a tty

sabudilovskiy commented 2 months ago

@sabudilovskiy Also, could you share a repro for this? I don't see this reproducing, if I define a binaryDir in my preset, then that directory is used rather than any cmake.buildDirectory from the settings.

Thanks.

1) 1.19.50 2) I think I've figured out what the problem is, and honestly, the mistake is on my side on the one hand, but on the other... In general, as I understand it, the extension parses the preset and overturns all options manually? If in the end there is something crooked in the options (in my case "binaryDir": "${SourceDir}/build_debug"), then it takes the default option. I don't quite understand, but why don't you just use cmake --preset if the user specifies to always use it?

gcampbell-msft commented 2 months ago

@sabudilovskiy Does it reproduce in any other versions prior to 1.19.50? This information is important to know so that we can understand when the issue may have been introduced

gcampbell-msft commented 2 months ago

@sabudilovskiy I also ask because I tried to repro this with a simple cmake project using presets and I don't believe that I was able to reproduce with the most recent version.

kuch3n commented 2 months ago

It would be nice to specify source/binary directories in presets, while ignoring those out of the settings. E.g., if using cygwins cmake, it needs to be called with cygwin style paths, otherwise non obvious problems will occur while building/configuring (see #1376). Though such options needs to be handled correctly by this plugin if a cygwin style is used.

On the other hand, cygpath could be used to translate given direcotries from and to Cygwins CMake, before doing the actual call: C:/cygwin64/bin/cmake -S(C:/cygwin64/bin/cygpath -u -a ".") -B(C:/cygwin64/bin/cygpath -u -a "./build")

EDIT: the binaryDir can be given with a cygwin prefix, but i don't see a way to specify a source dir without braking the plugin

kuch3n commented 2 months ago

Handling windows paths should be done by CMake and not any IDE out there. Therefore i created an issue and example implementation to fix it: https://gitlab.kitware.com/cmake/cmake/-/issues/26277

Thus this issue can be closed

v-frankwang commented 1 month ago

@kuch3n Thank you very much for the fix. We'll verify and close this issue after this PR merge

gcampbell-msft commented 1 month ago

Refreshing my context after being OOF and it seems like this issue can be closed basedon @kuch3n recent comments. I'll close this issue and it seems like both items have been resolved or understood. Please let me know or create a new issue if there are still things needed. Thanks!