From f35be385cbd61a74ee4ec317730c132f4b363c05 Mon Sep 17 00:00:00 2001 From: Garrett Campbell <86264750+gcampbell-msft@users.noreply.github.com> Date: Fri, 30 Aug 2024 14:42:56 -0400 Subject: [PATCH] Correctly store cache and update `binaryDir` (#4028) * 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 --- CHANGELOG.md | 1 + src/cmakeProject.ts | 8 ++-- src/preset.ts | 80 +++++++++++++++++++++++++++++----------- src/presetsController.ts | 53 +++++++++++++------------- 4 files changed, 90 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf2c1470b..a95c93409 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/src/cmakeProject.ts b/src/cmakeProject.ts index b953987fd..1779acb20 100644 --- a/src/cmakeProject.ts +++ b/src/cmakeProject.ts @@ -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]); @@ -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 @@ -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 @@ -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 diff --git a/src/preset.ts b/src/preset.ts index 3d689398a..f21c86f7a 100644 --- a/src/preset.ts +++ b/src/preset.ts @@ -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)]; } } @@ -1056,6 +1084,7 @@ async function getVsDevEnv(opts: VsDevEnvOptions): Promise { const useVsDeveloperEnvironmentMode = vscode.workspace.getConfiguration("cmake", vscode.Uri.file(workspaceFolder)).get("useVsDeveloperEnvironment") as UseVsDeveloperEnvironment; if (useVsDeveloperEnvironmentMode === "never") { + preset.__parentEnvironment = process.env; return; } @@ -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; @@ -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 { // Put the preset.environment on top of combined environment in the `__parentEnvironment` field. @@ -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); } @@ -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); } diff --git a/src/presetsController.ts b/src/presetsController.ts index eb83512de..c35c6adca 100644 --- a/src/presetsController.ts +++ b/src/presetsController.ts @@ -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) { @@ -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); } @@ -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); @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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 || '')); @@ -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; }