vscode-cmake-tools icon indicating copy to clipboard operation
vscode-cmake-tools copied to clipboard

Different behavior between extension and CMake cli, when choosing the default generator in presets

Open jochil opened this issue 3 years ago • 10 comments

Brief Issue Summary

As the generator field is optional in a preset using CMake directly falls back to the default generator discovery procedure. I would expect the vscode extension to do the same... but it falls back to Ninja as generator, according to observed behavior and this code block: https://github.com/microsoft/vscode-cmake-tools/blob/01c4796fca985956fb19acda0e8e7ff7d0425391/src/preset.ts#L707-L712

From the CMake docs at https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html:

An optional string representing the generator to use for the preset. If generator is not specified, it must be inherited from the inherits preset (unless this preset is hidden). In version 3 or above, this field may be omitted to fall back to regular generator discovery procedure.

I would like to know if there is a reason (I might have overlooked) for this behavior... if not I would be happy to create a PR :slightly_smiling_face:

CMake Tools Diagnostics

No response

Debug Log

No response

Additional Information

No response

jochil avatar Sep 08 '22 11:09 jochil

preferredGeneratorName is originally being determined in this function:

https://github.com/microsoft/vscode-cmake-tools/blob/01c4796fca985956fb19acda0e8e7ff7d0425391/src/cmakeProject.ts#L558-L572

Which searches for alternatives in user configuration. If it is still not defined, the extension will uses Ninja as the last alternative.

elahehrashedi avatar Sep 08 '22 15:09 elahehrashedi

What should be happening with presets is that if the generator is not defined somewhere in the inheritance, the preset should be considered invalid and not available for use. In that case, we shouldn't be hitting any fallback code.

Are you seeing incorrect behavior done by the extension? And if so, can you share your repro?

bobbrow avatar Sep 08 '22 16:09 bobbrow

Thanks for your answers!

if the generator is not defined somewhere in the inheritance, the preset should be considered invalid and not available for use

I think this is not correct, at least from what I got from the CMake docs. The generator field is optional...if it is missing and no generator is defined in the preset inheritance it is on the cmake command to figure out the generator. For example, if I run these presets https://github.com/CodeIntelligenceTesting/cifuzz/blob/main/tools/cmake/cifuzz/share/CMakePresets.json on my command line (cmake --preset="cifuzz (Fuzzing)" or cmake --build --preset="cifuzz (Fuzzing)" my system uses Make as a generator. If I run the presets in the exact same project with the extension, the following command gets generated:

[proc] Executing command: /usr/bin/cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCIFUZZ_ENGINE=libfuzzer "-DCIFUZZ_SANITIZERS=address;undefined" -DCIFUZZ_TESTING:BOOL=ON -DCMAKE_BUILD_RPATH_USE_ORIGIN:BOOL=ON -S/home/jochen/dev/workspace/ci/cifuzz/examples/cmake -B/home/jochen/dev/workspace/ci/cifuzz/examples/cmake/.cifuzz-build/libfuzzer/address+undefined -G Ninja

... which explicitly uses -G Ninja. Without the -G parameter it would behave exactly like executing the preset directly via cmake, which IMO should be the goal here

jochil avatar Sep 08 '22 17:09 jochil

I think this is not correct, at least from what I got from the CMake docs. The generator field is optional...if it is missing and no generator is defined in the preset inheritance it is on the cmake command to figure out the generator.

Sorry, I mispoke. You are correct. The behavior changed in v3. We might not have fixed the extension to deal with that yet. I don't believe the extension has a path to correctly handle unset generators yet either. For the time being we recommend you set it explicitly if possible.

bobbrow avatar Sep 08 '22 17:09 bobbrow

Sorry, I mispoke. You are correct. The behavior changed in v3. We might not have fixed the extension to deal with that yet. I don't believe the extension has a path to correctly handle unset generators yet either. For the time being we recommend you set it explicitly if possible.

Thanks... for my personal workflow, setting it explicitly would be fine, but we are recommending this extension in our project at https://github.com/CodeIntelligenceTesting/cifuzz and would like having consistent behavior for our users. Long story short: Would you prefer handling the issue yourself or would it be fine for you if I start working on a PR?

jochil avatar Sep 09 '22 08:09 jochil

We would accept a PR if you are willing to work on this.

bobbrow avatar Sep 09 '22 15:09 bobbrow

@bobbrow I am working on the PR (https://github.com/microsoft/vscode-cmake-tools/compare/main...jochil:vscode-cmake-tools:v3-preset-default-generator) but I have a few questions:

  1. The unit tests are not running on my system... I get a lot of this Error: Error starting up cmake-server. Are there any instructions on how to setup the test environment?
  2. What way would you prefer to cover this change with a test?
  3. I only found the default generator behavior for configure presets, it seems like for build/test presets it isn't handled... Am I right or am I overseeing something?

jochil avatar Sep 13 '22 11:09 jochil

  1. Sorry about that. We need to write a testing guide. To make the cmake-server tests work, you need to install cmake version 3.18.2 and put it in the path ahead of your normal version of cmake
  2. The first place that comes to mind would be adding another preset to test/extension-tests/single-root-UI/project-folder/CMakeUserPresets.json and adding a test case in test/extension-tests/single-root-UI/test/configure-and-build-presets.test.ts. I think that would be sufficient.
  3. I'm not sure I understand the question. The build presets shouldn't need a generator. Are you saying that a build preset linked to a configure preset that doesn't have a generator won't work? Does it work in the terminal, but not the extension?

bobbrow avatar Sep 14 '22 18:09 bobbrow

@bobbrow thanks for the answers... with 3. I was wrong of course, no need for a generator here :slightly_smiling_face: I created the PR: https://github.com/microsoft/vscode-cmake-tools/pull/2748

Thanks a lot for the support

jochil avatar Sep 15 '22 13:09 jochil

Awesome! Thanks for the help! We'll try to take a look within the next few days.

bobbrow avatar Sep 15 '22 19:09 bobbrow