Skip to content

Commit

Permalink
Support multiple root cpp build configurations
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@vince-fugnitto
vince-fugnitto committed Mar 26, 2019
1 parent 6d605f8 commit 8af9c27
Show file tree
Hide file tree
Showing 5 changed files with 114 additions and 52 deletions.
1 change: 1 addition & 0 deletions packages/cpp/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
19 changes: 15 additions & 4 deletions packages/cpp/src/browser/cpp-build-configurations-statusbar-element.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}

/**
Expand All @@ -45,14 +45,25 @@ 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,
priority: 0.5,
});
}

/**
* 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;
}

}
51 changes: 42 additions & 9 deletions packages/cpp/src/browser/cpp-build-configurations.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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';
Expand All @@ -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();
Expand All @@ -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>(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,
Expand All @@ -66,8 +87,14 @@ async function initializeTest(buildConfigurations: CppBuildConfiguration[] | und
// Save active build config
if (activeBuildConfigName !== undefined) {
const storage = container.get<StorageService>(StorageService);
storage.setData('cpp.active-build-configuration', {
configName: activeBuildConfigName,
storage.setData('cpp.active-build-configurations-map', {
configs: [[
'/tmp',
{
name: 'Release',
directory: '/tmp/builds/release',
}
]],
});
}

Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
85 changes: 51 additions & 34 deletions packages/cpp/src/browser/cpp-build-configurations.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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');
Expand All @@ -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<string, CppBuildConfiguration | undefined>;

/**
* Event emitted when the active build configuration changes.
Expand Down Expand Up @@ -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<string, CppBuildConfiguration | undefined>
= new Map<string, CppBuildConfiguration | undefined>();

/**
* Emitter for when the active build configuration changes.
*/
protected readonly activeConfigChangeEmitter = new Emitter<CppBuildConfiguration | undefined>();

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<void>;

Expand All @@ -143,17 +153,14 @@ export class CppBuildConfigurationManagerImpl implements CppBuildConfigurationMa
* Load the active build configuration from persistent storage.
*/
protected async loadActiveConfiguration(): Promise<void> {
const savedConfig =
await this.storageService.getData<SavedActiveBuildConfiguration>(
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<SavedActiveBuildConfigurations>(
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]
));
}
}

Expand All @@ -162,11 +169,13 @@ export class CppBuildConfigurationManagerImpl implements CppBuildConfigurationMa
*
* @param config the active `CppBuildConfiguration`.
*/
protected saveActiveConfiguration(config: CppBuildConfiguration | undefined): void {
this.storageService.setData<SavedActiveBuildConfiguration>(
this.ACTIVE_BUILD_CONFIGURATION_STORAGE_KEY, {
configName: config ? config.name : undefined,
});
protected saveActiveConfiguration(configs: Map<string, CppBuildConfiguration | undefined>): void {
const normalizedMap = Array.from(configs.entries()).map<[string, CppBuildConfiguration | undefined]>(
([key, value]) => [key.toString(), value]
);
this.storageService.setData<SavedActiveBuildConfigurations>(
this.ACTIVE_BUILD_CONFIGURATIONS_MAP_STORAGE_KEY, { configs: normalizedMap }
);
}

/**
Expand All @@ -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<string, CppBuildConfiguration | undefined> {
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);
}

Expand Down
10 changes: 5 additions & 5 deletions packages/cpp/src/browser/cpp-task-provider.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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((<ProcessTaskConfiguration>resolvedTask).cwd === '/tmp/build1');
expect((<ProcessTaskConfiguration>resolvedTask).command === 'very');
expect((<ProcessTaskConfiguration>resolvedTask).cwd).to.be.equal('/tmp/build2');
expect((<ProcessTaskConfiguration>resolvedTask).command).to.be.equal('very');
expect((<ProcessTaskConfiguration>resolvedTask).args).to.deep.equal(['complex', 'command']);
});
});

0 comments on commit 8af9c27

Please sign in to comment.