Skip to content

Commit

Permalink
Correctly store cache and update binaryDir (#4028)
Browse files Browse the repository at this point in the history 
* fix binary dir

* fixes the issue, but more testing and possible improvement to come

* set expandedPreset.binaryDir if already set too

* properly update caches, ensure we correctly store cached presets

* remove some unnecessary comments

* fix linter errors

* update envOverride with expandedPreset env

* update the changelog

* update parent environment when never
  • Loading branch information
@gcampbell-msft
gcampbell-msft authored Aug 30, 2024
1 parent 5fbcb13 commit f35be38
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 52 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
Bug Fixes:

- Fix env expansion of all variables (toolchainFile, etc.) in presets. [#4019](https://github.com/microsoft/vscode-cmake-tools/issues/4019)
- Fix issues with inheritance and presets. [#4023](https://github.com/microsoft/vscode-cmake-tools/issues/4023)
- Fix generator and preferredGenerator logic. [#4031](https://github.com/microsoft/vscode-cmake-tools/issues/4031), [#4005](https://github.com/microsoft/vscode-cmake-tools/issues/4005), [#4032](https://github.com/microsoft/vscode-cmake-tools/issues/4032)

## 1.19.49
Expand Down
8 changes: 4 additions & 4 deletions src/cmakeProject.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ export class CMakeProject {
}
}

preset.cacheExpandedPreset(this.folderPath, expandedConfigurePreset, "configurePresets");
preset.updateCachedExpandedPreset(this.folderPath, expandedConfigurePreset, "configurePresets");

// Make sure we pass CMakeDriver the preset defined env as well as the parent env
expandedConfigurePreset.environment = EnvironmentUtils.mergePreserveNull([expandedConfigurePreset.__parentEnvironment, expandedConfigurePreset.environment]);
Expand Down Expand Up @@ -407,7 +407,7 @@ export class CMakeProject {
}

if (expandedBuildPreset.name !== preset.defaultBuildPreset.name) {
preset.cacheExpandedPreset(this.folderPath, expandedBuildPreset, "buildPresets");
preset.updateCachedExpandedPreset(this.folderPath, expandedBuildPreset, "buildPresets");
}

// Make sure we pass CMakeDriver the preset defined env as well as the parent env
Expand Down Expand Up @@ -501,7 +501,7 @@ export class CMakeProject {
}

if (expandedTestPreset.name !== preset.defaultTestPreset.name) {
preset.cacheExpandedPreset(this.folderPath, expandedTestPreset, "testPresets");
preset.updateCachedExpandedPreset(this.folderPath, expandedTestPreset, "testPresets");
}

// Make sure we pass CMakeDriver the preset defined env as well as the parent env
Expand Down Expand Up @@ -595,7 +595,7 @@ export class CMakeProject {
}

if (expandedPackagePreset.name !== preset.defaultPackagePreset.name) {
preset.cacheExpandedPreset(this.folderPath, expandedPackagePreset, "packagePresets");
preset.updateCachedExpandedPreset(this.folderPath, expandedPackagePreset, "packagePresets");
}

// Make sure we pass CMakeDriver the preset defined env as well as the parent env
Expand Down
80 changes: 58 additions & 22 deletions src/preset.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -492,26 +492,54 @@ export function setExpandedPresets(folder: string, presets: PresetsFile | undefi
expandedPresets.set(folder, presets);
}

export function cacheExpandedPreset(folder: string, preset: Preset, presetType: 'configurePresets' | 'buildPresets' | 'testPresets' | 'packagePresets' | 'workflowPresets') {
const expanded = expandedPresets.get(folder);
/**
* This function updates the cache in both the regular presets cache and user presets cache.
* However, this only updates the cache if the preset was already in the cache.
* @param folder Folder to grab the cached expanded presets for
* @param preset The updated preset to cache
* @param presetType Type of the preset.
*/
export function updateCachedExpandedPreset(folder: string, preset: Preset, presetType: 'configurePresets' | 'buildPresets' | 'testPresets' | 'packagePresets' | 'workflowPresets') {
const clonedPreset = lodash.cloneDeep(preset);
if (expanded) {
if (presetType === 'configurePresets') {
expanded.configurePresets = expanded.configurePresets?.filter(p => p.name !== clonedPreset.name);
expanded.configurePresets?.push(clonedPreset as ConfigurePreset);
} else if (presetType === "buildPresets") {
expanded.buildPresets = expanded.buildPresets?.filter(p => p.name !== clonedPreset.name);
expanded.buildPresets?.push(clonedPreset as BuildPreset);
} else if (presetType === "testPresets") {
expanded.testPresets = expanded.testPresets?.filter(p => p.name !== clonedPreset.name);
expanded.testPresets?.push(clonedPreset as TestPreset);
} else if (presetType === "packagePresets") {
expanded.packagePresets = expanded.packagePresets?.filter(p => p.name !== clonedPreset.name);
expanded.packagePresets?.push(clonedPreset as PackagePreset);
} else if (presetType === "workflowPresets") {
expanded.workflowPresets = expanded.workflowPresets?.filter(p => p.name !== clonedPreset.name);
expanded.workflowPresets?.push(clonedPreset as WorkflowPreset);
}
const expanded = expandedPresets.get(folder);
const userExpanded = expandedUserPresets.get(folder);
updateCachedExpandedPresethelper(expanded, clonedPreset, presetType);
updateCachedExpandedPresethelper(userExpanded, clonedPreset, presetType);
}

/**
* Updates the cache only if the preset was already present in the cache.
* Updates the cache in-place, the sorting of the list will remain the same.
* @param cache The cache to update.
* @param preset The updated preset to cache
* @param presetType Type of the preset.
* @returns void
*/
function updateCachedExpandedPresethelper(cache: PresetsFile | undefined, preset: Preset, presetType: 'configurePresets' | 'buildPresets' | 'testPresets' | 'packagePresets' | 'workflowPresets') {
// Exit early if the cache or the list of presets is undefined.
if (!cache || !cache[presetType]) {
return;
}

// Exit early if the cache doesn't contain the preset.
const index = cache[presetType]!.findIndex(p => p.name === preset.name);
if (index === -1) {
return;
}

// TODO: I'd like to try and figure out how to template this so that we don't have this logic duplicated for each if statement.
// We know that the list exists so we use "!".
// We use slice so that we can insert the updated preset in the same location it was previously in.
if (presetType === 'configurePresets') {
cache.configurePresets = [...cache.configurePresets!.slice(0, index), preset as ConfigurePreset, ...cache.configurePresets!.slice(index + 1)];
} else if (presetType === "buildPresets") {
cache.buildPresets = [...cache.buildPresets!.slice(0, index), preset as BuildPreset, ...cache.buildPresets!.slice(index + 1)];
} else if (presetType === "testPresets") {
cache.testPresets = [...cache.testPresets!.slice(0, index), preset as TestPreset, ...cache.testPresets!.slice(index + 1)];
} else if (presetType === "packagePresets") {
cache.packagePresets = [...cache.packagePresets!.slice(0, index), preset as PackagePreset, ...cache.packagePresets!.slice(index + 1)];
} else if (presetType === "workflowPresets") {
cache.workflowPresets = [...cache.workflowPresets!.slice(0, index), preset as WorkflowPreset, ...cache.workflowPresets!.slice(index + 1)];
}
}

Expand Down Expand Up @@ -1056,6 +1084,7 @@ async function getVsDevEnv(opts: VsDevEnvOptions): Promise<EnvironmentWithNull |
export async function tryApplyVsDevEnv(preset: ConfigurePreset, workspaceFolder: string, sourceDir: string): Promise<void> {
const useVsDeveloperEnvironmentMode = vscode.workspace.getConfiguration("cmake", vscode.Uri.file(workspaceFolder)).get("useVsDeveloperEnvironment") as UseVsDeveloperEnvironment;
if (useVsDeveloperEnvironmentMode === "never") {
preset.__parentEnvironment = process.env;
return;
}

Expand Down Expand Up @@ -1187,6 +1216,7 @@ async function getConfigurePresetInheritsImpl(folder: string, name: string, allo
return null;
}

// This function modifies the preset parameter in-place. This means that the cache will be updated if the preset was retreived from the cache and not cloned.
async function getConfigurePresetInheritsHelper(folder: string, preset: ConfigurePreset, allowUserPreset: boolean = false, usePresetsPlusIncluded: boolean = false, errorHandler?: ExpansionErrorHandler) {
if (preset.__expanded) {
return preset;
Expand Down Expand Up @@ -1262,6 +1292,7 @@ async function getConfigurePresetInheritsHelper(folder: string, preset: Configur
return preset;
}

// This function does not modify the preset in place, it constructs a new expanded preset and returns it.
export async function expandConfigurePresetVariables(preset: ConfigurePreset, folder: string, name: string, workspaceFolder: string, sourceDir: string, allowUserPreset: boolean = false, usePresetsPlusIncluded: boolean = false, errorHandler?: ExpansionErrorHandler): Promise<ConfigurePreset> {

// Put the preset.environment on top of combined environment in the `__parentEnvironment` field.
Expand All @@ -1286,19 +1317,22 @@ export async function expandConfigurePresetVariables(preset: ConfigurePreset, fo

expansionOpts.envOverride = EnvironmentUtils.mergePreserveNull([env, expandedPreset.environment]);

expandedPreset.binaryDir = preset.binaryDir;

if (preset.__file && preset.__file.version >= 3) {
// For presets v3+ binaryDir is optional, but cmake-tools needs a value. Default to something reasonable.
if (!preset.binaryDir) {
const defaultValue = '${sourceDir}/out/build/${presetName}';

log.debug(localize('binaryDir.undefined', 'Configure preset {0}: No binaryDir specified, using default value {1}', preset.name, `"${defaultValue}"`));
preset.binaryDir = defaultValue;
// Modify the expandedPreset binary dir so that we don't modify the cache in place.
expandedPreset.binaryDir = defaultValue;
}
}

// Expand other fields
if (preset.binaryDir) {
expandedPreset.binaryDir = util.lightNormalizePath(await expandString(preset.binaryDir, expansionOpts, errorHandler));
if (expandedPreset.binaryDir) {
expandedPreset.binaryDir = util.lightNormalizePath(await expandString(expandedPreset.binaryDir, expansionOpts, errorHandler));
if (!path.isAbsolute(expandedPreset.binaryDir)) {
expandedPreset.binaryDir = util.resolvePath(expandedPreset.binaryDir, sourceDir);
}
Expand Down Expand Up @@ -2049,6 +2083,8 @@ export async function expandPackagePresetVariables(preset: PackagePreset, name:
}
}

expansionOpts.envOverride = EnvironmentUtils.mergePreserveNull([env, expandedPreset.environment]);

if (preset.condition) {
expandedPreset.condition = await expandCondition(preset.condition, expansionOpts, errorHandler);
}
Expand Down
53 changes: 27 additions & 26 deletions src/presetsController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,31 +139,37 @@ export class PresetsController implements vscode.Disposable {
}

private readonly _setExpandedPresets = (folder: string, presetsFile: preset.PresetsFile | undefined) => {
preset.setExpandedPresets(folder, presetsFile);
this._presetsChangedEmitter.fire(presetsFile);
const clone = lodash.cloneDeep(presetsFile);
preset.setExpandedPresets(folder, clone);
this._presetsChangedEmitter.fire(clone);
};

private readonly _setExpandedUserPresetsFile = (folder: string, presetsFile: preset.PresetsFile | undefined) => {
preset.setExpandedUserPresetsFile(folder, presetsFile);
this._userPresetsChangedEmitter.fire(presetsFile);
const clone = lodash.cloneDeep(presetsFile);
preset.setExpandedUserPresetsFile(folder, clone);
this._userPresetsChangedEmitter.fire(clone);
};

private readonly _setPresetsPlusIncluded = (folder: string, presetsFile: preset.PresetsFile | undefined) => {
preset.setPresetsPlusIncluded(folder, presetsFile);
this._presetsChangedEmitter.fire(presetsFile);
const clone = lodash.cloneDeep(presetsFile);
preset.setPresetsPlusIncluded(folder, clone);
this._presetsChangedEmitter.fire(clone);
};

private readonly _setUserPresetsPlusIncluded = (folder: string, presetsFile: preset.PresetsFile | undefined) => {
preset.setUserPresetsPlusIncluded(folder, presetsFile);
this._userPresetsChangedEmitter.fire(presetsFile);
const clone = lodash.cloneDeep(presetsFile);
preset.setUserPresetsPlusIncluded(folder, clone);
this._userPresetsChangedEmitter.fire(clone);
};

private readonly _setOriginalPresetsFile = (folder: string, presetsFile: preset.PresetsFile | undefined) => {
preset.setOriginalPresetsFile(folder, presetsFile);
const clone = lodash.cloneDeep(presetsFile);
preset.setOriginalPresetsFile(folder, clone);
};

private readonly _setOriginalUserPresetsFile = (folder: string, presetsFile: preset.PresetsFile | undefined) => {
preset.setOriginalUserPresetsFile(folder, presetsFile);
const clone = lodash.cloneDeep(presetsFile);
preset.setOriginalUserPresetsFile(folder, clone);
};

private async resetPresetsFile(file: string, setExpandedPresets: SetPresetsFileFunc, setPresetsPlusIncluded: SetPresetsFileFunc, setOriginalPresetsFile: SetPresetsFileFunc, fileExistCallback: (fileExists: boolean) => void, referencedFiles: Map<string, preset.PresetsFile | undefined>) {
Expand All @@ -176,8 +182,7 @@ export class PresetsController implements vscode.Disposable {
let presetsFile = await this.parsePresetsFile(presetsFileBuffer, file);
referencedFiles.set(file, presetsFile);
if (presetsFile) {
// Parse again so we automatically have a copy by value
setOriginalPresetsFile(this.folderPath, await this.parsePresetsFile(presetsFileBuffer, file));
setOriginalPresetsFile(this.folderPath, presetsFile);
} else {
setOriginalPresetsFile(this.folderPath, undefined);
}
Expand All @@ -188,14 +193,12 @@ export class PresetsController implements vscode.Disposable {
this.populatePrivatePresetsFields(presetsFile, file);
await this.mergeIncludeFiles(presetsFile, file, referencedFiles);

const copyOfPresetsFile = lodash.cloneDeep(presetsFile);
// add the include files to the original presets file
setPresetsPlusIncluded(this.folderPath, copyOfPresetsFile);
setPresetsPlusIncluded(this.folderPath, presetsFile);

const copyAgain = lodash.cloneDeep(copyOfPresetsFile);
// set the pre-expanded version so we can call expandPresetsFile on it
setExpandedPresets(this.folderPath, copyAgain);
presetsFile = await this.expandPresetsFile(copyAgain);
setExpandedPresets(this.folderPath, presetsFile);
presetsFile = await this.expandPresetsFile(presetsFile);
}

setExpandedPresets(this.folderPath, presetsFile);
Expand Down Expand Up @@ -1718,14 +1721,12 @@ export class PresetsController implements vscode.Disposable {
return undefined;
}

const clonedPresetsFile = lodash.cloneDeep(presetsFile);

log.info(localize('expanding.presets.file', 'Expanding presets file {0}', presetsFile?.__path || ''));

const expansionErrors: ExpansionErrorHandler = { errorList: [], tempErrorList: []};

const expandedConfigurePresets: preset.ConfigurePreset[] = [];
for (const configurePreset of clonedPresetsFile?.configurePresets || []) {
for (const configurePreset of presetsFile?.configurePresets || []) {
const inheritedPreset = await preset.getConfigurePresetInherits(
this.folderPath,
configurePreset.name,
Expand All @@ -1747,7 +1748,7 @@ export class PresetsController implements vscode.Disposable {
}

const expandedBuildPresets: preset.BuildPreset[] = [];
for (const buildPreset of clonedPresetsFile?.buildPresets || []) {
for (const buildPreset of presetsFile?.buildPresets || []) {
const inheritedPreset = await preset.getBuildPresetInherits(
this.folderPath,
buildPreset.name,
Expand All @@ -1771,7 +1772,7 @@ export class PresetsController implements vscode.Disposable {
}

const expandedTestPresets: preset.TestPreset[] = [];
for (const testPreset of clonedPresetsFile?.testPresets || []) {
for (const testPreset of presetsFile?.testPresets || []) {
const inheritedPreset = await preset.getTestPresetInherits(
this.folderPath,
testPreset.name,
Expand All @@ -1795,7 +1796,7 @@ export class PresetsController implements vscode.Disposable {
}

const expandedPackagePresets: preset.PackagePreset[] = [];
for (const packagePreset of clonedPresetsFile?.packagePresets || []) {
for (const packagePreset of presetsFile?.packagePresets || []) {
const inheritedPreset = await preset.getPackagePresetInherits(
this.folderPath,
packagePreset.name,
Expand All @@ -1819,7 +1820,7 @@ export class PresetsController implements vscode.Disposable {
}

const expandedWorkflowPresets: preset.WorkflowPreset[] = [];
for (const workflowPreset of clonedPresetsFile?.workflowPresets || []) {
for (const workflowPreset of presetsFile?.workflowPresets || []) {
const inheritedPreset = await preset.getWorkflowPresetInherits(
this.folderPath,
workflowPreset.name,
Expand All @@ -1837,7 +1838,7 @@ export class PresetsController implements vscode.Disposable {

if (expansionErrors.errorList.length > 0) {
log.error(localize('expansion.errors', 'Expansion errors found in the presets file.'));
await this.reportPresetsFileErrors(clonedPresetsFile.__path, expansionErrors);
await this.reportPresetsFileErrors(presetsFile.__path, expansionErrors);
return undefined;
} else {
log.info(localize('successfully.expanded.presets.file', 'Successfully expanded presets file {0}', presetsFile?.__path || ''));
Expand All @@ -1851,7 +1852,7 @@ export class PresetsController implements vscode.Disposable {
presetsFile.workflowPresets = expandedWorkflowPresets;

// clear out the errors since there are none now
collections.presets.set(vscode.Uri.file(clonedPresetsFile.__path || ""), undefined);
collections.presets.set(vscode.Uri.file(presetsFile.__path || ""), undefined);

return presetsFile;
}
Expand Down

0 comments on commit f35be38

Please sign in to comment.