Skip to content

Commit ca28e03

Browse files
Copilotsnehara99
Copilot
and
snehara99
authored
Fix $comment not being accepted inside cacheVariable object in presets (#4692)
* Initial plan * Fix $comment not being accepted inside cacheVariable object in presets Co-authored-by: snehara99 <[email protected]> * Improve test coverage for $comment inside cacheVariable object Co-authored-by: snehara99 <[email protected]> * Fix issue number in CHANGELOG from #4709 to #4600 Co-authored-by: snehara99 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: snehara99 <[email protected]>
1 parent 0802573 commit ca28e03

4 files changed

Lines changed: 99 additions & 1 deletion

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ Bug Fixes:
1818
- Fix `cmake.compileFile` failing to find compilation info when using presets without `CMAKE_EXPORT_COMPILE_COMMANDS`. [#4484](https://github.com/microsoft/vscode-cmake-tools/issues/4484)
1919
- Fix "Copy Value" in CMake debugger copying variable name instead of value. [#4551](https://github.com/microsoft/vscode-cmake-tools/issues/4551)
2020
- cmakeDriver: Fixes getCompilerVersion by using compilerPath instead of compilerName. [#4647](https://github.com/microsoft/vscode-cmake-tools/pull/4647) [@lygstate](https://github.com/lygstate)
21+
- Fix `$comment` not being accepted inside a cacheVariable object in CMake presets. [#4600](https://github.com/microsoft/vscode-cmake-tools/issues/4600)
2122
- Fix kits from `cmake.additionalKits` not being shown when `cmake.showSystemKits` is `false`. [#4651](https://github.com/microsoft/vscode-cmake-tools/issues/4651)
2223

2324
## 1.22.27

‎schemas/CMakePresets-v10-schema.json‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,9 @@
385385
"type": "object",
386386
"description": "An optional map of cache variables. The key is the variable name (which must not be an empty string). Cache variables are inherited through the inherits field, and the preset's variables will be the union of its own cacheVariables and the cacheVariables from all its parents. If multiple presets in this union define the same variable, the standard rules of inherits are applied.",
387387
"properties": {
388+
"$comment": {
389+
"$ref": "#/definitions/$comment"
390+
},
388391
"CMAKE_C_COMPILER": {
389392
"anyOf": [
390393
{
@@ -398,6 +401,9 @@
398401
{
399402
"type": "object",
400403
"properties": {
404+
"$comment": {
405+
"$ref": "#/definitions/$comment"
406+
},
401407
"type": {
402408
"type": "string"
403409
},
@@ -422,6 +428,9 @@
422428
{
423429
"type": "object",
424430
"properties": {
431+
"$comment": {
432+
"$ref": "#/definitions/$comment"
433+
},
425434
"type": {
426435
"type": "string"
427436
},
@@ -446,6 +455,9 @@
446455
{
447456
"type": "object",
448457
"properties": {
458+
"$comment": {
459+
"$ref": "#/definitions/$comment"
460+
},
449461
"type": {
450462
"type": "string"
451463
},
@@ -470,6 +482,9 @@
470482
{
471483
"type": "object",
472484
"properties": {
485+
"$comment": {
486+
"$ref": "#/definitions/$comment"
487+
},
473488
"type": {
474489
"type": "string"
475490
},
@@ -496,6 +511,9 @@
496511
{
497512
"type": "object",
498513
"properties": {
514+
"$comment": {
515+
"$ref": "#/definitions/$comment"
516+
},
499517
"type": {
500518
"type": "string"
501519
},
@@ -533,6 +551,9 @@
533551
"type": "object",
534552
"description": "An object representing the type and value of the variable.",
535553
"properties": {
554+
"$comment": {
555+
"$ref": "#/definitions/$comment"
556+
},
536557
"type": {
537558
"type": "string",
538559
"description": "An optional string representing the type of the variable. It should be BOOL, FILEPATH, PATH, STRING, or INTERNAL."

‎src/presets/preset.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1244,6 +1244,10 @@ export async function expandConfigurePresetVariables(preset: ConfigurePreset, fo
12441244
if (preset.cacheVariables) {
12451245
expandedPreset.cacheVariables = {};
12461246
for (const cacheVarName in preset.cacheVariables) {
1247+
// Skip $comment keys as they are only for documentation purposes
1248+
if (cacheVarName === '$comment') {
1249+
continue;
1250+
}
12471251
const cacheVar = preset.cacheVariables[cacheVarName];
12481252
if (typeof cacheVar === 'boolean') {
12491253
expandedPreset.cacheVariables[cacheVarName] = cacheVar;
@@ -2138,6 +2142,10 @@ export function configureArgs(preset: ConfigurePreset): string[] {
21382142
// CacheVariables
21392143
if (preset.cacheVariables) {
21402144
util.objectPairs(preset.cacheVariables).forEach(([key, value]) => {
2145+
// Skip $comment keys as they are only for documentation purposes
2146+
if (key === '$comment') {
2147+
return;
2148+
}
21412149
if (util.isString(value) || typeof value === 'boolean') {
21422150
result.push(`-D${key}=${value}`);
21432151
} else if (value) {

‎test/unit-tests/presets/presets.test.ts‎

Lines changed: 69 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Condition, evaluateCondition, getArchitecture, getToolset } from '@cmt/presets/preset';
1+
import { Condition, configureArgs, evaluateCondition, getArchitecture, getToolset } from '@cmt/presets/preset';
22
import { expect } from '@test/util';
33
import * as os from "os";
44

@@ -127,4 +127,72 @@ suite('Preset tests', () => {
127127
badCondition = { type: 'equals', lhs: 'lhs' };
128128
expect(() => evaluateCondition(badCondition)).to.throw();
129129
});
130+
131+
test('configureArgs skips $comment keys in cacheVariables', () => {
132+
// Test that $comment at the top level of cacheVariables is skipped
133+
const preset1: any = {
134+
name: 'test',
135+
cacheVariables: {
136+
'$comment': 'This is a comment',
137+
'CMAKE_BUILD_TYPE': 'Debug'
138+
}
139+
};
140+
const args1 = configureArgs(preset1);
141+
expect(args1).to.deep.eq(['-DCMAKE_BUILD_TYPE=Debug']);
142+
expect(args1.some(arg => arg.includes('$comment'))).to.eq(false);
143+
144+
// Test that $comment as an array (multi-line) is also skipped
145+
const preset2: any = {
146+
name: 'test',
147+
cacheVariables: {
148+
'$comment': ['Line 1', 'Line 2'],
149+
'MY_VAR': 'value'
150+
}
151+
};
152+
const args2 = configureArgs(preset2);
153+
expect(args2).to.deep.eq(['-DMY_VAR=value']);
154+
155+
// Test with object-style cache variable with $comment inside the object
156+
// This is the main use case from issue #4709 - $comment inside cacheVariable object
157+
const preset3: any = {
158+
name: 'test',
159+
cacheVariables: {
160+
'CMAKE_EXE_LINKER_FLAGS': {
161+
type: 'STRING',
162+
'$comment': 'Suppress warning about free-nonheap-object',
163+
value: '-Wno-error=free-nonheap-object'
164+
}
165+
}
166+
};
167+
const args3 = configureArgs(preset3);
168+
expect(args3).to.deep.eq(['-DCMAKE_EXE_LINKER_FLAGS:STRING=-Wno-error=free-nonheap-object']);
169+
// Verify $comment inside the object doesn't affect the output
170+
expect(args3.some(arg => arg.includes('$comment'))).to.eq(false);
171+
172+
// Test with $comment both at top level and inside object
173+
const preset4: any = {
174+
name: 'test',
175+
cacheVariables: {
176+
'$comment': 'Top-level comment',
177+
'CMAKE_BUILD_TYPE': {
178+
type: 'STRING',
179+
'$comment': 'Build type comment',
180+
value: 'Release'
181+
}
182+
}
183+
};
184+
const args4 = configureArgs(preset4);
185+
expect(args4).to.deep.eq(['-DCMAKE_BUILD_TYPE:STRING=Release']);
186+
expect(args4.some(arg => arg.includes('$comment'))).to.eq(false);
187+
188+
// Test empty cacheVariables (should produce no args)
189+
const preset5: any = {
190+
name: 'test',
191+
cacheVariables: {
192+
'$comment': 'Only comment, no vars'
193+
}
194+
};
195+
const args5 = configureArgs(preset5);
196+
expect(args5).to.deep.eq([]);
197+
});
130198
});

0 commit comments

Comments
 (0)