Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle multiple compile commands on client side (needs native server side support) #12960

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Handle multiple compile commands on client side (needs native server side support) #12960

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f346723
accept an array in compile commands schema
Nov 10, 2024
2dc0fa3
handle settings.json of multiple compile commands
yiftahw Nov 10, 2024
02a2812
update schemas
yiftahw Nov 10, 2024
280aee1
an array is returned from default settings only if non-empty with non…
yiftahw Nov 10, 2024
b934a92
basic handling of compile commands array in configurations
yiftahw Nov 11, 2024
e273731
Merge branch 'main' into handle-multiple-compile-commands
Nov 13, 2024
0c8ccb1
fixes after merge
Nov 13, 2024
8e84183
yarn lint
yiftahw Nov 14, 2024
22de1e0
unify check behavior
yiftahw Nov 14, 2024
62a8915
update desription in schema
yiftahw Nov 14, 2024
d7e2a4a
handle parsed values from c_cpp_properties.json
yiftahw Nov 14, 2024
b1614de
reverse logic to default as undefined
yiftahw Nov 14, 2024
7fe9904
squiggles for multi paths
yiftahw Nov 14, 2024
82c4e53
remove not needed optional guard
yiftahw Nov 14, 2024
69c4013
squiggles finalized
yiftahw Nov 14, 2024
d2bf51d
paths should be separated telemetry enabled for a single string
yiftahw Nov 14, 2024
6aa66f6
yarn lint
yiftahw Nov 14, 2024
45bb041
update UI to handle multiple compile commands files
yiftahw Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions Extension/c_cpp_properties.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,22 @@
]
},
"compileCommands": {
"markdownDescription": "Full path to `compile_commands.json` file for the workspace.",
"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"
"oneOf": [
{
"type": "string",
"default": ""
bobbrow marked this conversation as resolved.
Show resolved Hide resolved
},
{
"type": "array",
"items": {
"type": "string"
},
"uniqueItems": true,
"default": []
}
],
"markdownDescription": "Full path or a list of full paths to `compile_commands.json` files for the workspace.",
"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."
},
"includePath": {
"markdownDescription": "A list of paths for the IntelliSense engine to use while searching for included headers. Searching on these paths is not recursive. Specify `**` to indicate recursive search. For example, `${workspaceFolder}/**` will search through all subdirectories while `${workspaceFolder}` will not. Usually, this should not include system includes; instead, set `C_Cpp.default.compilerPath`.",
Expand Down
15 changes: 14 additions & 1 deletion Extension/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,20 @@
"scope": "machine-overridable"
},
"C_Cpp.default.compileCommands": {
"type": "string",
"oneOf": [
{
"type": "string",
"default": ""
},
{
"type": "array",
"items": {
"type": "string"
},
"uniqueItems": true,
"default": []
}
],
"markdownDescription": "%c_cpp.configuration.default.compileCommands.markdownDescription%",
"scope": "machine-overridable"
},
Expand Down
97 changes: 60 additions & 37 deletions Extension/src/LanguageServer/configurations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ export interface Configuration {
defines?: string[];
intelliSenseMode?: string;
intelliSenseModeIsExplicit?: boolean;
compileCommandsInCppPropertiesJson?: string;
compileCommands?: string;
compileCommandsInCppPropertiesJson?: string[];
compileCommands?: string[];
forcedInclude?: string[];
configurationProviderInCppPropertiesJson?: string;
configurationProvider?: string;
Expand Down Expand Up @@ -136,7 +136,7 @@ export class CppProperties {
private currentConfigurationIndex: PersistentFolderState<number> | undefined;
private configFileWatcher: vscode.FileSystemWatcher | null = null;
private configFileWatcherFallbackTime: Date = new Date(); // Used when file watching fails.
private compileCommandsFile: vscode.Uri | undefined | null = undefined;
private compileCommandsFiles: Set<string> = new Set();
private compileCommandsFileWatchers: fs.FSWatcher[] = [];
private compileCommandsFileWatcherFallbackTime: Map<string, Date> = new Map<string, Date>(); // Used when file watching fails.
private defaultCompilerPath: string | null = null;
Expand Down Expand Up @@ -389,7 +389,8 @@ export class CppProperties {
configuration.windowsSdkVersion = this.defaultWindowsSdkVersion;
}
if (isUnset(settings.defaultCompilerPath) && this.defaultCompilerPath &&
(isUnset(settings.defaultCompileCommands) || settings.defaultCompileCommands === "") && !configuration.compileCommands) {
(isUnset(settings.defaultCompileCommands) || settings.defaultCompileCommands?.length === 0) &&
(isUnset(configuration.compileCommands) || configuration.compileCommands?.length === 0)) {
// compile_commands.json already specifies a compiler. compilerPath overrides the compile_commands.json compiler so
// don't set a default when compileCommands is in use.

Expand Down Expand Up @@ -684,7 +685,7 @@ export class CppProperties {
this.parsePropertiesFile(); // Clear out any modifications we may have made internally.
const config: Configuration | undefined = this.CurrentConfiguration;
if (config) {
config.compileCommands = path;
config.compileCommands = [path];
Copy link
Member

@bobbrow bobbrow Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for my team. Do we want to prefer storing the compileCommands property as a string in the actual c_cpp_properties.json file, or as an array? Either way is acceptable here because the json will be reparsed in handleConfigurationChange and converted back to an array.

One other thing to note is that we should change the Edit Configurations UI to make the input for compileCommands be a textarea instead, along with whatever associated changes should come with that. The answer to the question above would also guide the implementation for the storage of the data in the json file. (e.g. if we prefer string for the single entry case, we'd need to test for that when it comes via the UI and react accordingly)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick suggestion for your first question, might be an overkill. Instead of forcing the value to be a string array both in parsePropertiesFile() and in handleSquiggles(), we create a "version 5" of the properties and add a function "updateToVersion5()" which actually saves a single entry as an array in the json file.
Again, this might be overkill, just a suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have considered making changes to the version number, but this gets disruptive because many people commit their configurations to source control. We received a lot of negative feedback in the past when version changes were more frequent so we avoid it as best we can.

We are technically breaking versioning and assuming some risk by not incrementing the version number with this change, but the addition is very subtle. We will need to discuss this as a team and decide if this is acceptable or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bobbrow Can't it be a string or an array in the c_cpp_properties.json?

this.writeToJson();
}
// Any time parsePropertiesFile is called, configurationJson gets
Expand Down Expand Up @@ -938,7 +939,7 @@ export class CppProperties {
configuration.macFrameworkPath = this.updateConfigurationStringArray(configuration.macFrameworkPath, settings.defaultMacFrameworkPath, env);
configuration.windowsSdkVersion = this.updateConfigurationString(configuration.windowsSdkVersion, settings.defaultWindowsSdkVersion, env);
configuration.forcedInclude = this.updateConfigurationPathsArray(configuration.forcedInclude, settings.defaultForcedInclude, env, false);
configuration.compileCommands = this.updateConfigurationString(configuration.compileCommands, settings.defaultCompileCommands, env);
configuration.compileCommands = this.updateConfigurationStringArray(configuration.compileCommands, settings.defaultCompileCommands, env);
configuration.compilerArgs = this.updateConfigurationStringArray(configuration.compilerArgs, settings.defaultCompilerArgs, env);
configuration.cStandard = this.updateConfigurationString(configuration.cStandard, settings.defaultCStandard, env);
configuration.cppStandard = this.updateConfigurationString(configuration.cppStandard, settings.defaultCppStandard, env);
Expand Down Expand Up @@ -1092,11 +1093,13 @@ export class CppProperties {
}

if (configuration.compileCommands) {
configuration.compileCommands = this.resolvePath(configuration.compileCommands);
if (!this.compileCommandsFileWatcherFallbackTime.has(configuration.compileCommands)) {
// Start tracking the fallback time for a new path.
this.compileCommandsFileWatcherFallbackTime.set(configuration.compileCommands, new Date());
}
configuration.compileCommands = configuration.compileCommands?.map((path: string) => this.resolvePath(path));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: I don't believe you need to use ?. here or on the line below because the existence of configuration.compileCommands was tested before entering this code block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

configuration.compileCommands?.forEach((path: string) => {
if (!this.compileCommandsFileWatcherFallbackTime.has(path)) {
// Start tracking the fallback time for a new path.
this.compileCommandsFileWatcherFallbackTime.set(path, new Date());
}
});
}

if (configuration.forcedInclude) {
Expand All @@ -1120,10 +1123,12 @@ export class CppProperties {
// Instead, we clear entries that are no longer relevant.
const trackedCompileCommandsPaths: Set<string> = new Set();
this.configurationJson?.configurations.forEach((config: Configuration) => {
const path = this.resolvePath(config.compileCommands);
if (path.length > 0) {
trackedCompileCommandsPaths.add(path);
}
config.compileCommands?.forEach((path: string) => {
const compileCommandsFile = this.resolvePath(path);
if (compileCommandsFile.length > 0) {
trackedCompileCommandsPaths.add(compileCommandsFile);
}
});
});

for (const path of this.compileCommandsFileWatcherFallbackTime.keys()) {
Expand All @@ -1144,12 +1149,12 @@ export class CppProperties {
this.compileCommandsFileWatchers = []; // reset it
const filePaths: Set<string> = new Set<string>();
this.configurationJson.configurations.forEach(c => {
if (c.compileCommands) {
const fileSystemCompileCommandsPath: string = this.resolvePath(c.compileCommands);
if (fs.existsSync(fileSystemCompileCommandsPath)) {
filePaths.add(fileSystemCompileCommandsPath);
c.compileCommands?.forEach((path: string) => {
const compileCommandsFile: string = this.resolvePath(path);
if (fs.existsSync(compileCommandsFile)) {
filePaths.add(compileCommandsFile);
}
}
});
});
try {
filePaths.forEach((path: string) => {
Expand Down Expand Up @@ -1441,6 +1446,22 @@ 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 behvaior, we convert it here to an array of strings.
for (let i: number = 0; i < newJson.configurations.length; i++) {
let compileCommandsInCppPropertiesJson: string | string[] | undefined = <any>newJson.configurations[i].compileCommands;
newJson.configurations[i].compileCommands = undefined;
if (util.isString(compileCommandsInCppPropertiesJson) && compileCommandsInCppPropertiesJson.length > 0) {
newJson.configurations[i].compileCommands = [compileCommandsInCppPropertiesJson];
} else if (util.isArrayOfString(compileCommandsInCppPropertiesJson)) {
compileCommandsInCppPropertiesJson = compileCommandsInCppPropertiesJson.filter((value: string) => value.length > 0);
if (compileCommandsInCppPropertiesJson.length > 0) {
newJson.configurations[i].compileCommands = compileCommandsInCppPropertiesJson;
}
}
}

this.configurationJson = newJson;
if (this.CurrentConfigurationIndex < 0 || this.CurrentConfigurationIndex >= newJson.configurations.length) {
const index: number | undefined = this.getConfigIndexForPlatform(newJson);
Expand Down Expand Up @@ -2325,27 +2346,29 @@ export class CppProperties {

public checkCompileCommands(): void {
// Check for changes in case of file watcher failure.
const compileCommands: string | undefined = this.CurrentConfiguration?.compileCommands;
const compileCommands: string[] | undefined = this.CurrentConfiguration?.compileCommands;
if (!compileCommands) {
return;
}
const compileCommandsFile: string | undefined = this.resolvePath(compileCommands);
fs.stat(compileCommandsFile, (err, stats) => {
if (err) {
if (err.code === "ENOENT" && this.compileCommandsFile) {
this.compileCommandsFileWatchers.forEach((watcher: fs.FSWatcher) => watcher.close());
this.compileCommandsFileWatchers = []; // reset file watchers
this.onCompileCommandsChanged(compileCommandsFile);
this.compileCommandsFile = null; // File deleted
}
} else {
const compileCommandsLastChanged: Date | undefined = this.compileCommandsFileWatcherFallbackTime.get(compileCommandsFile);
if (compileCommandsLastChanged !== undefined && stats.mtime > compileCommandsLastChanged) {
this.compileCommandsFileWatcherFallbackTime.set(compileCommandsFile, new Date());
this.onCompileCommandsChanged(compileCommandsFile);
this.compileCommandsFile = vscode.Uri.file(compileCommandsFile); // File created.
compileCommands.forEach((path: string) => {
const compileCommandsFile: string | undefined = this.resolvePath(path);
fs.stat(compileCommandsFile, (err, stats) => {
if (err) {
if (err.code === "ENOENT" && this.compileCommandsFiles.has(compileCommandsFile)) {
this.compileCommandsFileWatchers.forEach((watcher: fs.FSWatcher) => watcher.close());
this.compileCommandsFileWatchers = []; // reset file watchers
this.onCompileCommandsChanged(compileCommandsFile);
this.compileCommandsFiles.delete(compileCommandsFile); // File deleted
}
} else {
const compileCommandsLastChanged: Date | undefined = this.compileCommandsFileWatcherFallbackTime.get(compileCommandsFile);
if (compileCommandsLastChanged !== undefined && stats.mtime > compileCommandsLastChanged) {
this.compileCommandsFileWatcherFallbackTime.set(compileCommandsFile, new Date());
this.onCompileCommandsChanged(compileCommandsFile);
this.compileCommandsFiles.add(compileCommandsFile); // File created.
}
}
}
});
});
}

Expand Down
21 changes: 19 additions & 2 deletions Extension/src/LanguageServer/settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,9 +399,26 @@ export class CppSettings extends Settings {
public get defaultDotconfig(): string | undefined { return changeBlankStringToUndefined(this.getAsStringOrUndefined("default.dotConfig")); }
public get defaultMacFrameworkPath(): string[] | undefined { return this.getArrayOfStringsWithUndefinedDefault("default.macFrameworkPath"); }
public get defaultWindowsSdkVersion(): string | undefined { return changeBlankStringToUndefined(this.getAsStringOrUndefined("default.windowsSdkVersion")); }
public get defaultCompileCommands(): string | undefined { return changeBlankStringToUndefined(this.getAsStringOrUndefined("default.compileCommands")); }
public get defaultForcedInclude(): string[] | undefined { return this.getArrayOfStringsWithUndefinedDefault("default.forcedInclude"); }
public get defaultIntelliSenseMode(): string | undefined { return this.getAsStringOrUndefined("default.intelliSenseMode"); }
public get defaultCompileCommands(): string[] | undefined {
// Try to get the value as a string.
const value: string | undefined = this.getAsStringOrUndefined("default.compileCommands");
if (value !== undefined) {
if (changeBlankStringToUndefined(value) === undefined) {
return undefined;
}
return [value];
}

// value is not a string, try to get it as an array of strings instead.
let valueArray: string[] | undefined = this.getAsArrayOfStringsOrUndefined("default.compileCommands");
valueArray = valueArray?.filter((value: string) => value.length > 0);
if (valueArray?.length === 0) {
return undefined;
}
return valueArray;
}
public get defaultCompilerPath(): string | null { return this.getAsString("default.compilerPath", true); }

public set defaultCompilerPath(value: string) {
Expand Down Expand Up @@ -638,7 +655,7 @@ export class CppSettings extends Settings {
}

if (isArrayOfString(value)) {
if (setting.items.enum && !allowUndefinedEnums) {
if (setting.items?.enum !== undefined && !allowUndefinedEnums) {
if (!value.every(x => this.isValidEnum(setting.items.enum, x))) {
return setting.default;
}
Expand Down