Skip to content

Commit d05a382

Browse files
authored
Fix exportCompileCommands flag handling (#4899)
* fix: prevent passing -DCMAKE_EXPORT_COMPILE_COMMANDS flag when exportCompileCommandsFile is false * fix changelog --------- Co-authored-by: Hannia Valera <hanniavalera@users.noreply.github.com>
1 parent 1ef1fd4 commit d05a382

3 files changed

Lines changed: 128 additions & 2 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ Bug Fixes:
1313
## 1.23.52
1414

1515
Bug Fixes:
16+
- Fix `cmake.exportCompileCommandsFile` set to `false` still passing `-DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=FALSE` instead of omitting the flag entirely, which caused CMake warnings for projects with `LANGUAGES NONE`. [#4893](https://github.com/microsoft/vscode-cmake-tools/issues/4893)
1617
- Fix regression where Visual Studio kits with an existing Ninja-based build cache would fail due to a generator mismatch. Ninja is now preferred again when available, stale VS kits derive the correct generator at runtime as a fallback, and the build directory is auto-cleaned on generator mismatches. [#4890](https://github.com/microsoft/vscode-cmake-tools/issues/4890)
1718

1819
## 1.23

src/drivers/cmakeDriver.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1574,7 +1574,7 @@ export abstract class CMakeDriver implements vscode.Disposable {
15741574
const presetCacheVariables = configPreset.cacheVariables ?? {};
15751575
const hasExportCompileCommands = Object.prototype.hasOwnProperty.call(presetCacheVariables, 'CMAKE_EXPORT_COMPILE_COMMANDS')
15761576
|| expandedArgs.some(arg => arg.startsWith('-DCMAKE_EXPORT_COMPILE_COMMANDS'));
1577-
if (!hasExportCompileCommands) {
1577+
if (!hasExportCompileCommands && exportCompileCommandsFile) {
15781578
const exportCompileCommandsValue = util.cmakeify(exportCompileCommandsFile);
15791579
expandedArgs.push(`-DCMAKE_EXPORT_COMPILE_COMMANDS:${exportCompileCommandsValue.type}=${exportCompileCommandsValue.value}`);
15801580
}
@@ -1898,7 +1898,9 @@ export abstract class CMakeDriver implements vscode.Disposable {
18981898
// Export compile_commands.json
18991899
const exportCompileCommandsSetting = config.get<boolean>("exportCompileCommandsFile");
19001900
const exportCompileCommandsFile: boolean = exportCompileCommandsSetting === undefined ? true : (exportCompileCommandsSetting || false);
1901-
settingMap.CMAKE_EXPORT_COMPILE_COMMANDS = util.cmakeify(exportCompileCommandsFile);
1901+
if (exportCompileCommandsFile) {
1902+
settingMap.CMAKE_EXPORT_COMPILE_COMMANDS = util.cmakeify(exportCompileCommandsFile);
1903+
}
19021904

19031905
console.assert(!!this._kit);
19041906
if (!this._kit) {
Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
import { expect } from 'chai';
2+
3+
/**
4+
* Tests for the export compile commands logic in cmakeDriver.ts
5+
*
6+
* This tests the LOGIC PATTERN used for deciding whether to inject
7+
* -DCMAKE_EXPORT_COMPILE_COMMANDS into CMake configure arguments.
8+
*
9+
* The actual implementation lives in cmakeDriver.ts but depends on vscode,
10+
* so we mirror the decision logic here to validate correctness.
11+
*
12+
* Issue #4893: When cmake.exportCompileCommandsFile is set to false,
13+
* the flag should NOT be passed at all (not even as FALSE), to avoid
14+
* CMake warnings for projects with LANGUAGES NONE.
15+
*/
16+
17+
suite('[Export Compile Commands Logic]', () => {
18+
/**
19+
* Mirrors the logic from cmakeDriver.ts generateConfigArgsFromPreset()
20+
* and generateCMakeSettingsFlags()
21+
*
22+
* @param exportCompileCommandsSetting The raw setting value (undefined, true, or false)
23+
* @param hasExportCompileCommandsInPresetOrArgs Whether preset or args already specify the flag
24+
* @returns Whether to inject the -DCMAKE_EXPORT_COMPILE_COMMANDS flag
25+
*/
26+
function shouldInjectExportCompileCommandsFlag(
27+
exportCompileCommandsSetting: boolean | undefined,
28+
hasExportCompileCommandsInPresetOrArgs: boolean
29+
): boolean {
30+
// Mirror the logic from cmakeDriver.ts:
31+
// const exportCompileCommandsFile: boolean = exportCompileCommandsSetting === undefined ? true : (exportCompileCommandsSetting || false);
32+
const exportCompileCommandsFile: boolean = exportCompileCommandsSetting === undefined ? true : (exportCompileCommandsSetting || false);
33+
34+
// For presets mode (generateConfigArgsFromPreset):
35+
// if (!hasExportCompileCommands && exportCompileCommandsFile)
36+
//
37+
// For kits mode (generateCMakeSettingsFlags):
38+
// if (exportCompileCommandsFile)
39+
//
40+
// Both modes now check exportCompileCommandsFile is truthy before injecting
41+
return !hasExportCompileCommandsInPresetOrArgs && exportCompileCommandsFile;
42+
}
43+
44+
suite('Setting is undefined (default)', () => {
45+
test('Should inject flag when preset/args do not specify it', () => {
46+
const result = shouldInjectExportCompileCommandsFlag(undefined, false);
47+
expect(result).to.equal(true, 'Default behavior should inject TRUE to enable compile_commands.json');
48+
});
49+
50+
test('Should NOT inject flag when preset/args already specify it', () => {
51+
const result = shouldInjectExportCompileCommandsFlag(undefined, true);
52+
expect(result).to.equal(false, 'Should respect preset/args override');
53+
});
54+
});
55+
56+
suite('Setting is explicitly true', () => {
57+
test('Should inject flag when preset/args do not specify it', () => {
58+
const result = shouldInjectExportCompileCommandsFlag(true, false);
59+
expect(result).to.equal(true, 'Explicit true should inject the flag');
60+
});
61+
62+
test('Should NOT inject flag when preset/args already specify it', () => {
63+
const result = shouldInjectExportCompileCommandsFlag(true, true);
64+
expect(result).to.equal(false, 'Should respect preset/args override');
65+
});
66+
});
67+
68+
suite('Setting is explicitly false (Issue #4893 fix)', () => {
69+
test('Should NOT inject flag when preset/args do not specify it', () => {
70+
const result = shouldInjectExportCompileCommandsFlag(false, false);
71+
expect(result).to.equal(false, 'Explicit false should NOT inject the flag at all');
72+
});
73+
74+
test('Should NOT inject flag when preset/args already specify it', () => {
75+
const result = shouldInjectExportCompileCommandsFlag(false, true);
76+
expect(result).to.equal(false, 'Should respect preset/args override');
77+
});
78+
});
79+
80+
suite('exportCompileCommandsFile computation', () => {
81+
/**
82+
* Mirrors the computation: exportCompileCommandsSetting === undefined ? true : (exportCompileCommandsSetting || false)
83+
*/
84+
function computeExportCompileCommandsFile(setting: boolean | undefined): boolean {
85+
return setting === undefined ? true : (setting || false);
86+
}
87+
88+
test('undefined setting defaults to true', () => {
89+
expect(computeExportCompileCommandsFile(undefined)).to.equal(true);
90+
});
91+
92+
test('explicit true remains true', () => {
93+
expect(computeExportCompileCommandsFile(true)).to.equal(true);
94+
});
95+
96+
test('explicit false becomes false', () => {
97+
expect(computeExportCompileCommandsFile(false)).to.equal(false);
98+
});
99+
});
100+
101+
suite('Kits mode logic (generateCMakeSettingsFlags)', () => {
102+
/**
103+
* In kits mode, the logic is simpler: just check if exportCompileCommandsFile is truthy
104+
* (no hasExportCompileCommands check needed as configureSettings handles that)
105+
*/
106+
function shouldAddToSettingMapInKitsMode(exportCompileCommandsSetting: boolean | undefined): boolean {
107+
const exportCompileCommandsFile: boolean = exportCompileCommandsSetting === undefined ? true : (exportCompileCommandsSetting || false);
108+
return exportCompileCommandsFile;
109+
}
110+
111+
test('undefined setting should add to settingMap', () => {
112+
expect(shouldAddToSettingMapInKitsMode(undefined)).to.equal(true);
113+
});
114+
115+
test('explicit true should add to settingMap', () => {
116+
expect(shouldAddToSettingMapInKitsMode(true)).to.equal(true);
117+
});
118+
119+
test('explicit false should NOT add to settingMap (Issue #4893 fix)', () => {
120+
expect(shouldAddToSettingMapInKitsMode(false)).to.equal(false);
121+
});
122+
});
123+
});

0 commit comments

Comments
 (0)