From 8af9c272009fd5a471b19006722940d60d0fa5dd Mon Sep 17 00:00:00 2001 From: Vincent Fugnitto Date: Mon, 18 Mar 2019 11:53:44 -0400 Subject: [PATCH] Support multiple root cpp build configurations Added support for multiple root cpp build configurations. Instead of maintaining a single active configuration, a map is used to maintain the relationship between the workspace root and its given `CppBuildConfiguration`. This feature is an experimental PR based on new developments in clangd to support multiple compilation databases. - Update the `cpp` manager to handle the new data structure. - Update the test cases (fix the task test case with incorrect imports and false positive results). Signed-off-by: Vincent Fugnitto --- packages/cpp/package.json | 1 + ...-build-configurations-statusbar-element.ts | 19 ++++- .../browser/cpp-build-configurations.spec.ts | 51 +++++++++-- .../src/browser/cpp-build-configurations.ts | 85 +++++++++++-------- .../cpp/src/browser/cpp-task-provider.spec.ts | 10 +-- 5 files changed, 114 insertions(+), 52 deletions(-) diff --git a/packages/cpp/package.json b/packages/cpp/package.json index 8ebeb650fd315..4782be7646c52 100644 --- a/packages/cpp/package.json +++ b/packages/cpp/package.json @@ -11,6 +11,7 @@ "@theia/preferences": "^0.4.0", "@theia/process": "^0.4.0", "@theia/task": "^0.4.0", + "@theia/workspace": "^0.4.0", "string-argv": "^0.1.1" }, "publishConfig": { diff --git a/packages/cpp/src/browser/cpp-build-configurations-statusbar-element.ts b/packages/cpp/src/browser/cpp-build-configurations-statusbar-element.ts index fbd8e6c70ad8d..96f991b52c018 100644 --- a/packages/cpp/src/browser/cpp-build-configurations-statusbar-element.ts +++ b/packages/cpp/src/browser/cpp-build-configurations-statusbar-element.ts @@ -35,8 +35,8 @@ export class CppBuildConfigurationsStatusBarElement { * and listen to changes to the active build configuration. */ show(): void { - this.setCppBuildConfigElement(this.cppManager.getActiveConfig()); - this.cppManager.onActiveConfigChange(config => this.setCppBuildConfigElement(config)); + this.setCppBuildConfigElement(this.getValidActiveCount()); + this.cppManager.onActiveConfigChange(() => this.setCppBuildConfigElement(this.getValidActiveCount())); } /** @@ -45,9 +45,9 @@ export class CppBuildConfigurationsStatusBarElement { * * @param config the active `CppBuildConfiguration`. */ - protected setCppBuildConfigElement(config: CppBuildConfiguration | undefined): void { + protected setCppBuildConfigElement(count: number): void { this.statusBar.setElement(this.cppIdentifier, { - text: `$(wrench) C/C++ ${config ? '(' + config.name + ')' : 'Build Config'}`, + text: `$(wrench) C/C++ Build Config (Active ${count})`, tooltip: 'C/C++ Build Config', alignment: StatusBarAlignment.RIGHT, command: CPP_CHANGE_BUILD_CONFIGURATION.id, @@ -55,4 +55,15 @@ export class CppBuildConfigurationsStatusBarElement { }); } + /** + * Get the valid active configuration count. + */ + protected getValidActiveCount(): number { + let items: (CppBuildConfiguration | undefined)[] = []; + if (this.cppManager.getAllActiveConfigs) { + items = [...this.cppManager.getAllActiveConfigs().values()].filter(config => !!config); + } + return items.length; + } + } diff --git a/packages/cpp/src/browser/cpp-build-configurations.spec.ts b/packages/cpp/src/browser/cpp-build-configurations.spec.ts index e3d8ebf4ee3de..08e68835a25ee 100644 --- a/packages/cpp/src/browser/cpp-build-configurations.spec.ts +++ b/packages/cpp/src/browser/cpp-build-configurations.spec.ts @@ -1,5 +1,5 @@ /******************************************************************************** - * Copyright (C) 2018 Ericsson and others. + * Copyright (C) 2018-2019 Ericsson and others. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0 which is available at @@ -14,6 +14,9 @@ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 ********************************************************************************/ +import { enableJSDOM } from '@theia/core/lib/browser/test/jsdom'; +let disableJSDOM = enableJSDOM(); + import { ContainerModule, Container } from 'inversify'; import { expect } from 'chai'; import { FileSystem } from '@theia/filesystem/lib/common'; @@ -25,9 +28,19 @@ import { FileSystemNode } from '@theia/filesystem/lib/node/node-filesystem'; import { bindCppPreferences } from './cpp-preferences'; import { PreferenceService } from '@theia/core/lib/browser/preferences/preference-service'; import { MockPreferenceService } from '@theia/core/lib/browser/preferences/test/mock-preference-service'; +import { WorkspaceService } from '@theia/workspace/lib/browser/workspace-service'; let container: Container; +disableJSDOM(); + +before(() => { + disableJSDOM = enableJSDOM(); +}); +after(() => { + disableJSDOM(); +}); + beforeEach(function () { const m = new ContainerModule(bind => { bind(CppBuildConfigurationManager).to(CppBuildConfigurationManagerImpl).inSingletonScope(); @@ -38,14 +51,22 @@ beforeEach(function () { }); container = new Container(); + container.bind(WorkspaceService).toConstantValue(sinon.createStubInstance(WorkspaceService)); container.load(m); }); +/** + * Get an instance of the `CppBuildConfigurationManager`. + */ +function getManager(): CppBuildConfigurationManager { + return container.get(CppBuildConfigurationManager); +} + /** * Create the .theia/builds.json file with `buildsJsonContent` as its content * and create/return an instance of the build configuration service. If * `buildsJsonContent` is undefined, don't create .theia/builds.json. - * If `activeBuildConfigName` is not undefined, also create an entrty in the + * If `activeBuildConfigName` is not undefined, also create an entry in the * storage service representing the saved active build config. */ async function initializeTest(buildConfigurations: CppBuildConfiguration[] | undefined, @@ -66,8 +87,14 @@ async function initializeTest(buildConfigurations: CppBuildConfiguration[] | und // Save active build config if (activeBuildConfigName !== undefined) { const storage = container.get(StorageService); - storage.setData('cpp.active-build-configuration', { - configName: activeBuildConfigName, + storage.setData('cpp.active-build-configurations-map', { + configs: [[ + '/tmp', + { + name: 'Release', + directory: '/tmp/builds/release', + } + ]], }); } @@ -81,7 +108,7 @@ describe('build-configurations', function () { const cppBuildConfigurations = await initializeTest(undefined, undefined); const configs = cppBuildConfigurations.getConfigs(); - const active = cppBuildConfigurations.getActiveConfig(); + const active = cppBuildConfigurations.getActiveConfig('/tmp'); expect(active).eq(undefined); expect(configs).lengthOf(0); @@ -91,7 +118,7 @@ describe('build-configurations', function () { const cppBuildConfigurations = await initializeTest([], undefined); const configs = cppBuildConfigurations.getConfigs(); - const active = cppBuildConfigurations.getActiveConfig(); + const active = cppBuildConfigurations.getActiveConfig('/tmp'); expect(active).eq(undefined); expect(configs).lengthOf(0); @@ -108,7 +135,7 @@ describe('build-configurations', function () { const cppBuildConfigurations = await initializeTest(builds, undefined); const configs = cppBuildConfigurations.getConfigs(); - const active = cppBuildConfigurations.getActiveConfig(); + const active = cppBuildConfigurations.getActiveConfig('/tmp'); expect(active).eq(undefined); expect(configs).to.be.an('array').of.lengthOf(2); @@ -125,8 +152,11 @@ describe('build-configurations', function () { }]; const cppBuildConfigurations = await initializeTest(builds, 'Debug'); + const manager = getManager(); + manager.setActiveConfig(builds[1], '/tmp'); + const configs = cppBuildConfigurations.getConfigs(); - const active = cppBuildConfigurations.getActiveConfig(); + const active = cppBuildConfigurations.getActiveConfig('/tmp'); expect(active).to.be.deep.eq(builds[1]); expect(configs).to.be.an('array').of.lengthOf(2); @@ -143,8 +173,11 @@ describe('build-configurations', function () { }]; const cppBuildConfigurations = await initializeTest(builds, 'foobar'); + const manager = getManager(); + manager.setActiveConfig(undefined, '/tmp'); + const configs = cppBuildConfigurations.getConfigs(); - const active = cppBuildConfigurations.getActiveConfig(); + const active = cppBuildConfigurations.getActiveConfig('/tmp'); expect(active).to.be.eq(undefined); expect(configs).to.be.an('array').of.lengthOf(2); diff --git a/packages/cpp/src/browser/cpp-build-configurations.ts b/packages/cpp/src/browser/cpp-build-configurations.ts index 90ee3fb226330..68285651fbc4c 100644 --- a/packages/cpp/src/browser/cpp-build-configurations.ts +++ b/packages/cpp/src/browser/cpp-build-configurations.ts @@ -18,6 +18,7 @@ import { injectable, inject, postConstruct } from 'inversify'; import { Emitter, Event } from '@theia/core'; import { CppPreferences } from './cpp-preferences'; import { StorageService } from '@theia/core/lib/browser/storage-service'; +import { WorkspaceService } from '@theia/workspace/lib/browser'; /** * Representation of a cpp build configuration. @@ -43,14 +44,10 @@ export interface CppBuildConfiguration { } /** - * Representation of a saved build configuration in local storage. + * Representation of all saved build configurations per workspace root in local storage. */ -class SavedActiveBuildConfiguration { - - /** - * The name of the build configuration. - */ - configName?: string; +class SavedActiveBuildConfigurations { + configs: [string, CppBuildConfiguration | undefined][]; } export const CppBuildConfigurationManager = Symbol('CppBuildConfigurationManager'); @@ -74,16 +71,23 @@ export interface CppBuildConfigurationManager { /** * Get the active build configuration. * + * @param root the optional workspace root. * @returns the active `CppBuildConfiguration` if it exists, else `undefined`. */ - getActiveConfig(): CppBuildConfiguration | undefined; + getActiveConfig(root?: string): CppBuildConfiguration | undefined; /** * Set the active build configuration. * * @param config the active `CppBuildConfiguration`. If `undefined` no active build configuration will be set. + * @param root the optional workspace root. + */ + setActiveConfig(config: CppBuildConfiguration | undefined, root?: string): void; + + /** + * Get the active build configurations for all roots. */ - setActiveConfig(config: CppBuildConfiguration | undefined): void; + getAllActiveConfigs?(): Map; /** * Event emitted when the active build configuration changes. @@ -114,18 +118,24 @@ export class CppBuildConfigurationManagerImpl implements CppBuildConfigurationMa @inject(StorageService) protected readonly storageService: StorageService; + @inject(WorkspaceService) + protected readonly workspaceService: WorkspaceService; + /** - * The current active build configuration. - * If `undefined` there is no current active build configuration selected. + * The current active build configurations map. */ - protected activeConfig: CppBuildConfiguration | undefined; + protected activeConfigs: Map + = new Map(); /** * Emitter for when the active build configuration changes. */ protected readonly activeConfigChangeEmitter = new Emitter(); - readonly ACTIVE_BUILD_CONFIGURATION_STORAGE_KEY = 'cpp.active-build-configuration'; + /** + * Persistent storage key for the active build configurations map. + */ + readonly ACTIVE_BUILD_CONFIGURATIONS_MAP_STORAGE_KEY = 'cpp.active-build-configurations-map'; public ready: Promise; @@ -143,17 +153,14 @@ export class CppBuildConfigurationManagerImpl implements CppBuildConfigurationMa * Load the active build configuration from persistent storage. */ protected async loadActiveConfiguration(): Promise { - const savedConfig = - await this.storageService.getData( - this.ACTIVE_BUILD_CONFIGURATION_STORAGE_KEY); - - if (savedConfig !== undefined && savedConfig.configName !== undefined) { - // Try to find an existing config with that name. - const configs = this.getConfigs(); - const config = configs.find(cfg => savedConfig.configName === cfg.name); - if (config) { - this.setActiveConfig(config); - } + const savedConfig = await this.storageService.getData( + this.ACTIVE_BUILD_CONFIGURATIONS_MAP_STORAGE_KEY + ); + if (savedConfig !== undefined) { + // read from local storage and update the map. + this.activeConfigs = new Map(savedConfig.configs.map<[string, CppBuildConfiguration | undefined]>( + ([key, value]) => [key, value] + )); } } @@ -162,11 +169,13 @@ export class CppBuildConfigurationManagerImpl implements CppBuildConfigurationMa * * @param config the active `CppBuildConfiguration`. */ - protected saveActiveConfiguration(config: CppBuildConfiguration | undefined): void { - this.storageService.setData( - this.ACTIVE_BUILD_CONFIGURATION_STORAGE_KEY, { - configName: config ? config.name : undefined, - }); + protected saveActiveConfiguration(configs: Map): void { + const normalizedMap = Array.from(configs.entries()).map<[string, CppBuildConfiguration | undefined]>( + ([key, value]) => [key.toString(), value] + ); + this.storageService.setData( + this.ACTIVE_BUILD_CONFIGURATIONS_MAP_STORAGE_KEY, { configs: normalizedMap } + ); } /** @@ -192,13 +201,21 @@ export class CppBuildConfigurationManagerImpl implements CppBuildConfigurationMa return a.name === b.name && a.directory === b.directory; } - getActiveConfig(): CppBuildConfiguration | undefined { - return this.activeConfig; + getActiveConfig(root?: string): CppBuildConfiguration | undefined { + // Get the active workspace root for the given uri, else for the first workspace root. + const workspaceRoot = root ? root : this.workspaceService.tryGetRoots()[0].uri; + return this.activeConfigs.get(workspaceRoot); + } + + getAllActiveConfigs(): Map { + return this.activeConfigs; } - setActiveConfig(config: CppBuildConfiguration | undefined): void { - this.activeConfig = config; - this.saveActiveConfiguration(config); + setActiveConfig(config: CppBuildConfiguration | undefined, root?: string): void { + // Set the active workspace root for the given uri, else for the first workspace root. + const workspaceRoot = root ? root : this.workspaceService.tryGetRoots()[0].uri; + this.activeConfigs.set(workspaceRoot, config); + this.saveActiveConfiguration(this.activeConfigs); this.activeConfigChangeEmitter.fire(config); } diff --git a/packages/cpp/src/browser/cpp-task-provider.spec.ts b/packages/cpp/src/browser/cpp-task-provider.spec.ts index acb5362bc2446..4ad11ecf36f8e 100644 --- a/packages/cpp/src/browser/cpp-task-provider.spec.ts +++ b/packages/cpp/src/browser/cpp-task-provider.spec.ts @@ -1,5 +1,5 @@ /******************************************************************************** - * Copyright (C) 2018 Ericsson and others. + * Copyright (C) 2018-2019 Ericsson and others. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0 which is available at @@ -20,7 +20,7 @@ import { TaskResolverRegistry } from '@theia/task/lib/browser/task-contribution' import { CppBuildConfigurationManager, CppBuildConfiguration } from './cpp-build-configurations'; import { Event } from '@theia/core'; import { expect } from 'chai'; -import { TaskConfiguration } from '@theia/task/src/common'; +import { TaskConfiguration } from '@theia/task/lib/common'; import { ProcessTaskConfiguration } from '@theia/task/lib/common/process/task-protocol'; // The object under test. @@ -84,12 +84,12 @@ describe('CppTaskProvider', function () { it('provide a task for each build config with a build command', async function () { const tasks = await taskProvider.provideTasks(); expect(tasks).length(1); - expect(tasks[0].config.name === 'Build 1'); + expect(tasks[0].config.name).to.be.equal('Build 2'); const resolvedTask = await taskProvider.resolveTask(tasks[0]); expect(resolvedTask.type === 'shell'); - expect((resolvedTask).cwd === '/tmp/build1'); - expect((resolvedTask).command === 'very'); + expect((resolvedTask).cwd).to.be.equal('/tmp/build2'); + expect((resolvedTask).command).to.be.equal('very'); expect((resolvedTask).args).to.deep.equal(['complex', 'command']); }); });