CMakeCache.txt removed when switching kit but cmake.buildDirectory depends on ${buildKitVendor}

[david-fong]

Brief Issue Summary

Severity (from my POV): minor bug

When I do the select kit action and change between kits, the extension deletes the CMakeCache.txt and CMakeFiles.txt files from the build directory. In my project's settings.json, I have set cmake.buildDirectory to "${workspaceFolder}/build/${buildKitVendor}". due to the file removal, when I try to run cmake --build in the switched-from (the previous) kit's build directory, I get an error saying the cache file is gone.

[driver] Switching to kit: GCC 11.2.0 x86_64-linux-gnu
[driver] Removing /home/david/code/mine/okiidoku/cpp/build/Clang/CMakeCache.txt
[driver] Removing /home/david/code/mine/okiidoku/cpp/build/Clang/CMakeFiles
[proc] Executing command: /usr/bin/gcc -v

I think I can fix this by just running the cmake configure command manually in the switched-from kit's folder to recreate the cmake cache file (in which case, do I need to copy the exact command that the extension uses to create the same cmake cache file contents?).

EDIT: see my below comment for the description of the bug.

CMake Tools Diagnostics

No response

Debug Log

No response

Additional Information

I set up my project in this way because I want to be able to easily and quickly test my project locally with GCC, Clang, and (on my windows machine) MSVC, and also be able to compare the build artifact symbol maps and sizes. (Maybe there's a better way to setup my project to achieve this?)

[elahehrashedi]

We should be able to detect a change in the build directory when changing the kit. We may not be able to get to this immediately, but we would accept a PR from the community.

[david-fong]

I did some reading. The code that removes the cache file is at src/drivers/cmakeDriver.ts:395 (_cleanPriorConfiguration()).

The relevant call to that function is the one at src/drivers/cmakeFileApiDriver.ts:180 (doSetKit())

Which is called at src/drivers/cmakeDriver.ts:515 (setKit()).

Now here's something surprising- the check for change in binaryDir is already written! The bug is that (at line 512) it takes the current this.expansionOptions and only updates it opts.vars.buildKit with the new kit name before doing variable expansion to get the new binaryDir; all the rest of the substitution of variables to get the buildDir for the new kit is still using values from the old kit! Indeed- if in my settings.json I set cmake.buildDirectory to "${workspaceFolder}/build/${buildKit}", the cache file does not get removed.

The solution is to make it so that all variables related to build kits get properly used to calculate the new binaryDir.

Possible fixes:

1. The first fix that comes to mind is to just add lines after line 512 that update all those variables. It seems ugly, but it should work. However, if in the future, new buildKit* variables are added, it is likely to be forgotten that this code will need to be updated.

2. Add logic in kitChangeNeedsClean to check all the buildKit* variables for changes and return true if any of those properties is different. This option loses the benefit of the "compare-old-and-new-buildDir" option- that it is designed to only consider those variables that are used in the user-configured buildDirectory setting, and that is conceptually very simple.

3. I'm assuming the rest of the expansion is done later at line 518 by the call to _refreshExpansions. If possible, defer the decision (the expansion to get the new buildDir and compare it with the old buildDir) of whether or not to remove the cache file to after _refreshExpansions has been performed. This means that the removal of the cache file (the call to _cleanPriorConfiguration) also would need to be deferred. That part will require care: I have no idea whether or not _cleanPriorConfiguration is safe to move to go later in the procedure like that. I'm not that familiar with the codebase.

I can make a PR to examplify what I'm imagining for option 3. But I have no experience with vscode extension development and local-testing of extensions. Unless someone wants to guide me through that I won't be able to test it locally myself.

[david-fong]

A relevant past PR: #2335 which added the call to refreshExpansions in the setKit callback

[david-fong]

Fixed by #2551 :)