Partial/full activation mode and multi root. Disable automatic configures. #1295
Conversation
src/cmake-tools.ts
Outdated
|
|
||
| log.debug(localize('cmakelists.save.trigger.reconfigure', "We are saving a CMakeLists.txt file, attempting automatic reconfigure...")); | ||
| await this.configure([], ConfigureType.Normal); | ||
| if (str === path.join(this.folder.uri.fsPath, "CMakeLists.txt")) { |
There was a problem hiding this comment.
Is this a multiroot problem that you're trying to fix? If so, then I think you need to look at the cmake.sourceDirectory setting and look if the CMakeLists.txt is under that folder instead.
Technically, I think that CMakeLists can reference folders anywhere in the filesystem, so that's not a 100% fix, but maybe addresses 99% of cases.
There was a problem hiding this comment.
Oh right, sourceDirectory is matched to folder root if is not set to something else. From my understanding while debugging this area, CMakeLists.txt is always under sourceDirectory, so I think this will be 100% fix.
There was a problem hiding this comment.
We also support sourceDirectory with CMakeLists.txt included in the path so you should account for that here.
There was a problem hiding this comment.
Good catch. It made me remember https://github.com/microsoft/vscode-cmake-tools/pull/1016/files and I actually debugged and I confirm that CMakeLists.txt gets removed from sourceDirectory before the CMakeLists path check. Do you think there is a different scenario where actually sourceDirectory would end with CMakeLists.txt once we hit the file content change listener?
| private _enableFullFeatureSetOnWorkspace = false; | ||
| public enableFullFeatureSet(fullFeatureSet: boolean, folder: vscode.WorkspaceFolder) { | ||
| this.getFolderContext(folder).ignoreCMakeListsMissing = !fullFeatureSet; | ||
| this._enableFullFeatureSetOnWorkspace = this._enableFullFeatureSetOnWorkspace || fullFeatureSet; |
There was a problem hiding this comment.
I find this pattern confusing. You need to set _enableFullFeatureSetOnWorkspace to false outside of this method because fullFeatureSet can't change the true to false. Is there a reason you can't just take the value of fullFeatureSet?
There was a problem hiding this comment.
I'm not sure I understand but let me explain more and you can follow up with rephrasing the above if needed.
Each folder of the multi root workspace may have different values for fullFeatureSet that need to stay persisted in the stage of each of them. Once I apply any OR to them, we'll persist the wrong value. I was thinking that we persist the exact state for each folder: full or partial activation, while partial activation means the "ignore" button has been pressed already and full means the ignore wasn't pressed. And calculate based on them a final decision for the whole multi root workspace. And yes, once true it can't get back to false if we don't have the extra variable.
There was a problem hiding this comment.
Do we ever need to turn support off? I'm wondering if we should just leave the full feature set enabled once it's been enabled.
There was a problem hiding this comment.
If you remove the state that would make CMake Tools enable the full feature set, then you would need to reload the window at that point to disable it. I would be fine with that behavior.
|
I am looking at the failed tests. Again, "electron: Loading non context-aware native modules in the renderer process is deprecated...". I fixed this before with another PR a few weeks or months ago. I had to move a particular call somewhere else. |
…teners and call to enableFullFeatureSet still causes Electron issue.
|
The only fix needed for the Electron issue was to not call enableFullFeatureSet from driver.ts. To still achieve the same functionality (we want full activation when beforeConfigureOrBuild returns true and partial only for missing CMakeLists.txt), I introduced a -2 error code (which wasn't used already, only -1 was returned on any failure path) to recognize later that we have a missing CMakeLists.txt, thus calling enableFullFeatureSet in a place where Electron doesn't complain. The reason for commenting out the sourceDirectory listeners in the driver (the previous commit) was that I had infrastructure issues on my Ubuntu and with no amount of cleanup (not even fresh cloning of CMake Tools) I wasn't able to get rid of the Electron test issue so I just made some experiments to leverage the GitHub CI. |
Introduce cmake.configureOnEdit to control the automatic configures triggered by editing CMakeLists.txt or cmake.sourceDirectory.
Refactored and fixed the whole partial/full activation modes in the multi root scenario.
This addresses GitHub issues:
#1259
#1269