From 8cbc06692dc7bda435b9ad439101fbf5dfa1beba Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Thu, 24 Apr 2025 14:04:00 -0700 Subject: [PATCH 01/14] make the compilerPath validation common across JSON and UI --- .../src/LanguageServer/configurations.ts | 180 ++++++-------- .../LanguageServer/cppBuildTaskProvider.ts | 2 +- Extension/src/common.ts | 68 +++--- .../SingleRootProject/assets/b i n/clang++ | 0 .../assets/b i n/clang++.exe | 0 .../SingleRootProject/assets/bin/cl.exe | 0 .../SingleRootProject/assets/bin/clang-cl.exe | 0 .../SingleRootProject/assets/bin/gcc | 0 .../SingleRootProject/assets/bin/gcc.exe | 0 .../tests/compilerPath.test.ts | 223 ++++++++++++++++++ 10 files changed, 329 insertions(+), 144 deletions(-) create mode 100644 Extension/test/scenarios/SingleRootProject/assets/b i n/clang++ create mode 100644 Extension/test/scenarios/SingleRootProject/assets/b i n/clang++.exe create mode 100644 Extension/test/scenarios/SingleRootProject/assets/bin/cl.exe create mode 100644 Extension/test/scenarios/SingleRootProject/assets/bin/clang-cl.exe create mode 100644 Extension/test/scenarios/SingleRootProject/assets/bin/gcc create mode 100644 Extension/test/scenarios/SingleRootProject/assets/bin/gcc.exe create mode 100644 Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts diff --git a/Extension/src/LanguageServer/configurations.ts b/Extension/src/LanguageServer/configurations.ts index 6870619b8..c4f1ac158 100644 --- a/Extension/src/LanguageServer/configurations.ts +++ b/Extension/src/LanguageServer/configurations.ts @@ -63,7 +63,7 @@ export interface ConfigurationJson { export interface Configuration { name: string; compilerPathInCppPropertiesJson?: string | null; - compilerPath?: string | null; + compilerPath?: string; compilerPathIsExplicit?: boolean; compilerArgs?: string[]; compilerArgsLegacy?: string[]; @@ -982,14 +982,13 @@ export class CppProperties { } else { // However, if compileCommands are used and compilerPath is explicitly set, it's still necessary to resolve variables in it. if (configuration.compilerPath === "${default}") { - configuration.compilerPath = settings.defaultCompilerPath; - } - if (configuration.compilerPath === null) { + configuration.compilerPath = settings.defaultCompilerPath ?? undefined; configuration.compilerPathIsExplicit = true; - } else if (configuration.compilerPath !== undefined) { + } + if (configuration.compilerPath) { configuration.compilerPath = util.resolveVariables(configuration.compilerPath, env); configuration.compilerPathIsExplicit = true; - } else { + } else if (configuration.compilerPathIsExplicit === undefined) { configuration.compilerPathIsExplicit = false; } } @@ -1596,88 +1595,93 @@ export class CppProperties { return result; } - private getErrorsForConfigUI(configIndex: number): ConfigurationErrors { - const errors: ConfigurationErrors = {}; - if (!this.configurationJson) { - return errors; - } - const isWindows: boolean = os.platform() === 'win32'; - const config: Configuration = this.configurationJson.configurations[configIndex]; - - // Check if config name is unique. - errors.name = this.isConfigNameUnique(config.name); - let resolvedCompilerPath: string | undefined | null; - // Validate compilerPath - if (config.compilerPath) { - resolvedCompilerPath = which.sync(config.compilerPath, { nothrow: true }); - } - + /** + * Get the compilerPath and args from a compilerPath string that has already passed through + * `this.resolvePath`. If there are errors processing the path, those will also be returned. + * + * @resolvedCompilerPath a compilerPath string that has already been resolved. + */ + public static validateCompilerPath(resolvedCompilerPath: string, rootUri?: vscode.Uri): util.CompilerPathAndArgs { if (!resolvedCompilerPath) { - resolvedCompilerPath = this.resolvePath(config.compilerPath); + return { compilerName: '', allCompilerArgs: [], compilerArgsFromCommandLineInPath: [] }; } - const settings: CppSettings = new CppSettings(this.rootUri); - const compilerPathAndArgs: util.CompilerPathAndArgs = util.extractCompilerPathAndArgs(!!settings.legacyCompilerArgsBehavior, resolvedCompilerPath); - if (resolvedCompilerPath - // Don't error cl.exe paths because it could be for an older preview build. - && compilerPathAndArgs.compilerName.toLowerCase() !== "cl.exe" - && compilerPathAndArgs.compilerName.toLowerCase() !== "cl") { - resolvedCompilerPath = resolvedCompilerPath.trim(); - - // Error when the compiler's path has spaces without quotes but args are used. - // Except, exclude cl.exe paths because it could be for an older preview build. - const compilerPathNeedsQuotes: boolean = - (compilerPathAndArgs.compilerArgsFromCommandLineInPath && compilerPathAndArgs.compilerArgsFromCommandLineInPath.length > 0) && - !resolvedCompilerPath.startsWith('"') && - compilerPathAndArgs.compilerPath !== undefined && - compilerPathAndArgs.compilerPath !== null && - compilerPathAndArgs.compilerPath.includes(" "); - - const compilerPathErrors: string[] = []; - if (compilerPathNeedsQuotes) { - compilerPathErrors.push(localize("path.with.spaces", 'Compiler path with spaces and arguments is missing double quotes " around the path.')); - } + resolvedCompilerPath = resolvedCompilerPath.trim(); + + const settings = new CppSettings(rootUri); + const compilerPathAndArgs = util.extractCompilerPathAndArgs(!!settings.legacyCompilerArgsBehavior, resolvedCompilerPath, undefined, rootUri?.fsPath); + const compilerLowerCase: string = compilerPathAndArgs.compilerName.toLowerCase(); + const isCl: boolean = compilerLowerCase === "cl" || compilerLowerCase === "cl.exe"; + const telemetry: { [key: string]: number } = {}; + // Don't error cl.exe paths because it could be for an older preview build. + if (!isCl) { // Get compiler path without arguments before checking if it exists - resolvedCompilerPath = compilerPathAndArgs.compilerPath ?? undefined; - if (resolvedCompilerPath) { + if (compilerPathAndArgs.compilerPath) { + resolvedCompilerPath = compilerPathAndArgs.compilerPath; let pathExists: boolean = true; const existsWithExeAdded: (path: string) => boolean = (path: string) => isWindows && !path.startsWith("/") && fs.existsSync(path + ".exe"); if (!fs.existsSync(resolvedCompilerPath)) { if (existsWithExeAdded(resolvedCompilerPath)) { resolvedCompilerPath += ".exe"; - } else if (!this.rootUri) { - pathExists = false; } else { - // Check again for a relative path. - const relativePath: string = this.rootUri.fsPath + path.sep + resolvedCompilerPath; - if (!fs.existsSync(relativePath)) { - if (existsWithExeAdded(resolvedCompilerPath)) { - resolvedCompilerPath += ".exe"; + const pathLocation = which.sync(resolvedCompilerPath, { nothrow: true }); + if (pathLocation) { + resolvedCompilerPath = pathLocation; + compilerPathAndArgs.compilerPath = pathLocation; + } else if (rootUri) { + // Check again for a relative path. + const relativePath: string = rootUri.fsPath + path.sep + resolvedCompilerPath; + if (!fs.existsSync(relativePath)) { + if (existsWithExeAdded(relativePath)) { + resolvedCompilerPath = relativePath + ".exe"; + } else { + pathExists = false; + } } else { - pathExists = false; + resolvedCompilerPath = relativePath; } - } else { - resolvedCompilerPath = relativePath; } } } + const compilerPathErrors: string[] = []; + if (!pathExists) { const message: string = localize('cannot.find', "Cannot find: {0}", resolvedCompilerPath); compilerPathErrors.push(message); - } else if (compilerPathAndArgs.compilerPath === "") { - const message: string = localize("cannot.resolve.compiler.path", "Invalid input, cannot resolve compiler path"); - compilerPathErrors.push(message); + telemetry.PathNonExistent = 1; } else if (!util.checkExecutableWithoutExtensionExistsSync(resolvedCompilerPath)) { const message: string = localize("path.is.not.a.file", "Path is not a file: {0}", resolvedCompilerPath); compilerPathErrors.push(message); + telemetry.PathNotAFile = 1; } if (compilerPathErrors.length > 0) { - errors.compilerPath = compilerPathErrors.join('\n'); + compilerPathAndArgs.error = compilerPathErrors.join('\n'); } } } + compilerPathAndArgs.telemetry = telemetry; + return compilerPathAndArgs; + } + + private getErrorsForConfigUI(configIndex: number): ConfigurationErrors { + const errors: ConfigurationErrors = {}; + if (!this.configurationJson) { + return errors; + } + const isWindows: boolean = os.platform() === 'win32'; + const config: Configuration = this.configurationJson.configurations[configIndex]; + + // Check if config name is unique. + errors.name = this.isConfigNameUnique(config.name); + let resolvedCompilerPath: string | undefined | null; + // Validate compilerPath + if (!resolvedCompilerPath) { + resolvedCompilerPath = this.resolvePath(config.compilerPath, false, false); + } + const compilerPathAndArgs: util.CompilerPathAndArgs = CppProperties.validateCompilerPath(resolvedCompilerPath, this.rootUri); + errors.compilerPath = compilerPathAndArgs.error; // Validate paths (directories) errors.includePath = this.validatePath(config.includePath, { globPaths: true }); @@ -1928,7 +1932,6 @@ export class CppProperties { // Check for path-related squiggles. const paths: string[] = []; - let compilerPath: string | undefined; for (const pathArray of [currentConfiguration.browse ? currentConfiguration.browse.path : undefined, currentConfiguration.includePath, currentConfiguration.macFrameworkPath]) { if (pathArray) { for (const curPath of pathArray) { @@ -1950,13 +1953,6 @@ export class CppProperties { paths.push(`${file}`); }); - if (currentConfiguration.compilerPath) { - // Unlike other cases, compilerPath may not start or end with " due to trimming of whitespace and the possibility of compiler args. - compilerPath = currentConfiguration.compilerPath; - } - - compilerPath = this.resolvePath(compilerPath).trim(); - // Get the start/end for properties that are file-only. const forcedIncludeStart: number = curText.search(/\s*\"forcedInclude\"\s*:\s*\[/); const forcedeIncludeEnd: number = forcedIncludeStart === -1 ? -1 : curText.indexOf("]", forcedIncludeStart); @@ -1973,46 +1969,20 @@ export class CppProperties { const processedPaths: Set = new Set(); // Validate compiler paths - let compilerPathNeedsQuotes: boolean = false; - let compilerMessage: string | undefined; - const compilerPathAndArgs: util.CompilerPathAndArgs = util.extractCompilerPathAndArgs(!!settings.legacyCompilerArgsBehavior, compilerPath); - const compilerLowerCase: string = compilerPathAndArgs.compilerName.toLowerCase(); - const isClCompiler: boolean = compilerLowerCase === "cl" || compilerLowerCase === "cl.exe"; - // Don't squiggle for invalid cl and cl.exe paths. - if (compilerPathAndArgs.compilerPath && !isClCompiler) { - // Squiggle when the compiler's path has spaces without quotes but args are used. - compilerPathNeedsQuotes = (compilerPathAndArgs.compilerArgsFromCommandLineInPath && compilerPathAndArgs.compilerArgsFromCommandLineInPath.length > 0) - && !compilerPath.startsWith('"') - && compilerPathAndArgs.compilerPath.includes(" "); - compilerPath = compilerPathAndArgs.compilerPath; - // Don't squiggle if compiler path is resolving with environment path. - if (compilerPathNeedsQuotes || (compilerPath && !which.sync(compilerPath, { nothrow: true }))) { - if (compilerPathNeedsQuotes) { - compilerMessage = localize("path.with.spaces", 'Compiler path with spaces and arguments is missing double quotes " around the path.'); - newSquiggleMetrics.CompilerPathMissingQuotes++; - } else if (!util.checkExecutableWithoutExtensionExistsSync(compilerPath)) { - compilerMessage = localize("path.is.not.a.file", "Path is not a file: {0}", compilerPath); - newSquiggleMetrics.PathNotAFile++; - } - } - } - let compilerPathExists: boolean = true; - if (this.rootUri && !isClCompiler) { - const checkPathExists: any = util.checkPathExistsSync(compilerPath, this.rootUri.fsPath + path.sep, isWindows, true); - compilerPathExists = checkPathExists.pathExists; - compilerPath = checkPathExists.path; - } - if (!compilerPathExists) { - compilerMessage = localize('cannot.find', "Cannot find: {0}", compilerPath); - newSquiggleMetrics.PathNonExistent++; - } - if (compilerMessage) { + const resolvedCompilerPath = this.resolvePath(currentConfiguration.compilerPath, false, false); + const compilerPathAndArgs: util.CompilerPathAndArgs = CppProperties.validateCompilerPath(resolvedCompilerPath, this.rootUri); + if (compilerPathAndArgs.error) { const diagnostic: vscode.Diagnostic = new vscode.Diagnostic( - new vscode.Range(document.positionAt(curTextStartOffset + compilerPathValueStart), - document.positionAt(curTextStartOffset + compilerPathEnd)), - compilerMessage, vscode.DiagnosticSeverity.Warning); + new vscode.Range(document.positionAt(curTextStartOffset + compilerPathValueStart), document.positionAt(curTextStartOffset + compilerPathEnd)), + compilerPathAndArgs.error, + vscode.DiagnosticSeverity.Warning); diagnostics.push(diagnostic); } + if (compilerPathAndArgs.telemetry) { + for (const o of Object.keys(compilerPathAndArgs.telemetry)) { + newSquiggleMetrics[o] = compilerPathAndArgs.telemetry[o]; + } + } // validate .config path let dotConfigPath: string | undefined; diff --git a/Extension/src/LanguageServer/cppBuildTaskProvider.ts b/Extension/src/LanguageServer/cppBuildTaskProvider.ts index 014d2316c..de08d6e43 100644 --- a/Extension/src/LanguageServer/cppBuildTaskProvider.ts +++ b/Extension/src/LanguageServer/cppBuildTaskProvider.ts @@ -98,7 +98,7 @@ export class CppBuildTaskProvider implements TaskProvider { // Get user compiler path. const userCompilerPathAndArgs: util.CompilerPathAndArgs | undefined = await activeClient.getCurrentCompilerPathAndArgs(); - let userCompilerPath: string | undefined | null; + let userCompilerPath: string | undefined; if (userCompilerPathAndArgs) { userCompilerPath = userCompilerPathAndArgs.compilerPath; if (userCompilerPath && userCompilerPathAndArgs.compilerName) { diff --git a/Extension/src/common.ts b/Extension/src/common.ts index fba6c3ac3..40db7ec4a 100644 --- a/Extension/src/common.ts +++ b/Extension/src/common.ts @@ -1092,15 +1092,20 @@ export function isCl(compilerPath: string): boolean { /** CompilerPathAndArgs retains original casing of text input for compiler path and args */ export interface CompilerPathAndArgs { - compilerPath?: string | null; + compilerPath?: string; compilerName: string; compilerArgs?: string[]; compilerArgsFromCommandLineInPath: string[]; allCompilerArgs: string[]; + error?: string; + telemetry?: { [key: string]: number }; } -export function extractCompilerPathAndArgs(useLegacyBehavior: boolean, inputCompilerPath?: string | null, compilerArgs?: string[]): CompilerPathAndArgs { - let compilerPath: string | undefined | null = inputCompilerPath; +/** + * Parse the compiler path input into a compiler path and compiler args. If there are no args, the compilerPath will have been verified to exist. + */ +export function extractCompilerPathAndArgs(useLegacyBehavior: boolean, inputCompilerPath?: string, compilerArgs?: string[], cwd?: string): CompilerPathAndArgs { + let compilerPath: string | undefined = inputCompilerPath; let compilerName: string = ""; let compilerArgsFromCommandLineInPath: string[] = []; if (compilerPath) { @@ -1108,58 +1113,45 @@ export function extractCompilerPathAndArgs(useLegacyBehavior: boolean, inputComp if (isCl(compilerPath) || checkExecutableWithoutExtensionExistsSync(compilerPath)) { // If the path ends with cl, or if a file is found at that path, accept it without further validation. compilerName = path.basename(compilerPath); + } else if (cwd && checkExecutableWithoutExtensionExistsSync(path.join(cwd, compilerPath))) { + // If the path is relative and a file is found at that path, accept it without further validation. + compilerPath = path.join(cwd, compilerPath); + compilerName = path.basename(compilerPath); } else if (compilerPath.startsWith("\"") || (os.platform() !== 'win32' && compilerPath.startsWith("'"))) { // If the string starts with a quote, treat it as a command line. // Otherwise, a path with a leading quote would not be valid. - if (useLegacyBehavior) { - compilerArgsFromCommandLineInPath = legacyExtractArgs(compilerPath); - if (compilerArgsFromCommandLineInPath.length > 0) { - compilerPath = compilerArgsFromCommandLineInPath.shift(); - if (compilerPath) { + compilerArgsFromCommandLineInPath = useLegacyBehavior ? legacyExtractArgs(compilerPath) : extractArgs(compilerPath); + if (compilerArgsFromCommandLineInPath.length > 0) { + compilerPath = compilerArgsFromCommandLineInPath.shift(); + if (compilerPath) { + if (useLegacyBehavior) { // Try to trim quotes from compiler path. - const tempCompilerPath: string[] | undefined = extractArgs(compilerPath); - if (tempCompilerPath && compilerPath.length > 0) { + const tempCompilerPath: string[] = extractArgs(compilerPath); + if (tempCompilerPath?.length > 0) { compilerPath = tempCompilerPath[0]; } - compilerName = path.basename(compilerPath); - } - } - } else { - compilerArgsFromCommandLineInPath = extractArgs(compilerPath); - if (compilerArgsFromCommandLineInPath.length > 0) { - compilerPath = compilerArgsFromCommandLineInPath.shift(); - if (compilerPath) { - compilerName = path.basename(compilerPath); } + compilerName = path.basename(compilerPath); } } } else { - const spaceStart: number = compilerPath.lastIndexOf(" "); - if (spaceStart !== -1) { - // There is no leading quote, but a space suggests it might be a command line. - // Try processing it as a command line, and validate that by checking for the executable. + if (compilerPath.includes(' ')) { + // There is no leading quote, so we'll treat is as a command line. const potentialArgs: string[] = useLegacyBehavior ? legacyExtractArgs(compilerPath) : extractArgs(compilerPath); let potentialCompilerPath: string | undefined = potentialArgs.shift(); - if (useLegacyBehavior) { - if (potentialCompilerPath) { - const tempCompilerPath: string[] | undefined = extractArgs(potentialCompilerPath); - if (tempCompilerPath && compilerPath.length > 0) { - potentialCompilerPath = tempCompilerPath[0]; - } - } - } - if (potentialCompilerPath) { - if (isCl(potentialCompilerPath) || checkExecutableWithoutExtensionExistsSync(potentialCompilerPath)) { - compilerArgsFromCommandLineInPath = potentialArgs; - compilerPath = potentialCompilerPath; - compilerName = path.basename(compilerPath); + if (useLegacyBehavior && potentialCompilerPath) { + const tempCompilerPath: string[] = extractArgs(potentialCompilerPath); + if (tempCompilerPath?.length > 0) { + potentialCompilerPath = tempCompilerPath[0]; } } + compilerArgsFromCommandLineInPath = potentialArgs; + compilerPath = potentialCompilerPath; } + compilerName = path.basename(compilerPath ?? ''); } } - let allCompilerArgs: string[] = !compilerArgs ? [] : compilerArgs; - allCompilerArgs = allCompilerArgs.concat(compilerArgsFromCommandLineInPath); + const allCompilerArgs: string[] = (compilerArgs ?? []).concat(compilerArgsFromCommandLineInPath); return { compilerPath, compilerName, compilerArgs, compilerArgsFromCommandLineInPath, allCompilerArgs }; } diff --git a/Extension/test/scenarios/SingleRootProject/assets/b i n/clang++ b/Extension/test/scenarios/SingleRootProject/assets/b i n/clang++ new file mode 100644 index 000000000..e69de29bb diff --git a/Extension/test/scenarios/SingleRootProject/assets/b i n/clang++.exe b/Extension/test/scenarios/SingleRootProject/assets/b i n/clang++.exe new file mode 100644 index 000000000..e69de29bb diff --git a/Extension/test/scenarios/SingleRootProject/assets/bin/cl.exe b/Extension/test/scenarios/SingleRootProject/assets/bin/cl.exe new file mode 100644 index 000000000..e69de29bb diff --git a/Extension/test/scenarios/SingleRootProject/assets/bin/clang-cl.exe b/Extension/test/scenarios/SingleRootProject/assets/bin/clang-cl.exe new file mode 100644 index 000000000..e69de29bb diff --git a/Extension/test/scenarios/SingleRootProject/assets/bin/gcc b/Extension/test/scenarios/SingleRootProject/assets/bin/gcc new file mode 100644 index 000000000..e69de29bb diff --git a/Extension/test/scenarios/SingleRootProject/assets/bin/gcc.exe b/Extension/test/scenarios/SingleRootProject/assets/bin/gcc.exe new file mode 100644 index 000000000..e69de29bb diff --git a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts new file mode 100644 index 000000000..8d01de71a --- /dev/null +++ b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts @@ -0,0 +1,223 @@ +/* -------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All Rights Reserved. + * See 'LICENSE' in the project root for license information. + * ------------------------------------------------------------------------------------------ */ + +import { describe, it } from 'mocha'; +import { deepEqual, equal, ok } from 'node:assert'; +import * as path from 'path'; +import { Uri } from 'vscode'; +import { CppProperties } from '../../../../src/LanguageServer/configurations'; +import { extractCompilerPathAndArgs } from '../../../../src/common'; +import { isWindows } from '../../../../src/constants'; + +const assetsFolder = Uri.file(path.normalize(path.join(__dirname.replace(/dist[\/\\]/, ''), '..', 'assets'))); +const assetsFolderFsPath = assetsFolder.fsPath; + +if (isWindows) { + describe('extractCompilerPathAndArgs', () => { + // [compilerPath, useLegacyBehavior, additionalArgs, result.compilerName, result.allCompilerArgs] + const nonArgsTests: [string, boolean, string[] | undefined, string, string[]][] = [ + ['cl', false, undefined, 'cl', []], + ['cl.exe', false, undefined, 'cl.exe', []], + [path.join(assetsFolderFsPath, 'bin', 'cl.exe'), false, undefined, 'cl.exe', []], + [path.join(assetsFolderFsPath, 'bin', 'gcc.exe'), false, undefined, 'gcc.exe', []], + [path.join(assetsFolderFsPath, 'b i n', 'clang++.exe'), false, undefined, 'clang++.exe', []], + [path.join(assetsFolderFsPath, 'b i n', 'clang++'), false, undefined, 'clang++', []], + [path.join('bin', 'gcc.exe'), false, undefined, 'gcc.exe', []], + [path.join('bin', 'gcc'), false, undefined, 'gcc', []] + ]; + it('Verify various compilerPath strings without args', () => { + nonArgsTests.forEach(test => { + const result = extractCompilerPathAndArgs(test[1], test[0], test[2], assetsFolderFsPath); + ok(result.compilerPath?.endsWith(test[0]), `compilerPath should end with ${test[0]}`); + equal(result.compilerName, test[3], 'compilerName should match'); + deepEqual(result.compilerArgs, test[2], 'compilerArgs should match'); + deepEqual(result.compilerArgsFromCommandLineInPath, [], 'compilerArgsFromCommandLineInPath should be empty'); + deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); + equal(result.error, undefined, 'error should be undefined'); + equal(result.telemetry, undefined, 'telemetry should be undefined'); + }); + }); + + const argsTests: [string, boolean, string[] | undefined, string, string[]][] = [ + ['cl.exe /c /Fo"test.obj" test.cpp', false, undefined, 'cl.exe', ['/c', '/Fotest.obj', 'test.cpp']], // extra quotes missing, but not needed. + ['cl.exe /c /Fo"test.obj" test.cpp', true, undefined, 'cl.exe', ['/c', '/Fo"test.obj"', 'test.cpp']], + ['cl.exe /c /Fo"test.obj" test.cpp', false, ['/O2'], 'cl.exe', ['/O2', '/c', '/Fotest.obj', 'test.cpp']], + ['cl.exe /c /Fo"test.obj" test.cpp', true, ['/O2'], 'cl.exe', ['/O2', '/c', '/Fo"test.obj"', 'test.cpp']], + [`"${path.join(assetsFolderFsPath, 'b i n', 'clang++.exe')}" -std=c++20`, false, undefined, 'clang++.exe', ['-std=c++20']], + [`"${path.join(assetsFolderFsPath, 'b i n', 'clang++.exe')}" -std=c++20`, true, undefined, 'clang++.exe', ['-std=c++20']], + [`"${path.join(assetsFolderFsPath, 'b i n', 'clang++.exe')}" -std=c++20`, false, ['-O2'], 'clang++.exe', ['-O2', '-std=c++20']], + [`"${path.join(assetsFolderFsPath, 'b i n', 'clang++.exe')}" -std=c++20`, true, ['-O2'], 'clang++.exe', ['-O2', '-std=c++20']], + [`${path.join('bin', 'gcc.exe')} -O2`, false, undefined, 'gcc.exe', ['-O2']], + [`${path.join('bin', 'gcc.exe')} -O2`, true, undefined, 'gcc.exe', ['-O2']] + ]; + it('Verify various compilerPath strings with args', () => { + argsTests.forEach(test => { + const result = extractCompilerPathAndArgs(test[1], test[0], test[2]); + const cp = test[0].substring(test[0].at(0) === '"' ? 1 : 0, test[0].indexOf(test[3]) + test[3].length); + ok(result.compilerPath?.endsWith(cp), `${result.compilerPath} !endswith ${cp}`); + equal(result.compilerName, test[3], 'compilerName should match'); + deepEqual(result.compilerArgs, test[2], 'compilerArgs should match'); + deepEqual(result.compilerArgsFromCommandLineInPath, test[4].filter(a => !test[2]?.includes(a)), 'compilerArgsFromCommandLineInPath should match those from the command line'); + deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); + equal(result.error, undefined, 'error should be undefined'); + equal(result.telemetry, undefined, 'telemetry should be undefined'); + }); + }); + + const negativeTests: [string, boolean, string[] | undefined, string, string[]][] = [ + [`${path.join(assetsFolderFsPath, 'b i n', 'clang++.exe')} -std=c++20`, false, undefined, 'b', ['i', 'n\\clang++.exe', '-std=c++20']] + ]; + it('Verify various compilerPath strings with args that should fail', () => { + negativeTests.forEach(test => { + const result = extractCompilerPathAndArgs(test[1], test[0], test[2]); + equal(result.compilerName, test[3], 'compilerName should match'); + deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); + + // errors and telemetry are set by validateCompilerPath + equal(result.error, undefined, 'error should be undefined'); + equal(result.telemetry, undefined, 'telemetry should be undefined'); + }); + }); + }); +} else { + describe('extractCompilerPathAndArgs', () => { + // [compilerPath, useLegacyBehavior, additionalArgs, result.compilerName, result.allCompilerArgs] + const tests: [string, boolean, string[] | undefined, string, string[]][] = [ + ['clang', false, undefined, 'clang', []], + [path.join(assetsFolderFsPath, 'bin', 'gcc'), false, undefined, 'gcc', []], + [path.join(assetsFolderFsPath, 'b i n', 'clang++'), false, undefined, 'clang++', []], + [path.join('bin', 'gcc'), false, undefined, 'gcc', []] + ]; + it('Verify various compilerPath strings without args', () => { + tests.forEach(test => { + const result = extractCompilerPathAndArgs(test[1], test[0], test[2]); + equal(result.compilerName, test[3], 'compilerName should match'); + deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); + equal(result.error, undefined, 'error should be undefined'); + equal(result.telemetry, undefined, 'telemetry should be undefined'); + }); + }); + + const argsTests: [string, boolean, string[] | undefined, string, string[]][] = [ + ['clang -O2 -Wall', false, undefined, 'clang', ['-O2', '-Wall']], + ['clang -O2 -Wall', true, undefined, 'clang', ['-O2', '-Wall']], + ['clang -O2 -Wall', false, ['-O3'], 'clang', ['-O3', '-O2', '-Wall']], + ['clang -O2 -Wall', true, ['-O3'], 'clang', ['-O3', '-O2', '-Wall']], + [`"${path.join(assetsFolderFsPath, 'b i n', 'clang++')}" -std=c++20`, false, undefined, 'clang++', ['-std=c++20']], + [`"${path.join(assetsFolderFsPath, 'b i n', 'clang++')}" -std=c++20`, true, undefined, 'clang++', ['-std=c++20']], + [`"${path.join(assetsFolderFsPath, 'b i n', 'clang++')}" -std=c++20`, false, ['-O2'], 'clang++', ['-O2', '-std=c++20']], + [`"${path.join(assetsFolderFsPath, 'b i n', 'clang++')}" -std=c++20`, true, ['-O2'], 'clang++', ['-O2', '-std=c++20']], + [`${path.join('bin', 'gcc')} -O2`, false, undefined, 'gcc', ['-O2']], + [`${path.join('bin', 'gcc')} -O2`, true, undefined, 'gcc', ['-O2']] + ]; + it('Verify various compilerPath strings with args', () => { + argsTests.forEach(test => { + const result = extractCompilerPathAndArgs(test[1], test[0], test[2]); + equal(result.compilerName, test[3], 'compilerName should match'); + deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); + equal(result.error, undefined, 'error should be undefined'); + equal(result.telemetry, undefined, 'telemetry should be undefined'); + }); + }); + + const negativeTests: [string, boolean, string[] | undefined, string, string[]][] = [ + [`${path.join(assetsFolderFsPath, 'b i n', 'clang++')} -std=c++20`, false, undefined, 'b', ['i', 'n/clang++', '-std=c++20']] + ]; + it('Verify various compilerPath strings with args that should fail', () => { + negativeTests.forEach(test => { + const result = extractCompilerPathAndArgs(test[1], test[0], test[2]); + equal(result.compilerName, test[3], 'compilerName should match'); + deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); + + // errors and telemetry are set by validateCompilerPath + equal(result.error, undefined, 'error should be undefined'); + equal(result.telemetry, undefined, 'telemetry should be undefined'); + }); + }); + }); +} + +describe('validateCompilerPath', () => { + // [compilerPath, cwd, result.compilerName, result.allCompilerArgs, result.error, result.telemetry] + const tests: [string, Uri, string, string[]][] = [ + ['cl.exe', assetsFolder, 'cl.exe', []], + ['cl', assetsFolder, 'cl', []], + ['clang', assetsFolder, 'clang', []], + [path.join(assetsFolderFsPath, 'bin', 'cl'), assetsFolder, 'cl', []], + [path.join(assetsFolderFsPath, 'bin', 'clang-cl'), assetsFolder, 'clang-cl', []], + [path.join(assetsFolderFsPath, 'bin', 'gcc'), assetsFolder, 'gcc', []], + [path.join(assetsFolderFsPath, 'b i n', 'clang++'), assetsFolder, 'clang++', []], + [path.join('bin', 'gcc'), assetsFolder, 'gcc', []], + [path.join('bin', 'clang-cl'), assetsFolder, 'clang-cl', []], + ['', assetsFolder, '', []], + [' cl.exe ', assetsFolder, 'cl.exe', []] + ]; + it('Verify various compilerPath strings without args', () => { + let index = -1; + tests.forEach(test => { + index++; + if (!isWindows && test[0].includes('clang-cl')) { + return; // This test is for checking the addition of .exe to the compiler name on Windows only. + } + const result = CppProperties.validateCompilerPath(test[0], test[1]); + equal(result.compilerName, test[2], `(test ${index}) compilerName should match`); + deepEqual(result.allCompilerArgs, test[3], `(test ${index}) allCompilerArgs should match`); + equal(result.error, undefined, `(test ${index}) error should be undefined`); + deepEqual(result.telemetry, test[0] === '' ? undefined : {}, `(test ${index}) telemetry should be empty`); + }); + }); + + const argsTests: [string, Uri, string, string[]][] = [ + ['cl.exe /std:c++20 /O2', assetsFolder, 'cl.exe', ['/std:c++20', '/O2']], // issue with /Fo"test.obj" argument + [`"${path.join(assetsFolderFsPath, 'b i n', 'clang++')}" -std=c++20 -O2`, assetsFolder, 'clang++', ['-std=c++20', '-O2']], + [`${path.join('bin', 'gcc')} -std=c++20 -Wall`, assetsFolder, 'gcc', ['-std=c++20', '-Wall']] + ]; + it('Verify various compilerPath strings with args', () => { + let index = -1; + argsTests.forEach(test => { + index++; + const result = CppProperties.validateCompilerPath(test[0], test[1]); + equal(result.compilerName, test[2], `(test ${index}) compilerName should match`); + deepEqual(result.allCompilerArgs, test[3], `(test ${index}) allCompilerArgs should match`); + equal(result.error, undefined, `(test ${index} error should be undefined`); + deepEqual(result.telemetry, {}, `(test ${index}) telemetry should be empty`); + }); + }); + + it('Verify errors with invalid relative compiler path', async () => { + const result = CppProperties.validateCompilerPath(path.join('assets', 'bin', 'gcc'), assetsFolder); + equal(result.compilerName, 'gcc', 'compilerName should be found'); + equal(result.allCompilerArgs.length, 0, 'Should not have any args'); + ok(result.error?.includes('Cannot find'), 'Should have an error for relative paths'); + equal(result.telemetry?.PathNonExistent, 1, 'Should have telemetry for relative paths'); + equal(result.telemetry?.PathNotAFile, undefined, 'Should not have telemetry for invalid paths'); + }); + + it('Verify errors with invalid absolute compiler path', async () => { + const result = CppProperties.validateCompilerPath(path.join(assetsFolderFsPath, 'assets', 'bin', 'gcc'), assetsFolder); + equal(result.compilerName, 'gcc', 'compilerName should be found'); + equal(result.allCompilerArgs.length, 0, 'Should not have any args'); + ok(result.error?.includes('Cannot find'), 'Should have an error for absolute paths'); + equal(result.telemetry?.PathNonExistent, 1, 'Should have telemetry for absolute paths'); + equal(result.telemetry?.PathNotAFile, undefined, 'Should not have telemetry for invalid paths'); + }); + + it('Verify errors with non-file compilerPath', async () => { + const result = CppProperties.validateCompilerPath('bin', assetsFolder); + equal(result.compilerName, 'bin', 'compilerName should be found'); + equal(result.allCompilerArgs.length, 0, 'Should not have any args'); + ok(result.error?.includes('Path is not a file'), 'Should have an error for non-file paths'); + equal(result.telemetry?.PathNonExistent, undefined, 'Should not have telemetry for relative paths'); + equal(result.telemetry?.PathNotAFile, 1, 'Should have telemetry for invalid paths'); + }); + + it('Verify errors with unknown compiler not in Path', async () => { + const result = CppProperties.validateCompilerPath('icc', assetsFolder); + equal(result.compilerName, 'icc', 'compilerName should be found'); + equal(result.allCompilerArgs.length, 0, 'Should not have any args'); + equal(result.telemetry?.PathNonExistent, 1, 'Should have telemetry for relative paths'); + equal(result.telemetry?.PathNotAFile, undefined, 'Should not have telemetry for invalid paths'); + }); +}); From 6a8155dd835407d57b5607b66ef91307125bba39 Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Thu, 24 Apr 2025 17:06:47 -0700 Subject: [PATCH 02/14] Add back an error for missing quotes when the compiler parsed does not exist --- .../src/LanguageServer/configurations.ts | 77 ++++++++++--------- Extension/src/common.ts | 34 ++++---- .../tests/compilerPath.test.ts | 15 +++- 3 files changed, 68 insertions(+), 58 deletions(-) diff --git a/Extension/src/LanguageServer/configurations.ts b/Extension/src/LanguageServer/configurations.ts index c4f1ac158..fc8b37113 100644 --- a/Extension/src/LanguageServer/configurations.ts +++ b/Extension/src/LanguageServer/configurations.ts @@ -1614,51 +1614,54 @@ export class CppProperties { const telemetry: { [key: string]: number } = {}; // Don't error cl.exe paths because it could be for an older preview build. - if (!isCl) { - // Get compiler path without arguments before checking if it exists - if (compilerPathAndArgs.compilerPath) { - resolvedCompilerPath = compilerPathAndArgs.compilerPath; - let pathExists: boolean = true; - const existsWithExeAdded: (path: string) => boolean = (path: string) => isWindows && !path.startsWith("/") && fs.existsSync(path + ".exe"); - if (!fs.existsSync(resolvedCompilerPath)) { - if (existsWithExeAdded(resolvedCompilerPath)) { - resolvedCompilerPath += ".exe"; - } else { - const pathLocation = which.sync(resolvedCompilerPath, { nothrow: true }); - if (pathLocation) { - resolvedCompilerPath = pathLocation; - compilerPathAndArgs.compilerPath = pathLocation; - } else if (rootUri) { - // Check again for a relative path. - const relativePath: string = rootUri.fsPath + path.sep + resolvedCompilerPath; - if (!fs.existsSync(relativePath)) { - if (existsWithExeAdded(relativePath)) { - resolvedCompilerPath = relativePath + ".exe"; - } else { - pathExists = false; - } + if (!isCl && compilerPathAndArgs.compilerPath) { + const compilerPathMayNeedQuotes: boolean = !resolvedCompilerPath.startsWith('"') && resolvedCompilerPath.includes(" ") && compilerPathAndArgs.compilerArgsFromCommandLineInPath.length > 0; + let pathExists: boolean = true; + const existsWithExeAdded: (path: string) => boolean = (path: string) => isWindows && !path.startsWith("/") && fs.existsSync(path + ".exe"); + + resolvedCompilerPath = compilerPathAndArgs.compilerPath; + if (!fs.existsSync(resolvedCompilerPath)) { + if (existsWithExeAdded(resolvedCompilerPath)) { + resolvedCompilerPath += ".exe"; + } else { + const pathLocation = which.sync(resolvedCompilerPath, { nothrow: true }); + if (pathLocation) { + resolvedCompilerPath = pathLocation; + compilerPathAndArgs.compilerPath = pathLocation; + } else if (rootUri) { + // Check again for a relative path. + const relativePath: string = rootUri.fsPath + path.sep + resolvedCompilerPath; + if (!fs.existsSync(relativePath)) { + if (existsWithExeAdded(relativePath)) { + resolvedCompilerPath = relativePath + ".exe"; } else { - resolvedCompilerPath = relativePath; + pathExists = false; } + } else { + resolvedCompilerPath = relativePath; } } } + } - const compilerPathErrors: string[] = []; + const compilerPathErrors: string[] = []; + if (compilerPathMayNeedQuotes && !pathExists) { + compilerPathErrors.push(localize("path.with.spaces", 'Compiler path with spaces and arguments is missing double quotes " around the path.')); + telemetry.CompilerPathMissingQuotes = 1; + } - if (!pathExists) { - const message: string = localize('cannot.find', "Cannot find: {0}", resolvedCompilerPath); - compilerPathErrors.push(message); - telemetry.PathNonExistent = 1; - } else if (!util.checkExecutableWithoutExtensionExistsSync(resolvedCompilerPath)) { - const message: string = localize("path.is.not.a.file", "Path is not a file: {0}", resolvedCompilerPath); - compilerPathErrors.push(message); - telemetry.PathNotAFile = 1; - } + if (!pathExists) { + const message: string = localize('cannot.find', "Cannot find: {0}", resolvedCompilerPath); + compilerPathErrors.push(message); + telemetry.PathNonExistent = 1; + } else if (!util.checkExecutableWithoutExtensionExistsSync(resolvedCompilerPath)) { + const message: string = localize("path.is.not.a.file", "Path is not a file: {0}", resolvedCompilerPath); + compilerPathErrors.push(message); + telemetry.PathNotAFile = 1; + } - if (compilerPathErrors.length > 0) { - compilerPathAndArgs.error = compilerPathErrors.join('\n'); - } + if (compilerPathErrors.length > 0) { + compilerPathAndArgs.error = compilerPathErrors.join('\n'); } } compilerPathAndArgs.telemetry = telemetry; diff --git a/Extension/src/common.ts b/Extension/src/common.ts index 40db7ec4a..23fa2dafb 100644 --- a/Extension/src/common.ts +++ b/Extension/src/common.ts @@ -1108,6 +1108,16 @@ export function extractCompilerPathAndArgs(useLegacyBehavior: boolean, inputComp let compilerPath: string | undefined = inputCompilerPath; let compilerName: string = ""; let compilerArgsFromCommandLineInPath: string[] = []; + const trimLegacyQuotes = (compilerPath?: string): string | undefined => { + if (compilerPath && useLegacyBehavior) { + // Try to trim quotes from compiler path. + const tempCompilerPath: string[] = extractArgs(compilerPath); + if (tempCompilerPath.length > 0) { + return tempCompilerPath[0]; + } + } + return compilerPath; + }; if (compilerPath) { compilerPath = compilerPath.trim(); if (isCl(compilerPath) || checkExecutableWithoutExtensionExistsSync(compilerPath)) { @@ -1122,31 +1132,15 @@ export function extractCompilerPathAndArgs(useLegacyBehavior: boolean, inputComp // Otherwise, a path with a leading quote would not be valid. compilerArgsFromCommandLineInPath = useLegacyBehavior ? legacyExtractArgs(compilerPath) : extractArgs(compilerPath); if (compilerArgsFromCommandLineInPath.length > 0) { - compilerPath = compilerArgsFromCommandLineInPath.shift(); - if (compilerPath) { - if (useLegacyBehavior) { - // Try to trim quotes from compiler path. - const tempCompilerPath: string[] = extractArgs(compilerPath); - if (tempCompilerPath?.length > 0) { - compilerPath = tempCompilerPath[0]; - } - } - compilerName = path.basename(compilerPath); - } + compilerPath = trimLegacyQuotes(compilerArgsFromCommandLineInPath.shift()); + compilerName = path.basename(compilerPath ?? ''); } } else { if (compilerPath.includes(' ')) { - // There is no leading quote, so we'll treat is as a command line. + // There is no leading quote, so we'll treat it as a command line. const potentialArgs: string[] = useLegacyBehavior ? legacyExtractArgs(compilerPath) : extractArgs(compilerPath); - let potentialCompilerPath: string | undefined = potentialArgs.shift(); - if (useLegacyBehavior && potentialCompilerPath) { - const tempCompilerPath: string[] = extractArgs(potentialCompilerPath); - if (tempCompilerPath?.length > 0) { - potentialCompilerPath = tempCompilerPath[0]; - } - } + compilerPath = trimLegacyQuotes(potentialArgs.shift()); compilerArgsFromCommandLineInPath = potentialArgs; - compilerPath = potentialCompilerPath; } compilerName = path.basename(compilerPath ?? ''); } diff --git a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts index 8d01de71a..9121f8a86 100644 --- a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts +++ b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts @@ -172,7 +172,8 @@ describe('validateCompilerPath', () => { const argsTests: [string, Uri, string, string[]][] = [ ['cl.exe /std:c++20 /O2', assetsFolder, 'cl.exe', ['/std:c++20', '/O2']], // issue with /Fo"test.obj" argument [`"${path.join(assetsFolderFsPath, 'b i n', 'clang++')}" -std=c++20 -O2`, assetsFolder, 'clang++', ['-std=c++20', '-O2']], - [`${path.join('bin', 'gcc')} -std=c++20 -Wall`, assetsFolder, 'gcc', ['-std=c++20', '-Wall']] + [`${path.join('bin', 'gcc')} -std=c++20 -Wall`, assetsFolder, 'gcc', ['-std=c++20', '-Wall']], + ['clang -O2 -Wall', assetsFolder, 'clang', ['-O2', '-Wall']] ]; it('Verify various compilerPath strings with args', () => { let index = -1; @@ -220,4 +221,16 @@ describe('validateCompilerPath', () => { equal(result.telemetry?.PathNonExistent, 1, 'Should have telemetry for relative paths'); equal(result.telemetry?.PathNotAFile, undefined, 'Should not have telemetry for invalid paths'); }); + + it('Verify errors with unknown compiler not in Path with args', async () => { + const result = CppProperties.validateCompilerPath('icc -O2', assetsFolder); + equal(result.compilerName, 'icc', 'compilerName should be found'); + deepEqual(result.allCompilerArgs, ['-O2'], 'args should match'); + ok(result.error?.includes('Cannot find'), 'Should have an error for unknown compiler'); + ok(result.error?.includes('missing double quotes'), 'Should have an error for missing double quotes'); + equal(result.telemetry?.PathNonExistent, 1, 'Should have telemetry for relative paths'); + equal(result.telemetry?.PathNotAFile, undefined, 'Should not have telemetry for invalid paths'); + equal(result.telemetry?.CompilerPathMissingQuotes, 1, 'Should have telemetry for missing quotes'); + }); + }); From d2212dae312667f3baf8ba36ee4a1dcfce0b14c6 Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Fri, 25 Apr 2025 11:25:25 -0700 Subject: [PATCH 03/14] test updates --- .../tests/compilerPath.test.ts | 126 +++++++++++------- 1 file changed, 81 insertions(+), 45 deletions(-) diff --git a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts index 9121f8a86..3e439d16e 100644 --- a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts +++ b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts @@ -14,6 +14,19 @@ import { isWindows } from '../../../../src/constants'; const assetsFolder = Uri.file(path.normalize(path.join(__dirname.replace(/dist[\/\\]/, ''), '..', 'assets'))); const assetsFolderFsPath = assetsFolder.fsPath; +// A simple test counter for the tests that loop over several cases. +// This is to make it easier to see which test failed in the output. +// Start the counter with 1 instead of 0 since we're reporting on test cases, not arrays. +class Counter { + private count: number = 1; + public next(): void { + this.count++; + } + public get str(): string { + return `(test ${this.count})`; + } +} + if (isWindows) { describe('extractCompilerPathAndArgs', () => { // [compilerPath, useLegacyBehavior, additionalArgs, result.compilerName, result.allCompilerArgs] @@ -28,15 +41,19 @@ if (isWindows) { [path.join('bin', 'gcc'), false, undefined, 'gcc', []] ]; it('Verify various compilerPath strings without args', () => { + const c = new Counter(); nonArgsTests.forEach(test => { const result = extractCompilerPathAndArgs(test[1], test[0], test[2], assetsFolderFsPath); - ok(result.compilerPath?.endsWith(test[0]), `compilerPath should end with ${test[0]}`); - equal(result.compilerName, test[3], 'compilerName should match'); - deepEqual(result.compilerArgs, test[2], 'compilerArgs should match'); - deepEqual(result.compilerArgsFromCommandLineInPath, [], 'compilerArgsFromCommandLineInPath should be empty'); - deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); - equal(result.error, undefined, 'error should be undefined'); - equal(result.telemetry, undefined, 'telemetry should be undefined'); + ok(result.compilerPath?.endsWith(test[0]), `${c.str} compilerPath should end with ${test[0]}`); + equal(result.compilerName, test[3], `${c.str} compilerName should match`); + deepEqual(result.compilerArgs, test[2], `${c.str} compilerArgs should match`); + deepEqual(result.compilerArgsFromCommandLineInPath, [], `${c.str} compilerArgsFromCommandLineInPath should be empty`); + deepEqual(result.allCompilerArgs, test[4], `${c.str} allCompilerArgs should match`); + + // errors and telemetry are set by validateCompilerPath + equal(result.error, undefined, `${c.str} error should be undefined`); + equal(result.telemetry, undefined, `${c.str} telemetry should be undefined`); + c.next(); }); }); @@ -53,16 +70,20 @@ if (isWindows) { [`${path.join('bin', 'gcc.exe')} -O2`, true, undefined, 'gcc.exe', ['-O2']] ]; it('Verify various compilerPath strings with args', () => { + const c = new Counter(); argsTests.forEach(test => { const result = extractCompilerPathAndArgs(test[1], test[0], test[2]); const cp = test[0].substring(test[0].at(0) === '"' ? 1 : 0, test[0].indexOf(test[3]) + test[3].length); - ok(result.compilerPath?.endsWith(cp), `${result.compilerPath} !endswith ${cp}`); - equal(result.compilerName, test[3], 'compilerName should match'); - deepEqual(result.compilerArgs, test[2], 'compilerArgs should match'); - deepEqual(result.compilerArgsFromCommandLineInPath, test[4].filter(a => !test[2]?.includes(a)), 'compilerArgsFromCommandLineInPath should match those from the command line'); - deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); - equal(result.error, undefined, 'error should be undefined'); - equal(result.telemetry, undefined, 'telemetry should be undefined'); + ok(result.compilerPath?.endsWith(cp), `${c.str} ${result.compilerPath} !endswith ${cp}`); + equal(result.compilerName, test[3], `${c.str} compilerName should match`); + deepEqual(result.compilerArgs, test[2], `${c.str} compilerArgs should match`); + deepEqual(result.compilerArgsFromCommandLineInPath, test[4].filter(a => !test[2]?.includes(a)), `${c.str} compilerArgsFromCommandLineInPath should match those from the command line`); + deepEqual(result.allCompilerArgs, test[4], `${c.str} allCompilerArgs should match`); + + // errors and telemetry are set by validateCompilerPath + equal(result.error, undefined, `${c.str} error should be undefined`); + equal(result.telemetry, undefined, `${c.str} telemetry should be undefined`); + c.next(); }); }); @@ -70,14 +91,19 @@ if (isWindows) { [`${path.join(assetsFolderFsPath, 'b i n', 'clang++.exe')} -std=c++20`, false, undefined, 'b', ['i', 'n\\clang++.exe', '-std=c++20']] ]; it('Verify various compilerPath strings with args that should fail', () => { + const c = new Counter(); negativeTests.forEach(test => { const result = extractCompilerPathAndArgs(test[1], test[0], test[2]); - equal(result.compilerName, test[3], 'compilerName should match'); - deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); + ok(result.compilerPath?.endsWith(test[3]), `${c.str} ${result.compilerPath} !endswith ${test[3]}`); + equal(result.compilerName, test[3], `${c.str} compilerName should match`); + deepEqual(result.compilerArgs, test[2], `${c.str} compilerArgs should match`); + deepEqual(result.compilerArgsFromCommandLineInPath, test[4], `${c.str} allCompilerArgs should match`); + deepEqual(result.allCompilerArgs, test[4], `${c.str} allCompilerArgs should match`); // errors and telemetry are set by validateCompilerPath - equal(result.error, undefined, 'error should be undefined'); - equal(result.telemetry, undefined, 'telemetry should be undefined'); + equal(result.error, undefined, `${c.str} error should be undefined`); + equal(result.telemetry, undefined, `${c.str} telemetry should be undefined`); + c.next(); }); }); }); @@ -91,12 +117,14 @@ if (isWindows) { [path.join('bin', 'gcc'), false, undefined, 'gcc', []] ]; it('Verify various compilerPath strings without args', () => { + const c = new Counter(); tests.forEach(test => { const result = extractCompilerPathAndArgs(test[1], test[0], test[2]); - equal(result.compilerName, test[3], 'compilerName should match'); - deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); - equal(result.error, undefined, 'error should be undefined'); - equal(result.telemetry, undefined, 'telemetry should be undefined'); + equal(result.compilerName, test[3], `${c.str} compilerName should match`); + deepEqual(result.allCompilerArgs, test[4], `${c.str} allCompilerArgs should match`); + equal(result.error, undefined, `${c.str} error should be undefined`); + equal(result.telemetry, undefined, `${c.str} telemetry should be undefined`); + c.next(); }); }); @@ -113,12 +141,14 @@ if (isWindows) { [`${path.join('bin', 'gcc')} -O2`, true, undefined, 'gcc', ['-O2']] ]; it('Verify various compilerPath strings with args', () => { + const c = new Counter(); argsTests.forEach(test => { const result = extractCompilerPathAndArgs(test[1], test[0], test[2]); - equal(result.compilerName, test[3], 'compilerName should match'); - deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); - equal(result.error, undefined, 'error should be undefined'); - equal(result.telemetry, undefined, 'telemetry should be undefined'); + equal(result.compilerName, test[3], `${c.str} compilerName should match`); + deepEqual(result.allCompilerArgs, test[4], `${c.str} allCompilerArgs should match`); + equal(result.error, undefined, `${c.str} error should be undefined`); + equal(result.telemetry, undefined, `${c.str} telemetry should be undefined`); + c.next(); }); }); @@ -126,14 +156,16 @@ if (isWindows) { [`${path.join(assetsFolderFsPath, 'b i n', 'clang++')} -std=c++20`, false, undefined, 'b', ['i', 'n/clang++', '-std=c++20']] ]; it('Verify various compilerPath strings with args that should fail', () => { + const c = new Counter(); negativeTests.forEach(test => { const result = extractCompilerPathAndArgs(test[1], test[0], test[2]); - equal(result.compilerName, test[3], 'compilerName should match'); - deepEqual(result.allCompilerArgs, test[4], 'allCompilerArgs should match'); + equal(result.compilerName, test[3], `${c.str} compilerName should match`); + deepEqual(result.allCompilerArgs, test[4], `${c.str} allCompilerArgs should match`); // errors and telemetry are set by validateCompilerPath - equal(result.error, undefined, 'error should be undefined'); - equal(result.telemetry, undefined, 'telemetry should be undefined'); + equal(result.error, undefined, `${c.str} error should be undefined`); + equal(result.telemetry, undefined, `${c.str} telemetry should be undefined`); + c.next(); }); }); }); @@ -155,17 +187,17 @@ describe('validateCompilerPath', () => { [' cl.exe ', assetsFolder, 'cl.exe', []] ]; it('Verify various compilerPath strings without args', () => { - let index = -1; + const c = new Counter(); tests.forEach(test => { - index++; - if (!isWindows && test[0].includes('clang-cl')) { - return; // This test is for checking the addition of .exe to the compiler name on Windows only. + // Skip the clang-cl test on non-Windows. That test is for checking the addition of .exe to the compiler name on Windows only. + if (isWindows || !test[0].includes('clang-cl')) { + const result = CppProperties.validateCompilerPath(test[0], test[1]); + equal(result.compilerName, test[2], `${c.str} compilerName should match`); + deepEqual(result.allCompilerArgs, test[3], `${c.str} allCompilerArgs should match`); + equal(result.error, undefined, `${c.str} error should be undefined`); + deepEqual(result.telemetry, test[0] === '' ? undefined : {}, `${c.str} telemetry should be empty`); } - const result = CppProperties.validateCompilerPath(test[0], test[1]); - equal(result.compilerName, test[2], `(test ${index}) compilerName should match`); - deepEqual(result.allCompilerArgs, test[3], `(test ${index}) allCompilerArgs should match`); - equal(result.error, undefined, `(test ${index}) error should be undefined`); - deepEqual(result.telemetry, test[0] === '' ? undefined : {}, `(test ${index}) telemetry should be empty`); + c.next(); }); }); @@ -176,14 +208,14 @@ describe('validateCompilerPath', () => { ['clang -O2 -Wall', assetsFolder, 'clang', ['-O2', '-Wall']] ]; it('Verify various compilerPath strings with args', () => { - let index = -1; + const c = new Counter(); argsTests.forEach(test => { - index++; const result = CppProperties.validateCompilerPath(test[0], test[1]); - equal(result.compilerName, test[2], `(test ${index}) compilerName should match`); - deepEqual(result.allCompilerArgs, test[3], `(test ${index}) allCompilerArgs should match`); - equal(result.error, undefined, `(test ${index} error should be undefined`); - deepEqual(result.telemetry, {}, `(test ${index}) telemetry should be empty`); + equal(result.compilerName, test[2], `${c.str} compilerName should match`); + deepEqual(result.allCompilerArgs, test[3], `${c.str} allCompilerArgs should match`); + equal(result.error, undefined, `${c.str} error should be undefined`); + deepEqual(result.telemetry, {}, `${c.str} telemetry should be empty`); + c.next(); }); }); @@ -194,6 +226,7 @@ describe('validateCompilerPath', () => { ok(result.error?.includes('Cannot find'), 'Should have an error for relative paths'); equal(result.telemetry?.PathNonExistent, 1, 'Should have telemetry for relative paths'); equal(result.telemetry?.PathNotAFile, undefined, 'Should not have telemetry for invalid paths'); + equal(result.telemetry?.CompilerPathMissingQuotes, undefined, 'Should not have telemetry for missing quotes'); }); it('Verify errors with invalid absolute compiler path', async () => { @@ -203,6 +236,7 @@ describe('validateCompilerPath', () => { ok(result.error?.includes('Cannot find'), 'Should have an error for absolute paths'); equal(result.telemetry?.PathNonExistent, 1, 'Should have telemetry for absolute paths'); equal(result.telemetry?.PathNotAFile, undefined, 'Should not have telemetry for invalid paths'); + equal(result.telemetry?.CompilerPathMissingQuotes, undefined, 'Should not have telemetry for missing quotes'); }); it('Verify errors with non-file compilerPath', async () => { @@ -212,6 +246,7 @@ describe('validateCompilerPath', () => { ok(result.error?.includes('Path is not a file'), 'Should have an error for non-file paths'); equal(result.telemetry?.PathNonExistent, undefined, 'Should not have telemetry for relative paths'); equal(result.telemetry?.PathNotAFile, 1, 'Should have telemetry for invalid paths'); + equal(result.telemetry?.CompilerPathMissingQuotes, undefined, 'Should not have telemetry for missing quotes'); }); it('Verify errors with unknown compiler not in Path', async () => { @@ -220,6 +255,7 @@ describe('validateCompilerPath', () => { equal(result.allCompilerArgs.length, 0, 'Should not have any args'); equal(result.telemetry?.PathNonExistent, 1, 'Should have telemetry for relative paths'); equal(result.telemetry?.PathNotAFile, undefined, 'Should not have telemetry for invalid paths'); + equal(result.telemetry?.CompilerPathMissingQuotes, undefined, 'Should not have telemetry for missing quotes'); }); it('Verify errors with unknown compiler not in Path with args', async () => { From 912848e9493b2db10b75629aa11fb3302119c7dd Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Fri, 25 Apr 2025 15:18:34 -0700 Subject: [PATCH 04/14] Fix tests for linux/mac --- .../SingleRootProject/tests/compilerPath.test.ts | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts index 3e439d16e..00ce9f45e 100644 --- a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts +++ b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts @@ -7,12 +7,13 @@ import { describe, it } from 'mocha'; import { deepEqual, equal, ok } from 'node:assert'; import * as path from 'path'; import { Uri } from 'vscode'; -import { CppProperties } from '../../../../src/LanguageServer/configurations'; -import { extractCompilerPathAndArgs } from '../../../../src/common'; +import { extractCompilerPathAndArgs, setExtensionPath } from '../../../../src/common'; import { isWindows } from '../../../../src/constants'; +import { CppProperties } from '../../../../src/LanguageServer/configurations'; const assetsFolder = Uri.file(path.normalize(path.join(__dirname.replace(/dist[\/\\]/, ''), '..', 'assets'))); const assetsFolderFsPath = assetsFolder.fsPath; +const extensionPath = path.normalize(path.join(__dirname.replace(/dist[\/\\]/, ''), '..', '..', '..', '..')); // A simple test counter for the tests that loop over several cases. // This is to make it easier to see which test failed in the output. @@ -109,6 +110,10 @@ if (isWindows) { }); } else { describe('extractCompilerPathAndArgs', () => { + // The extension is not initialized the same way during tests, so this needs to be set manually + // so the tests can find `cpptools-wordexp`. + setExtensionPath(extensionPath); + // [compilerPath, useLegacyBehavior, additionalArgs, result.compilerName, result.allCompilerArgs] const tests: [string, boolean, string[] | undefined, string, string[]][] = [ ['clang', false, undefined, 'clang', []], @@ -172,6 +177,12 @@ if (isWindows) { } describe('validateCompilerPath', () => { + if (!isWindows) { + // The extension is not initialized the same way during tests, so this needs to be set manually + // so the tests can find `cpptools-wordexp`. + setExtensionPath(extensionPath); + } + // [compilerPath, cwd, result.compilerName, result.allCompilerArgs, result.error, result.telemetry] const tests: [string, Uri, string, string[]][] = [ ['cl.exe', assetsFolder, 'cl.exe', []], From 912d1ca5b58c7c3c492ad7d34bf1c66b8c56a7f2 Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Fri, 25 Apr 2025 16:16:45 -0700 Subject: [PATCH 05/14] Try to copy extension binaries for testing --- .github/workflows/job-compile-and-test.yml | 8 ++++---- .../template-copy-published-binaries.yml | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/template-copy-published-binaries.yml diff --git a/.github/workflows/job-compile-and-test.yml b/.github/workflows/job-compile-and-test.yml index 2468e22a8..9c8d82681 100644 --- a/.github/workflows/job-compile-and-test.yml +++ b/.github/workflows/job-compile-and-test.yml @@ -21,6 +21,10 @@ jobs: steps: - uses: actions/checkout@v4 + - name: Copy Published Extension Binaries + if: ${{ inputs.platform != 'windows' }} + uses: .github/workflows/template-copy-published-binaries.yml + - name: Use Node.js 22 uses: actions/setup-node@v4 with: @@ -42,11 +46,7 @@ jobs: run: yarn test working-directory: Extension - # These tests don't require the binary. - # On Linux, it is failing (before the tests actually run) with: Test run terminated with signal SIGSEGV. - # But it works on Linux during the E2E test. - name: Run SingleRootProject tests - if: ${{ inputs.platform != 'linux' }} run: yarn test --scenario=SingleRootProject --skipCheckBinaries working-directory: Extension diff --git a/.github/workflows/template-copy-published-binaries.yml b/.github/workflows/template-copy-published-binaries.yml new file mode 100644 index 000000000..ceba4e25c --- /dev/null +++ b/.github/workflows/template-copy-published-binaries.yml @@ -0,0 +1,18 @@ +name: Copy Published Binaries Template + +runs: + using: "composite" + steps: + - name: Install latest version of the C++ extension + run: | + code --install-extension ms-vscode.cpptools --force --pre-release + + - name: Copy binaries from the C++ extension + run: | + cp -r ~/.vscode/extensions/ms-vscode.cpptools-1.*/bin/* bin + working-directory: Extension + shell: bash + + - name: Uninstall the C++ extension + run: | + code --uninstall-extension ms-vscode.cpptools From 52404a68859aae7a9e7f07b3058e7e652938bb92 Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Fri, 25 Apr 2025 16:19:35 -0700 Subject: [PATCH 06/14] try these paths instead --- .github/workflows/job-compile-and-test.yml | 2 +- .github/workflows/template-copy-published-binaries.yml | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/job-compile-and-test.yml b/.github/workflows/job-compile-and-test.yml index 9c8d82681..a4ee6c863 100644 --- a/.github/workflows/job-compile-and-test.yml +++ b/.github/workflows/job-compile-and-test.yml @@ -23,7 +23,7 @@ jobs: - name: Copy Published Extension Binaries if: ${{ inputs.platform != 'windows' }} - uses: .github/workflows/template-copy-published-binaries.yml + uses: ./.github/workflows/template-copy-published-binaries.yml - name: Use Node.js 22 uses: actions/setup-node@v4 diff --git a/.github/workflows/template-copy-published-binaries.yml b/.github/workflows/template-copy-published-binaries.yml index ceba4e25c..c076acd6f 100644 --- a/.github/workflows/template-copy-published-binaries.yml +++ b/.github/workflows/template-copy-published-binaries.yml @@ -9,8 +9,7 @@ runs: - name: Copy binaries from the C++ extension run: | - cp -r ~/.vscode/extensions/ms-vscode.cpptools-1.*/bin/* bin - working-directory: Extension + cp -r ~/.vscode/extensions/ms-vscode.cpptools-1.*/bin/* ./Extension/bin shell: bash - name: Uninstall the C++ extension From 6d0d78ad99b820f1025818410f926d5d68dca891 Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Fri, 25 Apr 2025 16:27:05 -0700 Subject: [PATCH 07/14] forget the template --- .github/workflows/job-compile-and-test.yml | 16 ++++++++++++++-- .../template-copy-published-binaries.yml | 17 ----------------- 2 files changed, 14 insertions(+), 19 deletions(-) delete mode 100644 .github/workflows/template-copy-published-binaries.yml diff --git a/.github/workflows/job-compile-and-test.yml b/.github/workflows/job-compile-and-test.yml index a4ee6c863..50e79b866 100644 --- a/.github/workflows/job-compile-and-test.yml +++ b/.github/workflows/job-compile-and-test.yml @@ -21,9 +21,21 @@ jobs: steps: - uses: actions/checkout@v4 - - name: Copy Published Extension Binaries + - name: Install latest version of the C++ extension if: ${{ inputs.platform != 'windows' }} - uses: ./.github/workflows/template-copy-published-binaries.yml + run: | + code --install-extension ms-vscode.cpptools --force --pre-release + + - name: Copy binaries from the C++ extension + if: ${{ inputs.platform != 'windows' }} + run: | + cp -r ~/.vscode/extensions/ms-vscode.cpptools-1.*/bin/* ./Extension/bin + shell: bash + + - name: Uninstall the C++ extension + if: ${{ inputs.platform != 'windows' }} + run: | + code --uninstall-extension ms-vscode.cpptools - name: Use Node.js 22 uses: actions/setup-node@v4 diff --git a/.github/workflows/template-copy-published-binaries.yml b/.github/workflows/template-copy-published-binaries.yml deleted file mode 100644 index c076acd6f..000000000 --- a/.github/workflows/template-copy-published-binaries.yml +++ /dev/null @@ -1,17 +0,0 @@ -name: Copy Published Binaries Template - -runs: - using: "composite" - steps: - - name: Install latest version of the C++ extension - run: | - code --install-extension ms-vscode.cpptools --force --pre-release - - - name: Copy binaries from the C++ extension - run: | - cp -r ~/.vscode/extensions/ms-vscode.cpptools-1.*/bin/* ./Extension/bin - shell: bash - - - name: Uninstall the C++ extension - run: | - code --uninstall-extension ms-vscode.cpptools From 7c185bafe685bfc5f610d1323bd9c929c9c1bd3d Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Sat, 26 Apr 2025 14:59:55 -0700 Subject: [PATCH 08/14] don't run these tests on linux/mac yet --- .github/workflows/job-compile-and-test.yml | 20 ++++--------------- Extension/.gitignore | 1 + .../tests/compilerPath.test.ts | 5 +++++ 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/.github/workflows/job-compile-and-test.yml b/.github/workflows/job-compile-and-test.yml index 50e79b866..2468e22a8 100644 --- a/.github/workflows/job-compile-and-test.yml +++ b/.github/workflows/job-compile-and-test.yml @@ -21,22 +21,6 @@ jobs: steps: - uses: actions/checkout@v4 - - name: Install latest version of the C++ extension - if: ${{ inputs.platform != 'windows' }} - run: | - code --install-extension ms-vscode.cpptools --force --pre-release - - - name: Copy binaries from the C++ extension - if: ${{ inputs.platform != 'windows' }} - run: | - cp -r ~/.vscode/extensions/ms-vscode.cpptools-1.*/bin/* ./Extension/bin - shell: bash - - - name: Uninstall the C++ extension - if: ${{ inputs.platform != 'windows' }} - run: | - code --uninstall-extension ms-vscode.cpptools - - name: Use Node.js 22 uses: actions/setup-node@v4 with: @@ -58,7 +42,11 @@ jobs: run: yarn test working-directory: Extension + # These tests don't require the binary. + # On Linux, it is failing (before the tests actually run) with: Test run terminated with signal SIGSEGV. + # But it works on Linux during the E2E test. - name: Run SingleRootProject tests + if: ${{ inputs.platform != 'linux' }} run: yarn test --scenario=SingleRootProject --skipCheckBinaries working-directory: Extension diff --git a/Extension/.gitignore b/Extension/.gitignore index 1adad30d0..1965b6766 100644 --- a/Extension/.gitignore +++ b/Extension/.gitignore @@ -10,6 +10,7 @@ server debugAdapters LLVM bin/cpptools* +bin/libc.so bin/*.dll bin/.vs bin/LICENSE.txt diff --git a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts index 00ce9f45e..555b8218c 100644 --- a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts +++ b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts @@ -5,6 +5,7 @@ import { describe, it } from 'mocha'; import { deepEqual, equal, ok } from 'node:assert'; +import { skip } from 'node:test'; import * as path from 'path'; import { Uri } from 'vscode'; import { extractCompilerPathAndArgs, setExtensionPath } from '../../../../src/common'; @@ -113,6 +114,8 @@ if (isWindows) { // The extension is not initialized the same way during tests, so this needs to be set manually // so the tests can find `cpptools-wordexp`. setExtensionPath(extensionPath); + skip(); + return; // [compilerPath, useLegacyBehavior, additionalArgs, result.compilerName, result.allCompilerArgs] const tests: [string, boolean, string[] | undefined, string, string[]][] = [ @@ -181,6 +184,8 @@ describe('validateCompilerPath', () => { // The extension is not initialized the same way during tests, so this needs to be set manually // so the tests can find `cpptools-wordexp`. setExtensionPath(extensionPath); + skip(); + return; } // [compilerPath, cwd, result.compilerName, result.allCompilerArgs, result.error, result.telemetry] From f1a20c5904708baaa82b650a97b48a300db5cd0e Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Sat, 26 Apr 2025 15:29:26 -0700 Subject: [PATCH 09/14] lint --- .../scenarios/SingleRootProject/tests/compilerPath.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts index 555b8218c..f6faac7c9 100644 --- a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts +++ b/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts @@ -114,7 +114,7 @@ if (isWindows) { // The extension is not initialized the same way during tests, so this needs to be set manually // so the tests can find `cpptools-wordexp`. setExtensionPath(extensionPath); - skip(); + void skip(); return; // [compilerPath, useLegacyBehavior, additionalArgs, result.compilerName, result.allCompilerArgs] @@ -184,7 +184,7 @@ describe('validateCompilerPath', () => { // The extension is not initialized the same way during tests, so this needs to be set manually // so the tests can find `cpptools-wordexp`. setExtensionPath(extensionPath); - skip(); + void skip(); return; } From 479ee91c6f5ec7a51286bf67cc7d937e6c6269f7 Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Mon, 28 Apr 2025 14:55:20 -0700 Subject: [PATCH 10/14] Move the test to a different folder which will run in E2E mode --- .../assets/b i n/clang++ | 0 .../assets/b i n/clang++.exe | 0 .../assets/bin/cl.exe | 0 .../assets/bin/clang-cl.exe | 0 .../assets/bin/gcc | 0 .../assets/bin/gcc.exe | 0 .../tests/compilerPath.test.ts | 17 +---------------- 7 files changed, 1 insertion(+), 16 deletions(-) rename Extension/test/scenarios/{SingleRootProject => SimpleCppProject}/assets/b i n/clang++ (100%) rename Extension/test/scenarios/{SingleRootProject => SimpleCppProject}/assets/b i n/clang++.exe (100%) rename Extension/test/scenarios/{SingleRootProject => SimpleCppProject}/assets/bin/cl.exe (100%) rename Extension/test/scenarios/{SingleRootProject => SimpleCppProject}/assets/bin/clang-cl.exe (100%) rename Extension/test/scenarios/{SingleRootProject => SimpleCppProject}/assets/bin/gcc (100%) rename Extension/test/scenarios/{SingleRootProject => SimpleCppProject}/assets/bin/gcc.exe (100%) rename Extension/test/scenarios/{SingleRootProject => SimpleCppProject}/tests/compilerPath.test.ts (96%) diff --git a/Extension/test/scenarios/SingleRootProject/assets/b i n/clang++ b/Extension/test/scenarios/SimpleCppProject/assets/b i n/clang++ similarity index 100% rename from Extension/test/scenarios/SingleRootProject/assets/b i n/clang++ rename to Extension/test/scenarios/SimpleCppProject/assets/b i n/clang++ diff --git a/Extension/test/scenarios/SingleRootProject/assets/b i n/clang++.exe b/Extension/test/scenarios/SimpleCppProject/assets/b i n/clang++.exe similarity index 100% rename from Extension/test/scenarios/SingleRootProject/assets/b i n/clang++.exe rename to Extension/test/scenarios/SimpleCppProject/assets/b i n/clang++.exe diff --git a/Extension/test/scenarios/SingleRootProject/assets/bin/cl.exe b/Extension/test/scenarios/SimpleCppProject/assets/bin/cl.exe similarity index 100% rename from Extension/test/scenarios/SingleRootProject/assets/bin/cl.exe rename to Extension/test/scenarios/SimpleCppProject/assets/bin/cl.exe diff --git a/Extension/test/scenarios/SingleRootProject/assets/bin/clang-cl.exe b/Extension/test/scenarios/SimpleCppProject/assets/bin/clang-cl.exe similarity index 100% rename from Extension/test/scenarios/SingleRootProject/assets/bin/clang-cl.exe rename to Extension/test/scenarios/SimpleCppProject/assets/bin/clang-cl.exe diff --git a/Extension/test/scenarios/SingleRootProject/assets/bin/gcc b/Extension/test/scenarios/SimpleCppProject/assets/bin/gcc similarity index 100% rename from Extension/test/scenarios/SingleRootProject/assets/bin/gcc rename to Extension/test/scenarios/SimpleCppProject/assets/bin/gcc diff --git a/Extension/test/scenarios/SingleRootProject/assets/bin/gcc.exe b/Extension/test/scenarios/SimpleCppProject/assets/bin/gcc.exe similarity index 100% rename from Extension/test/scenarios/SingleRootProject/assets/bin/gcc.exe rename to Extension/test/scenarios/SimpleCppProject/assets/bin/gcc.exe diff --git a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts b/Extension/test/scenarios/SimpleCppProject/tests/compilerPath.test.ts similarity index 96% rename from Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts rename to Extension/test/scenarios/SimpleCppProject/tests/compilerPath.test.ts index f6faac7c9..fdb66ab0a 100644 --- a/Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts +++ b/Extension/test/scenarios/SimpleCppProject/tests/compilerPath.test.ts @@ -5,10 +5,9 @@ import { describe, it } from 'mocha'; import { deepEqual, equal, ok } from 'node:assert'; -import { skip } from 'node:test'; import * as path from 'path'; import { Uri } from 'vscode'; -import { extractCompilerPathAndArgs, setExtensionPath } from '../../../../src/common'; +import { extractCompilerPathAndArgs } from '../../../../src/common'; import { isWindows } from '../../../../src/constants'; import { CppProperties } from '../../../../src/LanguageServer/configurations'; @@ -111,12 +110,6 @@ if (isWindows) { }); } else { describe('extractCompilerPathAndArgs', () => { - // The extension is not initialized the same way during tests, so this needs to be set manually - // so the tests can find `cpptools-wordexp`. - setExtensionPath(extensionPath); - void skip(); - return; - // [compilerPath, useLegacyBehavior, additionalArgs, result.compilerName, result.allCompilerArgs] const tests: [string, boolean, string[] | undefined, string, string[]][] = [ ['clang', false, undefined, 'clang', []], @@ -180,14 +173,6 @@ if (isWindows) { } describe('validateCompilerPath', () => { - if (!isWindows) { - // The extension is not initialized the same way during tests, so this needs to be set manually - // so the tests can find `cpptools-wordexp`. - setExtensionPath(extensionPath); - void skip(); - return; - } - // [compilerPath, cwd, result.compilerName, result.allCompilerArgs, result.error, result.telemetry] const tests: [string, Uri, string, string[]][] = [ ['cl.exe', assetsFolder, 'cl.exe', []], From 77ff126b3d93b72a038f271894a66ecb9540efae Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Mon, 28 Apr 2025 14:59:47 -0700 Subject: [PATCH 11/14] Unused variable after merge --- .../test/scenarios/SimpleCppProject/tests/compilerPath.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/Extension/test/scenarios/SimpleCppProject/tests/compilerPath.test.ts b/Extension/test/scenarios/SimpleCppProject/tests/compilerPath.test.ts index fdb66ab0a..ac1cb053b 100644 --- a/Extension/test/scenarios/SimpleCppProject/tests/compilerPath.test.ts +++ b/Extension/test/scenarios/SimpleCppProject/tests/compilerPath.test.ts @@ -13,7 +13,6 @@ import { CppProperties } from '../../../../src/LanguageServer/configurations'; const assetsFolder = Uri.file(path.normalize(path.join(__dirname.replace(/dist[\/\\]/, ''), '..', 'assets'))); const assetsFolderFsPath = assetsFolder.fsPath; -const extensionPath = path.normalize(path.join(__dirname.replace(/dist[\/\\]/, ''), '..', '..', '..', '..')); // A simple test counter for the tests that loop over several cases. // This is to make it easier to see which test failed in the output. From d3d2316033f871d84175eb3cc484c8b656b7f963 Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Mon, 28 Apr 2025 15:37:23 -0700 Subject: [PATCH 12/14] pre-review --- Extension/c_cpp_properties.schema.json | 7 ++----- Extension/src/LanguageServer/configurations.ts | 15 ++++++++------- Extension/src/common.ts | 10 ++++++++-- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/Extension/c_cpp_properties.schema.json b/Extension/c_cpp_properties.schema.json index 939cb8a34..9ed052fcd 100644 --- a/Extension/c_cpp_properties.schema.json +++ b/Extension/c_cpp_properties.schema.json @@ -18,10 +18,7 @@ "compilerPath": { "markdownDescription": "Full path of the compiler being used, e.g. `/usr/bin/gcc`, to enable more accurate IntelliSense.", "descriptionHint": "Markdown text between `` should not be translated or localized (they represent literal text) and the capitalization, spacing, and punctuation (including the ``) should not be altered.", - "type": [ - "string", - "null" - ] + "type": "string" }, "compilerArgs": { "markdownDescription": "Compiler arguments to modify the includes or defines used, e.g. `-nostdinc++`, `-m32`, etc. Arguments that take additional space-delimited arguments should be entered as separate arguments in the array, e.g. for `--sysroot ` use `\"--sysroot\", \"\"`.", @@ -312,4 +309,4 @@ "version" ], "additionalProperties": false -} +} \ No newline at end of file diff --git a/Extension/src/LanguageServer/configurations.ts b/Extension/src/LanguageServer/configurations.ts index 49cc1e4bf..b2fff05be 100644 --- a/Extension/src/LanguageServer/configurations.ts +++ b/Extension/src/LanguageServer/configurations.ts @@ -1599,7 +1599,8 @@ export class CppProperties { * Get the compilerPath and args from a compilerPath string that has already passed through * `this.resolvePath`. If there are errors processing the path, those will also be returned. * - * @resolvedCompilerPath a compilerPath string that has already been resolved. + * @param resolvedCompilerPath a compilerPath string that has already been resolved. + * @param rootUri the workspace folder URI, if any. */ public static validateCompilerPath(resolvedCompilerPath: string, rootUri?: vscode.Uri): util.CompilerPathAndArgs { if (!resolvedCompilerPath) { @@ -1629,16 +1630,16 @@ export class CppProperties { resolvedCompilerPath = pathLocation; compilerPathAndArgs.compilerPath = pathLocation; } else if (rootUri) { - // Check again for a relative path. - const relativePath: string = rootUri.fsPath + path.sep + resolvedCompilerPath; - if (!fs.existsSync(relativePath)) { - if (existsWithExeAdded(relativePath)) { - resolvedCompilerPath = relativePath + ".exe"; + // Test if it was a relative path. + const absolutePath: string = rootUri.fsPath + path.sep + resolvedCompilerPath; + if (!fs.existsSync(absolutePath)) { + if (existsWithExeAdded(absolutePath)) { + resolvedCompilerPath = absolutePath + ".exe"; } else { pathExists = false; } } else { - resolvedCompilerPath = relativePath; + resolvedCompilerPath = absolutePath; } } } diff --git a/Extension/src/common.ts b/Extension/src/common.ts index 23fa2dafb..a48326eae 100644 --- a/Extension/src/common.ts +++ b/Extension/src/common.ts @@ -1102,7 +1102,13 @@ export interface CompilerPathAndArgs { } /** - * Parse the compiler path input into a compiler path and compiler args. If there are no args, the compilerPath will have been verified to exist. + * Parse the compiler path input into a compiler path and compiler args. If there are no args in the input string, this function will have + * verified that the compiler exists. (e.g. `compilerArgsFromCommandLineInPath` will be empty) + * + * @param useLegacyBehavior - If true, use the legacy behavior of separating the compilerPath from the args. + * @param inputCompilerPath - The compiler path input from the user. + * @param compilerArgs - The compiler args input from the user. + * @param cwd - The directory used to resolve relative paths. */ export function extractCompilerPathAndArgs(useLegacyBehavior: boolean, inputCompilerPath?: string, compilerArgs?: string[], cwd?: string): CompilerPathAndArgs { let compilerPath: string | undefined = inputCompilerPath; @@ -1137,7 +1143,7 @@ export function extractCompilerPathAndArgs(useLegacyBehavior: boolean, inputComp } } else { if (compilerPath.includes(' ')) { - // There is no leading quote, so we'll treat it as a command line. + // There is no leading quote, but there is a space so we'll treat it as a command line. const potentialArgs: string[] = useLegacyBehavior ? legacyExtractArgs(compilerPath) : extractArgs(compilerPath); compilerPath = trimLegacyQuotes(potentialArgs.shift()); compilerArgsFromCommandLineInPath = potentialArgs; From 3526478fb47a7f120c9183ed86d6996c2970d6b1 Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Wed, 30 Apr 2025 11:01:11 -0700 Subject: [PATCH 13/14] Address PR feedback --- Extension/c_cpp_properties.schema.json | 5 ++++- Extension/src/LanguageServer/configurations.ts | 15 +++++++++++---- .../SimpleCppProject/tests/compilerPath.test.ts | 2 +- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/Extension/c_cpp_properties.schema.json b/Extension/c_cpp_properties.schema.json index 9ed052fcd..21f80af76 100644 --- a/Extension/c_cpp_properties.schema.json +++ b/Extension/c_cpp_properties.schema.json @@ -18,7 +18,10 @@ "compilerPath": { "markdownDescription": "Full path of the compiler being used, e.g. `/usr/bin/gcc`, to enable more accurate IntelliSense.", "descriptionHint": "Markdown text between `` should not be translated or localized (they represent literal text) and the capitalization, spacing, and punctuation (including the ``) should not be altered.", - "type": "string" + "type": [ + "string", + "null" + ] }, "compilerArgs": { "markdownDescription": "Compiler arguments to modify the includes or defines used, e.g. `-nostdinc++`, `-m32`, etc. Arguments that take additional space-delimited arguments should be entered as separate arguments in the array, e.g. for `--sysroot ` use `\"--sysroot\", \"\"`.", diff --git a/Extension/src/LanguageServer/configurations.ts b/Extension/src/LanguageServer/configurations.ts index b2fff05be..406ae4369 100644 --- a/Extension/src/LanguageServer/configurations.ts +++ b/Extension/src/LanguageServer/configurations.ts @@ -63,7 +63,7 @@ export interface ConfigurationJson { export interface Configuration { name: string; compilerPathInCppPropertiesJson?: string | null; - compilerPath?: string; + compilerPath?: string; // Can be set to null based on the schema, but it will be fixed in parsePropertiesFile. compilerPathIsExplicit?: boolean; compilerArgs?: string[]; compilerArgsLegacy?: string[]; @@ -1443,10 +1443,17 @@ export class CppProperties { } } - // Configuration.compileCommands is allowed to be defined as a string in the schema, but we send an array to the language server. - // For having a predictable behavior, we convert it here to an array of strings. + // Special sanitization of the newly parsed configuration file happens here: for (let i: number = 0; i < newJson.configurations.length; i++) { + // Configuration.compileCommands is allowed to be defined as a string in the schema, but we send an array to the language server. + // For having a predictable behavior, we convert it here to an array of strings. newJson.configurations[i].compileCommands = this.forceCompileCommandsAsArray(newJson.configurations[i].compileCommands); + + // `compilerPath` is allowed to be set to null in the schema so that empty string is not the default value (which has another meaning). + // If we detect this, we treat it as undefined. + if (newJson.configurations[i].compilerPath === null) { + delete newJson.configurations[i].compilerPath; + } } this.configurationJson = newJson; @@ -1647,7 +1654,7 @@ export class CppProperties { const compilerPathErrors: string[] = []; if (compilerPathMayNeedQuotes && !pathExists) { - compilerPathErrors.push(localize("path.with.spaces", 'Compiler path with spaces and arguments is missing double quotes " around the path.')); + compilerPathErrors.push(localize("path.with.spaces", 'Compiler path with spaces could not be found. If this was intended to include compiler arguments, surround the compiler path with double quotes (").')); telemetry.CompilerPathMissingQuotes = 1; } diff --git a/Extension/test/scenarios/SimpleCppProject/tests/compilerPath.test.ts b/Extension/test/scenarios/SimpleCppProject/tests/compilerPath.test.ts index ac1cb053b..c4ee7a633 100644 --- a/Extension/test/scenarios/SimpleCppProject/tests/compilerPath.test.ts +++ b/Extension/test/scenarios/SimpleCppProject/tests/compilerPath.test.ts @@ -263,7 +263,7 @@ describe('validateCompilerPath', () => { equal(result.compilerName, 'icc', 'compilerName should be found'); deepEqual(result.allCompilerArgs, ['-O2'], 'args should match'); ok(result.error?.includes('Cannot find'), 'Should have an error for unknown compiler'); - ok(result.error?.includes('missing double quotes'), 'Should have an error for missing double quotes'); + ok(result.error?.includes('surround the compiler path with double quotes'), 'Should have an error for missing double quotes'); equal(result.telemetry?.PathNonExistent, 1, 'Should have telemetry for relative paths'); equal(result.telemetry?.PathNotAFile, undefined, 'Should not have telemetry for invalid paths'); equal(result.telemetry?.CompilerPathMissingQuotes, 1, 'Should have telemetry for missing quotes'); From 71eb0bdc71e5de88657da45c37db15c06fe57408 Mon Sep 17 00:00:00 2001 From: Bob Brown Date: Wed, 30 Apr 2025 11:14:59 -0700 Subject: [PATCH 14/14] adjust auto-complete for compilerPath in c_cpp_properties.json --- Extension/c_cpp_properties.schema.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Extension/c_cpp_properties.schema.json b/Extension/c_cpp_properties.schema.json index 21f80af76..b3ac9a6d4 100644 --- a/Extension/c_cpp_properties.schema.json +++ b/Extension/c_cpp_properties.schema.json @@ -19,8 +19,8 @@ "markdownDescription": "Full path of the compiler being used, e.g. `/usr/bin/gcc`, to enable more accurate IntelliSense.", "descriptionHint": "Markdown text between `` should not be translated or localized (they represent literal text) and the capitalization, spacing, and punctuation (including the ``) should not be altered.", "type": [ - "string", - "null" + "null", + "string" ] }, "compilerArgs": {