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

Author: gcampbell-msft

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

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

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<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;
     }
 
@@ -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<ConfigurePreset> {
 
     // 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);
     }

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<string, preset.PresetsFile | undefined>) {
@@ -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;
         }