Skip to content

Commit 3b33f35

Browse files
Copilotsnehara99
andauthored
Fix build preset jobs handling and add cmake.parallelJobs fallback (#4731)
* Initial plan * Fix CMake build preset jobs flag and parallelJobs setting inconsistency - Fix buildArgs() to handle jobs: 0 (0 is falsy in JS but valid for cmake --parallel 0) - Add fallbackJobs parameter to buildArgs() for cmake.parallelJobs setting fallback - Pass config.numJobs as fallback in generateBuildCommandFromPreset() - Fix same jobs: 0 issue in testArgs() for consistency - Add unit tests for buildArgs() covering all edge cases Co-authored-by: snehara99 <[email protected]> * Use raw parallelJobs for preset fallback to preserve jobs: 0 Add numJobsForPreset getter that preserves 0 as-is (since CMake natively handles --parallel 0) instead of converting to CPU count. Use it in generateBuildCommandFromPreset() as the fallback. Add test for fallbackJobs: 0 case. Co-authored-by: snehara99 <[email protected]> * Move cmake.parallelJobs to override settings table in docs Since our PR uses cmake.parallelJobs as a fallback when the build preset doesn't specify jobs, the documentation should reflect that it's no longer completely ignored when presets are active. Co-authored-by: snehara99 <[email protected]> * Use -j instead of --parallel in preset buildArgs Per reviewer preference, use the shorter -j flag instead of --parallel when passing the jobs count to cmake --build. Both flags are equivalent in CMake (cmake --build -j N == cmake --build --parallel N). Co-authored-by: snehara99 <[email protected]> * Use -j 0 for jobs==0 and --parallel N for jobs>0 in buildArgs Per reviewer request: use the shorter -j flag only when the value is 0, and use --parallel with the explicit count for positive values. Co-authored-by: snehara99 <[email protected]> * Move cmake.parallelJobs back to ignored settings table with fallback note The setting doesn't override buildPresets.jobs - it's only used as a fallback when jobs is not defined in the preset. Update the description to clarify this behavior. Co-authored-by: snehara99 <[email protected]> * Use cmake.parallelJobs fallback when preset jobs is 0 or undefined When buildPresets.jobs is 0, treat it like "not defined" and fall back to cmake.parallelJobs. If cmake.parallelJobs is also not set, the preset's own 0 value is preserved (producing -j 0). Updated docs and tests to match. Co-authored-by: snehara99 <[email protected]> * Revert last 2 commits per reviewer request Reverts the docs change (moving cmake.parallelJobs back to ignored table) and the code change (using fallback when preset jobs is 0). Restores the state from commit e907de6. Co-authored-by: snehara99 <[email protected]> * Revert docs to original state: cmake.parallelJobs in ignored table Move cmake.parallelJobs back to the "Ignored settings" table where it was originally, removing it from the "Override settings" table. Co-authored-by: snehara99 <[email protected]> * Changes before error encountered Co-authored-by: snehara99 <[email protected]> * Changes before error encountered Co-authored-by: snehara99 <[email protected]> * Fix tests to expect bare -j (no value) when jobs is 0 The code already pushes bare '-j' for jobs===0, but the tests still expected '-j' followed by '0'. Updated tests to verify bare -j is emitted without a numeric argument. Co-authored-by: snehara99 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: snehara99 <[email protected]>
1 parent 074387b commit 3b33f35

5 files changed

Lines changed: 99 additions & 5 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ Bug Fixes:
3535
- Add support for Visual Studio 2026 generator. [#4637](https://github.com/microsoft/vscode-cmake-tools/issues/4637)
3636
- Fix `$comment` not being accepted inside a cacheVariable object in CMake presets. [#4600](https://github.com/microsoft/vscode-cmake-tools/issues/4600)
3737
- Fix kits from `cmake.additionalKits` not being shown when `cmake.showSystemKits` is `false`. [#4651](https://github.com/microsoft/vscode-cmake-tools/issues/4651)
38+
- Fix how `jobs` is handled in build presets. Also update how `cmake.parallelJobs` is handled as a fallback when a build preset does not define `jobs`. [#4176](https://github.com/microsoft/vscode-cmake-tools/issues/4176)
3839

3940
## 1.22.28
4041

src/config.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -522,6 +522,13 @@ export class ConfigurationReader implements vscode.Disposable {
522522
}
523523
}
524524

525+
get numJobsForPreset(): number | undefined {
526+
if (this.isDefaultValue("parallelJobs")) {
527+
return undefined;
528+
}
529+
return this.parallelJobs;
530+
}
531+
525532
get numCTestJobs(): number {
526533
const ctestJobs = this.ctestParallelJobs;
527534
if (!ctestJobs) {

src/drivers/cmakeDriver.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1952,7 +1952,7 @@ export abstract class CMakeDriver implements vscode.Disposable {
19521952
} else {
19531953
buildPreset.__targets = buildPreset.targets;
19541954
}
1955-
const args = preset.buildArgs(buildPreset, this.config.buildArgs, this.config.buildToolArgs);
1955+
const args = preset.buildArgs(buildPreset, this.config.buildArgs, this.config.buildToolArgs, this.config.numJobsForPreset);
19561956
const initialEnvironment = EnvironmentUtils.create(buildPreset.environment);
19571957
const build_env = await this.getCMakeBuildCommandEnvironment(initialEnvironment);
19581958
const expanded_args_promises = args.map(async (value: string) => expand.expandString(value, { ...this.expansionOptions, envOverride: build_env }));

src/presets/preset.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2210,11 +2210,18 @@ export function configureArgs(preset: ConfigurePreset): string[] {
22102210
return result;
22112211
}
22122212

2213-
export function buildArgs(preset: BuildPreset, tempOverrideArgs?: string[], tempOverrideBuildToolArgs?: string[]): string[] {
2213+
export function buildArgs(preset: BuildPreset, tempOverrideArgs?: string[], tempOverrideBuildToolArgs?: string[], fallbackJobs?: number): string[] {
22142214
const result: string[] = [];
22152215

22162216
preset.__binaryDir && result.push('--build', preset.__binaryDir);
2217-
preset.jobs && result.push('--parallel', preset.jobs.toString());
2217+
const jobs = preset.jobs ?? fallbackJobs;
2218+
if (jobs !== undefined) {
2219+
if (jobs === 0) {
2220+
result.push('-j');
2221+
} else {
2222+
result.push('--parallel', jobs.toString());
2223+
}
2224+
}
22182225
preset.configuration && result.push('--config', preset.configuration);
22192226
preset.cleanFirst && result.push('--clean-first');
22202227
preset.verbose && result.push('--verbose');
@@ -2290,7 +2297,7 @@ export function testArgs(preset: TestPreset): string[] {
22902297
if (preset.execution) {
22912298
preset.execution.stopOnFailure && result.push('--stop-on-failure');
22922299
preset.execution.enableFailover && result.push('-F');
2293-
preset.execution.jobs && result.push('--parallel', preset.execution.jobs.toString());
2300+
(preset.execution.jobs !== undefined) && result.push('--parallel', preset.execution.jobs.toString());
22942301
preset.execution.resourceSpecFile && result.push('--resource-spec-file', preset.execution.resourceSpecFile);
22952302
preset.execution.testLoad && result.push('--test-load', preset.execution.testLoad.toString());
22962303
preset.execution.showOnly && result.push('--show-only', preset.execution.showOnly);

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

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

@@ -195,4 +195,83 @@ suite('Preset tests', () => {
195195
const args5 = configureArgs(preset5);
196196
expect(args5).to.deep.eq([]);
197197
});
198+
199+
test('buildArgs handles jobs: 0 correctly', () => {
200+
const preset: any = {
201+
name: 'test',
202+
__binaryDir: '/path/to/build',
203+
jobs: 0
204+
};
205+
const args = buildArgs(preset);
206+
expect(args).to.include('-j');
207+
// -j should be bare (no value) — next arg should NOT be '0'
208+
const idx = args.indexOf('-j');
209+
expect(args[idx + 1]).to.not.eq('0');
210+
});
211+
212+
test('buildArgs handles jobs: positive number', () => {
213+
const preset: any = {
214+
name: 'test',
215+
__binaryDir: '/path/to/build',
216+
jobs: 8
217+
};
218+
const args = buildArgs(preset);
219+
const idx = args.indexOf('--parallel');
220+
expect(idx).to.be.greaterThan(-1);
221+
expect(args[idx + 1]).to.eq('8');
222+
});
223+
224+
test('buildArgs omits -j and --parallel when jobs is undefined and no fallback', () => {
225+
const preset: any = {
226+
name: 'test',
227+
__binaryDir: '/path/to/build'
228+
};
229+
const args = buildArgs(preset);
230+
expect(args).to.not.include('-j');
231+
expect(args).to.not.include('--parallel');
232+
});
233+
234+
test('buildArgs uses fallbackJobs when jobs is undefined', () => {
235+
const preset: any = {
236+
name: 'test',
237+
__binaryDir: '/path/to/build'
238+
};
239+
const args = buildArgs(preset, undefined, undefined, 4);
240+
const idx = args.indexOf('--parallel');
241+
expect(idx).to.be.greaterThan(-1);
242+
expect(args[idx + 1]).to.eq('4');
243+
});
244+
245+
test('buildArgs prefers preset jobs over fallbackJobs', () => {
246+
const preset: any = {
247+
name: 'test',
248+
__binaryDir: '/path/to/build',
249+
jobs: 2
250+
};
251+
const args = buildArgs(preset, undefined, undefined, 8);
252+
const idx = args.indexOf('--parallel');
253+
expect(idx).to.be.greaterThan(-1);
254+
expect(args[idx + 1]).to.eq('2');
255+
});
256+
257+
test('buildArgs preset jobs: 0 takes precedence over fallbackJobs', () => {
258+
const preset: any = {
259+
name: 'test',
260+
__binaryDir: '/path/to/build',
261+
jobs: 0
262+
};
263+
const args = buildArgs(preset, undefined, undefined, 8);
264+
expect(args).to.include('-j');
265+
expect(args).to.not.include('--parallel');
266+
});
267+
268+
test('buildArgs uses fallbackJobs: 0 to pass bare -j', () => {
269+
const preset: any = {
270+
name: 'test',
271+
__binaryDir: '/path/to/build'
272+
};
273+
const args = buildArgs(preset, undefined, undefined, 0);
274+
expect(args).to.include('-j');
275+
expect(args).to.not.include('--parallel');
276+
});
198277
});

0 commit comments

Comments
 (0)