From eee243e63bc3ac883e6fd35698be5478d44bc703 Mon Sep 17 00:00:00 2001 From: Andreea Isac Date: Tue, 9 Jun 2020 10:03:18 -0700 Subject: [PATCH 1/5] Partial/full activation mode and multi root. Disable automatic configures. --- package.json | 6 +++ package.nls.json | 1 + src/cmake-tools.ts | 44 +++++++++++------- src/config.ts | 4 ++ src/drivers/cmfileapi-driver.ts | 25 +++++++++++ src/drivers/driver.ts | 14 ++++++ src/extension.ts | 79 ++++++++++++++------------------- src/state.ts | 7 +-- test/unit-tests/config.test.ts | 1 + 9 files changed, 113 insertions(+), 68 deletions(-) diff --git a/package.json b/package.json index f6565b181..300cf3e45 100644 --- a/package.json +++ b/package.json @@ -1249,6 +1249,12 @@ "description": "%cmake-tools.configuration.cmake.configureOnOpen.description%", "scope": "resource" }, + "cmake.configureOnEdit": { + "type": "boolean", + "default": true, + "description": "%cmake-tools.configuration.cmake.configureOnOpen.description%", + "scope": "resource" + }, "cmake.skipConfigureIfCachePresent": { "type": "boolean", "default": null, diff --git a/package.nls.json b/package.nls.json index 49c9aca33..9fb9f0d09 100644 --- a/package.nls.json +++ b/package.nls.json @@ -99,6 +99,7 @@ "cmake-tools.configuration.cmake.emscriptenSearchDirs.description": "Directories where Emscripten may be installed.", "cmake-tools.configuration.cmake.copyCompileCommands.description": "Copy compile_commands.json to this location after a successful configure.", "cmake-tools.configuration.cmake.configureOnOpen.description": "Automatically configure CMake project directories when they are opened.", + "cmake-tools.configuration.cmake.configureOnEdit.description": "Automatically configure CMake project directories when cmake.sourceDirectory or CMakeLists.txt content are saved.", "cmake-tools.configuration.cmake.skipConfigureIfCachePresent.description": "Skip over the configure process if cache is present.", "cmake-tools.configuration.cmake.cmakeCommunicationMode": "The protocol used to communicate between the extension and CMake", "cmake-tools.configuration.cmake.ignoreKitEnv.description": "Do not use the kit environment variables when running CMake commands.", diff --git a/src/cmake-tools.ts b/src/cmake-tools.ts index e4a163755..24c4dd8ea 100644 --- a/src/cmake-tools.ts +++ b/src/cmake-tools.ts @@ -9,7 +9,7 @@ import diagCollections from '@cmt/diagnostics/collections'; import * as shlex from '@cmt/shlex'; import {StateManager} from '@cmt/state'; import {Strand} from '@cmt/strand'; -import {ProgressHandle, versionToString} from '@cmt/util'; +import {ProgressHandle, versionToString, lightNormalizePath} from '@cmt/util'; import {DirectoryContext} from '@cmt/workspace'; import * as path from 'path'; import * as vscode from 'vscode'; @@ -271,8 +271,15 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI { const cmakeListsFile = await vscode.window.showOpenDialog(openOpts); if (cmakeListsFile) { const fullPathDir: string = path.parse(cmakeListsFile[0].fsPath).dir; - const relPathDir: string = path.relative(this.folder.uri.fsPath, fullPathDir); - vscode.workspace.getConfiguration('cmake').update("sourceDirectory", path.join("${workspaceFolder}", relPathDir)); + const relPathDir: string = lightNormalizePath(path.relative(this.folder.uri.fsPath, fullPathDir)); + const drv = await this.getCMakeDriverInstance(); + if (drv) { + const joinedPath = "${workspaceFolder}/".concat(relPathDir); + vscode.workspace.getConfiguration('cmake', this.folder.uri).update("sourceDirectory", joinedPath); + drv.config.updatePartial({sourceDirectory: joinedPath}); + } else { + throw new Error(localize('unable.to.save.sourceDirectory', 'Unable to save "cmake.sourceDirectory". There is no valid cmake driver instance.')); + } } else { fullFeatureSet = false; } @@ -289,14 +296,14 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI { // Previously, the user decided to ignore the missing CMakeFiles.txt. // Since we are here in cmakePreConditionProblemHandler, for the case of CMakePreconditionProblems.MissingCMakeListsFile, // it means that there weren't yet any reasons to switch to full functionality, - // so keep enableFullFeatureSet as true. + // so keep enableFullFeatureSet as false. fullFeatureSet = false; } break; } - await enableFullFeatureSet(fullFeatureSet); + await enableFullFeatureSet(fullFeatureSet, this.folder); } /** @@ -445,14 +452,22 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI { this.extensionContext.subscriptions.push(vscode.workspace.onDidSaveTextDocument(async td => { const str = td.uri.fsPath; - if (str.endsWith("CMakeLists.txt")) { - // Reset to a full activation mode. - // If required, the extension will switch back to partial activation mode - // after more analysis triggered by the below configure. - await enableFullFeatureSet(true); - - 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")) { + // The configure process can determine correctly whether the features set activation + // should be full or partial, so there is no need to proactively enable full here, + // unless the automatic configure is disabled. + // If there is a configure or a build in progress, we should avoid setting full activation here, + // even if cmake.configureOnEdit is true, because this may overwrite a different decision + // that was done earlier by that ongoing configure process. + const drv = await this.getCMakeDriverInstance(); + if (drv && !drv.configOrBuildInProgress()) { + if (drv.config.configureOnEdit) { + log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected saving of CMakeLists.txt, attempting automatic reconfigure...")); + await this.configure([], ConfigureType.Normal); + } else { + await enableFullFeatureSet(true, this.folder); + } + } } })); } @@ -678,9 +693,6 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI { break; } if (retc === 0) { - // Switch to full CMake Tools extension functionality: - // visible status bar and commands in the pallette. - await enableFullFeatureSet(true); await this._refreshCompileDatabase(drv.expansionOptions); } await this._ctestController.reloadTests(drv); diff --git a/src/config.ts b/src/config.ts index 5f5f47bc9..816d27c60 100644 --- a/src/config.ts +++ b/src/config.ts @@ -95,6 +95,7 @@ export interface ExtensionConfigurationSettings { emscriptenSearchDirs: string[]; copyCompileCommands: string|null; configureOnOpen: boolean|null; + configureOnEdit: boolean; skipConfigureIfCachePresent: boolean|null; useCMakeServer: boolean; cmakeCommunicationMode: CMakeCommunicationMode; @@ -241,6 +242,8 @@ export class ConfigurationReader implements vscode.Disposable { get configureOnOpen() { return this.configData.configureOnOpen; } + get configureOnEdit() { return this.configData.configureOnEdit; } + get skipConfigureIfCachePresent() { return this.configData.skipConfigureIfCachePresent; } get useCMakeServer(): boolean { return this.configData.useCMakeServer; } @@ -323,6 +326,7 @@ export class ConfigurationReader implements vscode.Disposable { emscriptenSearchDirs: new vscode.EventEmitter(), copyCompileCommands: new vscode.EventEmitter(), configureOnOpen: new vscode.EventEmitter(), + configureOnEdit: new vscode.EventEmitter(), skipConfigureIfCachePresent: new vscode.EventEmitter(), useCMakeServer: new vscode.EventEmitter(), cmakeCommunicationMode: new vscode.EventEmitter(), diff --git a/src/drivers/cmfileapi-driver.ts b/src/drivers/cmfileapi-driver.ts index ff55637e8..396762e0f 100644 --- a/src/drivers/cmfileapi-driver.ts +++ b/src/drivers/cmfileapi-driver.ts @@ -21,9 +21,14 @@ import rollbar from '@cmt/rollbar'; import * as util from '@cmt/util'; import * as path from 'path'; import * as vscode from 'vscode'; +import * as ext from '@cmt/extension'; import {NoGeneratorError} from './cms-driver'; +import * as nls from 'vscode-nls'; +nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })(); +const localize: nls.LocalizeFunc = nls.loadMessageBundle(); + const log = logging.createLogger('cmakefileapi-driver'); /** * The CMake driver with FileApi of CMake >= 3.15.0 @@ -97,6 +102,26 @@ export class CMakeFileApiDriver extends codemodel.CodeModelDriver { log.debug(`Reload CMake cache: ${this.cachePath} changed`); rollbar.invokeAsync('Reloading CMake Cache', () => this.updateCodeModel()); }); + + this.config.onChange('sourceDirectory', async () => { + // The configure process can determine correctly whether the features set activation + // should be full or partial, so there is no need to proactively enable full here, + // unless the automatic configure is disabled. + // If there is a configure or a build in progress, we should avoid setting full activation here, + // even if cmake.configureOnEdit is true, because this may overwrite a different decision + // that was done earlier by that ongoing configure process. + if (!this.configOrBuildInProgress()) { + if (this.config.configureOnEdit) { + log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure...")); + await this.configure([]); + } else if (this.workspaceFolder) { + const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file((this.workspaceFolder))); + if (folder) { + await ext.enableFullFeatureSet(true, folder); + } + } + } + }); } doConfigureSettingsChange() { this._needsReconfigure = true; } diff --git a/src/drivers/driver.ts b/src/drivers/driver.ts index f391c5aed..09d6a9f74 100644 --- a/src/drivers/driver.ts +++ b/src/drivers/driver.ts @@ -25,6 +25,7 @@ import * as shlex from '@cmt/shlex'; import * as telemetry from '@cmt/telemetry'; import * as util from '@cmt/util'; import {ConfigureArguments, VariantOption} from '@cmt/variant'; +import {enableFullFeatureSet} from '@cmt/extension'; import * as nls from 'vscode-nls'; nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })(); @@ -593,6 +594,10 @@ export abstract class CMakeDriver implements vscode.Disposable { private buildRunning: boolean = false; + public configOrBuildInProgress() : boolean { + return this.configRunning || this.buildRunning; + } + /** * Perform a clean configure. Deletes cached files before running the config * @param consumer The output consumer @@ -634,6 +639,15 @@ export abstract class CMakeDriver implements vscode.Disposable { return -1; } + // _beforeConfigureOrBuild contains the only scenario when we would want partial features set + // (when CMakeLists.txt is missing from the root folder and from the sourceDirectory location). + // A successful configure or a failed configure with any other reason than the above + // (which is treated within _beforeConfigureOrBuild) still require a full features set. + const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder as string)); + if (folder) { + await enableFullFeatureSet(true, folder); + } + const common_flags = ['--no-warn-unused-cli'].concat(extra_args, this.config.configureArgs); const define_flags = this.generateCMakeSettingsFlags(); const init_cache_flags = this.generateInitCacheFlags(); diff --git a/src/extension.ts b/src/extension.ts index 9620da2a4..cb3f0f514 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -104,6 +104,13 @@ class ExtensionManager implements vscode.Disposable { this._setupSubscriptions(); } await util.setContextValue(MULTI_ROOT_MODE_KEY, this._folders.isMultiRoot); + + // Removing a workspace should trigger a re-evaluation of the partial/full activation mode + // of the extension, because the visibility depends on having at least one folder + // with valid CMakeLists.txt. If that one happens to be this, we need an opportunity + // to hide the commands and status bar. + this._enableFullFeatureSetOnWorkspace = false; + this.enableWorkspaceFoldersFullFeatureSet(); } this._onDidChangeActiveTextEditorSub.dispose(); @@ -127,21 +134,6 @@ class ExtensionManager implements vscode.Disposable { } this._statusBar.setAutoSelectActiveFolder(v); }); - - this._workspaceConfig.onChange('sourceDirectory', async v => { - await this._onDidChangeActiveTextEditorSub.dispose(); - if (v) { - this._onDidChangeActiveTextEditorSub = vscode.window.onDidChangeActiveTextEditor(e => this._onDidChangeActiveTextEditor(e), this); - } else { - this._onDidChangeActiveTextEditorSub = new DummyDisposable(); - } - - // Reset to a full activation mode. - // If required, the extension will switch back to partial activation mode - // after more analysis triggered by the below configure. - await enableFullFeatureSet(true); - await vscode.commands.executeCommand("cmake.configure"); - }); } private _onDidChangeActiveTextEditorSub: vscode.Disposable = new DummyDisposable(); @@ -182,30 +174,24 @@ class ExtensionManager implements vscode.Disposable { telemetry.logEvent('open', telemetryProperties); } - public getActiveFolderContext() : StateManager | undefined { - const activeFolder = this.getActiveFolder(); - if (activeFolder) { - const extensionState = new StateManager(this.extensionContext, activeFolder); - return extensionState; - } - - return undefined; + public getFolderContext(folder: vscode.WorkspaceFolder): StateManager { + return new StateManager(this.extensionContext, folder); } // Partial activation means that the CMake Tools commands are hidden // from the commands pallette and the status bar is not visible. - // The context variable "cmake:enableFullFeatureSet" is always equal - // to the state setting ignoreCMakeListsMissing. - // To have them both always in sync, cmake:enableFullFeatureSet is set - // by the getter and setter of ignoreCMakeListsMissing. - public enableFullFeatureSet(fullFeatureSet: boolean) { - const context = this.getActiveFolderContext(); - if (context) { - context.ignoreCMakeListsMissing = !fullFeatureSet; - this._statusBar.setVisible(fullFeatureSet); - } else { - log.trace(localize('enableFullFeatureSet.no.active.folder', 'enableFullFeatureSet({0}) called but not active folder found.'), fullFeatureSet); - } + // The context variable "cmake:enableFullFeatureSet" (which controls + // all the cmake commands and UI elements) is set to true, + // if there is at least one folder with full features set enabled. + // We need to add this private _enableFullFeaturesSetOnWorkspace here + // because currently there is no way of reading a context variable + // like cmake:enableFullFeatureSet, to apply the OR operation on it. + private _enableFullFeatureSetOnWorkspace = false; + public enableFullFeatureSet(fullFeatureSet: boolean, folder: vscode.WorkspaceFolder) { + this.getFolderContext(folder).ignoreCMakeListsMissing = !fullFeatureSet; + this._enableFullFeatureSetOnWorkspace = this._enableFullFeatureSetOnWorkspace || fullFeatureSet; + util.setContextValue("cmake:enableFullFeatureSet", this._enableFullFeatureSetOnWorkspace); + this._statusBar.setVisible(this._enableFullFeatureSetOnWorkspace); } /** @@ -222,13 +208,6 @@ class ExtensionManager implements vscode.Disposable { * The folder controller manages multiple instances. One per folder. */ private readonly _folders = new CMakeToolsFolderController(this.extensionContext); - public getActiveFolder(): vscode.WorkspaceFolder | undefined { - if (!vscode.workspace.workspaceFolders || vscode.workspace.workspaceFolders.length === 0) { - return undefined; - } - - return this._folders?.activeFolder?.cmakeTools.folder || vscode.workspace.workspaceFolders![0]; - } /** * The status bar controller @@ -938,6 +917,14 @@ class ExtensionManager implements vscode.Disposable { this._statusBar.hideDebugButton(shouldHide); await util.setContextValue(HIDE_DEBUG_COMMAND_KEY, shouldHide); } + + // Helper that loops through all the workspace folders to enable full or partial feature set + // depending on their 'ignoreCMakeListsMissing' state variable. + enableWorkspaceFoldersFullFeatureSet() { + for (const cmtFolder of this._folders) { + this.enableFullFeatureSet(!this.getFolderContext(cmtFolder.folder)?.ignoreCMakeListsMissing, cmtFolder.folder); + } + } } /** @@ -952,8 +939,8 @@ async function setup(context: vscode.ExtensionContext, progress: ProgressHandle) // Load a new extension manager const ext = _EXT_MANAGER = await ExtensionManager.create(context); - // Enable full or partial feature set, depending on state variable. - ext.enableFullFeatureSet(!ext.getActiveFolderContext()?.ignoreCMakeListsMissing); + // Enable full or partial feature set for each workspace folder, depending on their state variable. + ext.enableWorkspaceFoldersFullFeatureSet(); // A register function that helps us bind the commands to the extension function register(name: K) { @@ -1127,8 +1114,8 @@ export async function activate(context: vscode.ExtensionContext) { // context.subscriptions.push(vscode.commands.registerCommand('cmake._extensionInstance', () => cmt)); } -export async function enableFullFeatureSet(fullFeatureSet: boolean) { - _EXT_MANAGER?.enableFullFeatureSet(fullFeatureSet); +export async function enableFullFeatureSet(fullFeatureSet: boolean, folder: vscode.WorkspaceFolder) { + _EXT_MANAGER?.enableFullFeatureSet(fullFeatureSet, folder); } // this method is called when your extension is deactivated diff --git a/src/state.ts b/src/state.ts index 1fb85bd48..17a147ecb 100644 --- a/src/state.ts +++ b/src/state.ts @@ -1,4 +1,3 @@ -import * as util from '@cmt/util'; import * as vscode from 'vscode'; /** @@ -26,15 +25,11 @@ export class StateManager { * Whether the user chose to ignore the popup message about missing CMakeLists.txt * from the root folder, for a code base that is not fully activating CMake Tools. */ - // Set the context var here in both getter and setter? get ignoreCMakeListsMissing(): boolean { - const ignore = this._get('ignoreCMakeListsMissing') || false; - util.setContextValue("cmake:enableFullFeatureSet", !ignore); - return ignore; + return this._get('ignoreCMakeListsMissing') || false; } set ignoreCMakeListsMissing(v: boolean) { this._update('ignoreCMakeListsMissing', v); - util.setContextValue("cmake:enableFullFeatureSet", !v); } /** diff --git a/test/unit-tests/config.test.ts b/test/unit-tests/config.test.ts index 403fa5774..0b6d32bd7 100644 --- a/test/unit-tests/config.test.ts +++ b/test/unit-tests/config.test.ts @@ -39,6 +39,7 @@ function createConfig(conf: Partial): Configurat emscriptenSearchDirs: [], copyCompileCommands: null, configureOnOpen: null, + configureOnEdit: true, skipConfigureIfCachePresent: null, useCMakeServer: true, cmakeCommunicationMode: 'automatic', From 159e0e6886bc9880c35715267ed656b78a623e6d Mon Sep 17 00:00:00 2001 From: Andreea Isac Date: Thu, 11 Jun 2020 09:10:01 -0700 Subject: [PATCH 2/5] Improvements from code review feedback --- package.json | 2 +- src/cmake-tools.ts | 19 +++++++++++++++---- src/drivers/cmfileapi-driver.ts | 2 +- src/drivers/cms-driver.ts | 26 +++++++++++++++++++++++++- src/drivers/driver.ts | 16 +++++++--------- 5 files changed, 49 insertions(+), 16 deletions(-) diff --git a/package.json b/package.json index 300cf3e45..f0594ab5b 100644 --- a/package.json +++ b/package.json @@ -1252,7 +1252,7 @@ "cmake.configureOnEdit": { "type": "boolean", "default": true, - "description": "%cmake-tools.configuration.cmake.configureOnOpen.description%", + "description": "%cmake-tools.configuration.cmake.configureOnEdit.description%", "scope": "resource" }, "cmake.skipConfigureIfCachePresent": { diff --git a/src/cmake-tools.ts b/src/cmake-tools.ts index 24c4dd8ea..f1eb08d12 100644 --- a/src/cmake-tools.ts +++ b/src/cmake-tools.ts @@ -272,13 +272,19 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI { if (cmakeListsFile) { const fullPathDir: string = path.parse(cmakeListsFile[0].fsPath).dir; const relPathDir: string = lightNormalizePath(path.relative(this.folder.uri.fsPath, fullPathDir)); + const joinedPath = "${workspaceFolder}/".concat(relPathDir); + vscode.workspace.getConfiguration('cmake', this.folder.uri).update("sourceDirectory", joinedPath); const drv = await this.getCMakeDriverInstance(); if (drv) { - const joinedPath = "${workspaceFolder}/".concat(relPathDir); - vscode.workspace.getConfiguration('cmake', this.folder.uri).update("sourceDirectory", joinedPath); drv.config.updatePartial({sourceDirectory: joinedPath}); } else { - throw new Error(localize('unable.to.save.sourceDirectory', 'Unable to save "cmake.sourceDirectory". There is no valid cmake driver instance.')); + const reloadWindowButton = localize('reload.window', 'Reload Window'); + const reload = await vscode.window.showErrorMessage(localize('setting.sourceDirectory.failed.needs.reload.window', + 'Something went wrong while updating the cmake.sourceDirectory setting. Please run the "Reload window" command for the change to take effect.'), + reloadWindowButton); + if (reload === reloadWindowButton) { + vscode.commands.executeCommand('workbench.action.reloadWindow'); + } } } else { fullFeatureSet = false; @@ -452,7 +458,8 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI { this.extensionContext.subscriptions.push(vscode.workspace.onDidSaveTextDocument(async td => { const str = td.uri.fsPath; - if (str === path.join(this.folder.uri.fsPath, "CMakeLists.txt")) { + const sourceDirectory = await this.sourceDir; + if (str === path.join(sourceDirectory, "CMakeLists.txt")) { // The configure process can determine correctly whether the features set activation // should be full or partial, so there is no need to proactively enable full here, // unless the automatic configure is disabled. @@ -467,6 +474,10 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI { } else { await enableFullFeatureSet(true, this.folder); } + } else { + log.warning(localize('cmakelists.save.could.not.reconfigure', + 'Detected saving of CMakeLists.txt but could not configure because there is no valid driver or the project is already building/configuring.')); + log.debug(localize('needs.reconfigure', 'The project needs to be reconfigured so that the changes saved in CMakeLists.txt have effect.')); } } })); diff --git a/src/drivers/cmfileapi-driver.ts b/src/drivers/cmfileapi-driver.ts index 396762e0f..a67a94447 100644 --- a/src/drivers/cmfileapi-driver.ts +++ b/src/drivers/cmfileapi-driver.ts @@ -115,7 +115,7 @@ export class CMakeFileApiDriver extends codemodel.CodeModelDriver { log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure...")); await this.configure([]); } else if (this.workspaceFolder) { - const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file((this.workspaceFolder))); + const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder)); if (folder) { await ext.enableFullFeatureSet(true, folder); } diff --git a/src/drivers/cms-driver.ts b/src/drivers/cms-driver.ts index ce3f431d1..69272f6f7 100644 --- a/src/drivers/cms-driver.ts +++ b/src/drivers/cms-driver.ts @@ -15,6 +15,7 @@ import * as proc from '@cmt/proc'; import rollbar from '@cmt/rollbar'; import { ConfigurationReader } from '@cmt/config'; import * as nls from 'vscode-nls'; +import * as ext from '@cmt/extension'; nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })(); const localize: nls.LocalizeFunc = nls.loadMessageBundle(); @@ -319,7 +320,30 @@ export class CMakeServerClientDriver extends codemodel.CodeModelDriver { } } - protected async doInit(): Promise { await this._restartClient(); } + protected async doInit(): Promise { + await this._restartClient(); + + + this.config.onChange('sourceDirectory', async () => { + // The configure process can determine correctly whether the features set activation + // should be full or partial, so there is no need to proactively enable full here, + // unless the automatic configure is disabled. + // If there is a configure or a build in progress, we should avoid setting full activation here, + // even if cmake.configureOnEdit is true, because this may overwrite a different decision + // that was done earlier by that ongoing configure process. + if (!this.configOrBuildInProgress()) { + if (this.config.configureOnEdit) { + log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure...")); + await this.configure([]); + } else if (this.workspaceFolder) { + const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder)); + if (folder) { + await ext.enableFullFeatureSet(true, folder); + } + } + } + }); + } static async create(cmake: CMakeExecutable, config: ConfigurationReader, kit: Kit|null, workspaceFolder: string | null, preconditionHandler: CMakePreconditionProblemSolver, preferredGenerators: CMakeGenerator[]): Promise { return this.createDerived(new CMakeServerClientDriver(cmake, config, workspaceFolder, preconditionHandler), kit, preferredGenerators); diff --git a/src/drivers/driver.ts b/src/drivers/driver.ts index 09d6a9f74..7b46236ed 100644 --- a/src/drivers/driver.ts +++ b/src/drivers/driver.ts @@ -639,15 +639,6 @@ export abstract class CMakeDriver implements vscode.Disposable { return -1; } - // _beforeConfigureOrBuild contains the only scenario when we would want partial features set - // (when CMakeLists.txt is missing from the root folder and from the sourceDirectory location). - // A successful configure or a failed configure with any other reason than the above - // (which is treated within _beforeConfigureOrBuild) still require a full features set. - const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder as string)); - if (folder) { - await enableFullFeatureSet(true, folder); - } - const common_flags = ['--no-warn-unused-cli'].concat(extra_args, this.config.configureArgs); const define_flags = this.generateCMakeSettingsFlags(); const init_cache_flags = this.generateInitCacheFlags(); @@ -877,6 +868,13 @@ export abstract class CMakeDriver implements vscode.Disposable { return false; } + // Returning success here should require a full activation mode, + // even if the configure process will fail later. + const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder as string)); + if (folder) { + await enableFullFeatureSet(true, folder); + } + return true; } From 3c27b408f99809f25066ded13be13ab357287f9b Mon Sep 17 00:00:00 2001 From: Andreea Isac Date: Thu, 11 Jun 2020 12:01:44 -0700 Subject: [PATCH 3/5] Rephrase warning message. --- src/cmake-tools.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cmake-tools.ts b/src/cmake-tools.ts index f1eb08d12..94fd41892 100644 --- a/src/cmake-tools.ts +++ b/src/cmake-tools.ts @@ -476,7 +476,7 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI { } } else { log.warning(localize('cmakelists.save.could.not.reconfigure', - 'Detected saving of CMakeLists.txt but could not configure because there is no valid driver or the project is already building/configuring.')); + 'Changes were detected in CMakeLists.txt but we could not reconfigure the project because another operation is already in progress.')); log.debug(localize('needs.reconfigure', 'The project needs to be reconfigured so that the changes saved in CMakeLists.txt have effect.')); } } From 34c100bc3630fb467c1055e4914afb8acfc9a43b Mon Sep 17 00:00:00 2001 From: andreeis Date: Wed, 17 Jun 2020 07:41:28 -0700 Subject: [PATCH 4/5] experiment, will undo some things after seeing tests results --- src/cmake-tools.ts | 5 +++++ src/drivers/cmfileapi-driver.ts | 40 ++++++++++++++++----------------- src/drivers/cms-driver.ts | 40 ++++++++++++++++----------------- src/drivers/driver.ts | 10 +-------- 4 files changed, 46 insertions(+), 49 deletions(-) diff --git a/src/cmake-tools.ts b/src/cmake-tools.ts index 94fd41892..0e57121ce 100644 --- a/src/cmake-tools.ts +++ b/src/cmake-tools.ts @@ -706,6 +706,11 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI { if (retc === 0) { await this._refreshCompileDatabase(drv.expansionOptions); } + if (retc !== 2) { + // Partial activation mode is required only for -2 error code, + // which represents a missing CMakeLists.txt + await enableFullFeatureSet(true, this.folder); + } await this._ctestController.reloadTests(drv); this._onReconfiguredEmitter.fire(); return retc; diff --git a/src/drivers/cmfileapi-driver.ts b/src/drivers/cmfileapi-driver.ts index 313f66e06..529b4ffea 100644 --- a/src/drivers/cmfileapi-driver.ts +++ b/src/drivers/cmfileapi-driver.ts @@ -21,7 +21,7 @@ import rollbar from '@cmt/rollbar'; import * as util from '@cmt/util'; import * as path from 'path'; import * as vscode from 'vscode'; -import * as ext from '@cmt/extension'; +//import * as ext from '@cmt/extension'; import {NoGeneratorError} from './cms-driver'; @@ -120,25 +120,25 @@ export class CMakeFileApiDriver extends codemodel.CodeModelDriver { rollbar.invokeAsync('Reloading CMake Cache', () => this.updateCodeModel()); }); - this.config.onChange('sourceDirectory', async () => { - // The configure process can determine correctly whether the features set activation - // should be full or partial, so there is no need to proactively enable full here, - // unless the automatic configure is disabled. - // If there is a configure or a build in progress, we should avoid setting full activation here, - // even if cmake.configureOnEdit is true, because this may overwrite a different decision - // that was done earlier by that ongoing configure process. - if (!this.configOrBuildInProgress()) { - if (this.config.configureOnEdit) { - log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure...")); - await this.configure([]); - } else if (this.workspaceFolder) { - const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder)); - if (folder) { - await ext.enableFullFeatureSet(true, folder); - } - } - } - }); + // this.config.onChange('sourceDirectory', async () => { + // // The configure process can determine correctly whether the features set activation + // // should be full or partial, so there is no need to proactively enable full here, + // // unless the automatic configure is disabled. + // // If there is a configure or a build in progress, we should avoid setting full activation here, + // // even if cmake.configureOnEdit is true, because this may overwrite a different decision + // // that was done earlier by that ongoing configure process. + // if (!this.configOrBuildInProgress()) { + // if (this.config.configureOnEdit) { + // log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure...")); + // await this.configure([]); + // } else if (this.workspaceFolder) { + // const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder)); + // if (folder) { + // //await ext.enableFullFeatureSet(true, folder); + // } + // } + // } + // }); } doConfigureSettingsChange() { this._needsReconfigure = true; } diff --git a/src/drivers/cms-driver.ts b/src/drivers/cms-driver.ts index 69272f6f7..2aa078c40 100644 --- a/src/drivers/cms-driver.ts +++ b/src/drivers/cms-driver.ts @@ -15,7 +15,7 @@ import * as proc from '@cmt/proc'; import rollbar from '@cmt/rollbar'; import { ConfigurationReader } from '@cmt/config'; import * as nls from 'vscode-nls'; -import * as ext from '@cmt/extension'; +//import * as ext from '@cmt/extension'; nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })(); const localize: nls.LocalizeFunc = nls.loadMessageBundle(); @@ -324,25 +324,25 @@ export class CMakeServerClientDriver extends codemodel.CodeModelDriver { await this._restartClient(); - this.config.onChange('sourceDirectory', async () => { - // The configure process can determine correctly whether the features set activation - // should be full or partial, so there is no need to proactively enable full here, - // unless the automatic configure is disabled. - // If there is a configure or a build in progress, we should avoid setting full activation here, - // even if cmake.configureOnEdit is true, because this may overwrite a different decision - // that was done earlier by that ongoing configure process. - if (!this.configOrBuildInProgress()) { - if (this.config.configureOnEdit) { - log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure...")); - await this.configure([]); - } else if (this.workspaceFolder) { - const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder)); - if (folder) { - await ext.enableFullFeatureSet(true, folder); - } - } - } - }); + // this.config.onChange('sourceDirectory', async () => { + // // The configure process can determine correctly whether the features set activation + // // should be full or partial, so there is no need to proactively enable full here, + // // unless the automatic configure is disabled. + // // If there is a configure or a build in progress, we should avoid setting full activation here, + // // even if cmake.configureOnEdit is true, because this may overwrite a different decision + // // that was done earlier by that ongoing configure process. + // if (!this.configOrBuildInProgress()) { + // if (this.config.configureOnEdit) { + // log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure...")); + // await this.configure([]); + // } else if (this.workspaceFolder) { + // const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder)); + // if (folder) { + // //await ext.enableFullFeatureSet(true, folder); + // } + // } + // } + // }); } static async create(cmake: CMakeExecutable, config: ConfigurationReader, kit: Kit|null, workspaceFolder: string | null, preconditionHandler: CMakePreconditionProblemSolver, preferredGenerators: CMakeGenerator[]): Promise { diff --git a/src/drivers/driver.ts b/src/drivers/driver.ts index 7b46236ed..8f6e2594b 100644 --- a/src/drivers/driver.ts +++ b/src/drivers/driver.ts @@ -25,7 +25,6 @@ import * as shlex from '@cmt/shlex'; import * as telemetry from '@cmt/telemetry'; import * as util from '@cmt/util'; import {ConfigureArguments, VariantOption} from '@cmt/variant'; -import {enableFullFeatureSet} from '@cmt/extension'; import * as nls from 'vscode-nls'; nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })(); @@ -636,7 +635,7 @@ export abstract class CMakeDriver implements vscode.Disposable { const pre_check_ok = await this._beforeConfigureOrBuild(); if (!pre_check_ok) { - return -1; + return -2; } const common_flags = ['--no-warn-unused-cli'].concat(extra_args, this.config.configureArgs); @@ -868,13 +867,6 @@ export abstract class CMakeDriver implements vscode.Disposable { return false; } - // Returning success here should require a full activation mode, - // even if the configure process will fail later. - const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder as string)); - if (folder) { - await enableFullFeatureSet(true, folder); - } - return true; } From dd39230c7b0e6e2d56440846525449439bcaeb08 Mon Sep 17 00:00:00 2001 From: andreeis Date: Wed, 17 Jun 2020 07:57:24 -0700 Subject: [PATCH 5/5] Fix one test baseline. Check if bringing back the sourceDirectory listeners and call to enableFullFeatureSet still causes Electron issue. --- src/drivers/cmfileapi-driver.ts | 40 +++++++++++++-------------- src/drivers/cms-driver.ts | 40 +++++++++++++-------------- test/unit-tests/driver/driver-test.ts | 2 +- 3 files changed, 41 insertions(+), 41 deletions(-) diff --git a/src/drivers/cmfileapi-driver.ts b/src/drivers/cmfileapi-driver.ts index 529b4ffea..313f66e06 100644 --- a/src/drivers/cmfileapi-driver.ts +++ b/src/drivers/cmfileapi-driver.ts @@ -21,7 +21,7 @@ import rollbar from '@cmt/rollbar'; import * as util from '@cmt/util'; import * as path from 'path'; import * as vscode from 'vscode'; -//import * as ext from '@cmt/extension'; +import * as ext from '@cmt/extension'; import {NoGeneratorError} from './cms-driver'; @@ -120,25 +120,25 @@ export class CMakeFileApiDriver extends codemodel.CodeModelDriver { rollbar.invokeAsync('Reloading CMake Cache', () => this.updateCodeModel()); }); - // this.config.onChange('sourceDirectory', async () => { - // // The configure process can determine correctly whether the features set activation - // // should be full or partial, so there is no need to proactively enable full here, - // // unless the automatic configure is disabled. - // // If there is a configure or a build in progress, we should avoid setting full activation here, - // // even if cmake.configureOnEdit is true, because this may overwrite a different decision - // // that was done earlier by that ongoing configure process. - // if (!this.configOrBuildInProgress()) { - // if (this.config.configureOnEdit) { - // log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure...")); - // await this.configure([]); - // } else if (this.workspaceFolder) { - // const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder)); - // if (folder) { - // //await ext.enableFullFeatureSet(true, folder); - // } - // } - // } - // }); + this.config.onChange('sourceDirectory', async () => { + // The configure process can determine correctly whether the features set activation + // should be full or partial, so there is no need to proactively enable full here, + // unless the automatic configure is disabled. + // If there is a configure or a build in progress, we should avoid setting full activation here, + // even if cmake.configureOnEdit is true, because this may overwrite a different decision + // that was done earlier by that ongoing configure process. + if (!this.configOrBuildInProgress()) { + if (this.config.configureOnEdit) { + log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure...")); + await this.configure([]); + } else if (this.workspaceFolder) { + const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder)); + if (folder) { + await ext.enableFullFeatureSet(true, folder); + } + } + } + }); } doConfigureSettingsChange() { this._needsReconfigure = true; } diff --git a/src/drivers/cms-driver.ts b/src/drivers/cms-driver.ts index 2aa078c40..69272f6f7 100644 --- a/src/drivers/cms-driver.ts +++ b/src/drivers/cms-driver.ts @@ -15,7 +15,7 @@ import * as proc from '@cmt/proc'; import rollbar from '@cmt/rollbar'; import { ConfigurationReader } from '@cmt/config'; import * as nls from 'vscode-nls'; -//import * as ext from '@cmt/extension'; +import * as ext from '@cmt/extension'; nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })(); const localize: nls.LocalizeFunc = nls.loadMessageBundle(); @@ -324,25 +324,25 @@ export class CMakeServerClientDriver extends codemodel.CodeModelDriver { await this._restartClient(); - // this.config.onChange('sourceDirectory', async () => { - // // The configure process can determine correctly whether the features set activation - // // should be full or partial, so there is no need to proactively enable full here, - // // unless the automatic configure is disabled. - // // If there is a configure or a build in progress, we should avoid setting full activation here, - // // even if cmake.configureOnEdit is true, because this may overwrite a different decision - // // that was done earlier by that ongoing configure process. - // if (!this.configOrBuildInProgress()) { - // if (this.config.configureOnEdit) { - // log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure...")); - // await this.configure([]); - // } else if (this.workspaceFolder) { - // const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder)); - // if (folder) { - // //await ext.enableFullFeatureSet(true, folder); - // } - // } - // } - // }); + this.config.onChange('sourceDirectory', async () => { + // The configure process can determine correctly whether the features set activation + // should be full or partial, so there is no need to proactively enable full here, + // unless the automatic configure is disabled. + // If there is a configure or a build in progress, we should avoid setting full activation here, + // even if cmake.configureOnEdit is true, because this may overwrite a different decision + // that was done earlier by that ongoing configure process. + if (!this.configOrBuildInProgress()) { + if (this.config.configureOnEdit) { + log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure...")); + await this.configure([]); + } else if (this.workspaceFolder) { + const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder)); + if (folder) { + await ext.enableFullFeatureSet(true, folder); + } + } + } + }); } static async create(cmake: CMakeExecutable, config: ConfigurationReader, kit: Kit|null, workspaceFolder: string | null, preconditionHandler: CMakePreconditionProblemSolver, preferredGenerators: CMakeGenerator[]): Promise { diff --git a/test/unit-tests/driver/driver-test.ts b/test/unit-tests/driver/driver-test.ts index 171beaf5b..1635cf7ca 100644 --- a/test/unit-tests/driver/driver-test.ts +++ b/test/unit-tests/driver/driver-test.ts @@ -180,7 +180,7 @@ export function makeDriverTestsuite(driver_generator: (cmake: CMakeExecutable, }; driver = await driver_generator(executable, config, kitDefault, emptyWorkspaceFolder, checkPreconditionHelper, []); - expect(await driver.cleanConfigure([])).to.be.eq(-1); + expect(await driver.cleanConfigure([])).to.be.eq(-2); expect(called).to.be.true; }).timeout(60000);