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.

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

implement addNodeAddonIncludePaths #6731

Merged
merged 11 commits into from
Jan 14, 2021
8 changes: 7 additions & 1 deletion Extension/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1102,6 +1102,12 @@
"markdownDescription": "%c_cpp.configuration.vcpkg.enabled.markdownDescription%",
"scope": "resource"
},
"C_Cpp.addNodeAddonIncludePaths": {
"type": "boolean",
"default": false,
"markdownDescription": "%c_cpp.configuration.addNodeAddonIncludePaths.description%",
"scope": "application"
},
"C_Cpp.renameRequiresIdentifier": {
"type": "boolean",
"default": true,
Expand Down Expand Up @@ -2679,4 +2685,4 @@
"integrity": "CF1A01AA75275F76800F6BC1D289F2066DCEBCD983376D344ABF6B03FDB8FEA0"
}
]
}
}
1 change: 1 addition & 0 deletions Extension/package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@
"c_cpp.configuration.enhancedColorization.description": "If enabled, code is colorized based on IntelliSense. This setting only applies if intelliSenseEngine is set to \"Default\".",
"c_cpp.configuration.codeFolding.description": "If enabled, code folding ranges are provided by the language server.",
"c_cpp.configuration.vcpkg.enabled.markdownDescription": "Enable integration services for the [vcpkg dependency manager](https://aka.ms/vcpkg/).",
"c_cpp.configuration.addNodeAddonIncludePaths.description": "Add include paths from nan and node-addon-api when they're dependencies.",
"c_cpp.configuration.renameRequiresIdentifier.description": "If true, 'Rename Symbol' will require a valid C/C++ identifier.",
"c_cpp.configuration.debugger.useBacktickCommandSubstitution.description": "If true, debugger shell command substitution will use obsolete backtick (`).",
"c_cpp.contributes.views.cppReferencesView.title": "C/C++: Other references results",
Expand Down
5 changes: 5 additions & 0 deletions Extension/src/LanguageServer/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1367,6 +1367,11 @@ export class DefaultClient implements Client {
this.semanticTokensProvider = undefined;
}
}
// if addNodeAddonIncludePaths was turned on but no includes have been found yet then 1) presume that nan
// or node-addon-api was installed so prompt for reload.
if (changedSettings["addNodeAddonIncludePaths"] && settings.addNodeAddonIncludePaths && this.configuration.nodeAddonIncludesFound() === 0) {
util.promptForReloadWindowDueToSettingsChange();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will only get shown if a user adds the nan/node-addon-api to their package.json and then enables the setting -- is that intentional? It seems fine to me, but it just covers a subset of the cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

prompts for reload if addNodeAddonIncludePaths is turned on but no addon includes were found by readNodeAddonIncludeLocations. this doesn't catch the case where nan was found but they added node-addon-api before turning on the setting.

above is my original comment on this. it's definitely a subset but seemed like one that is likely to be hit with a typical workflow - create a project, start editing, add node-addon-api or nan, notice the squiggles, and then fix it.

if your experience is different i'm happy to take it out. but it's kind of the best that could be done without creating a file-watcher or changing readNodeAddonIncludeLocations to be synchronous.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, seems fine to me.

}
}
this.configuration.onDidChangeSettings();
telemetry.logLanguageServerEvent("CppSettingsChange", changedSettings, undefined);
Expand Down
75 changes: 74 additions & 1 deletion Extension/src/LanguageServer/configurations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ export class CppProperties {
private defaultWindowsSdkVersion: string | null = null;
private vcpkgIncludes: string[] = [];
private vcpkgPathReady: boolean = false;
private nodeAddonIncludes: string[] = [];
private nodeAddonIncludePathsReady: boolean = false;
private defaultIntelliSenseMode?: string;
private defaultCustomConfigurationVariables?: { [key: string]: string };
private readonly configurationGlobPattern: string = "c_cpp_properties.json";
Expand All @@ -155,6 +157,7 @@ export class CppProperties {
this.configFolder = path.join(rootPath, ".vscode");
this.diagnosticCollection = vscode.languages.createDiagnosticCollection(rootPath);
this.buildVcpkgIncludePath();
this.readNodeAddonIncludeLocations(rootPath);
this.disposables.push(vscode.Disposable.from(this.configurationsChanged, this.selectionChanged, this.compileCommandsChanged));
}

Expand Down Expand Up @@ -288,7 +291,8 @@ export class CppProperties {
}

private applyDefaultIncludePathsAndFrameworks(): void {
if (this.configurationIncomplete && this.defaultIncludes && this.defaultFrameworks && this.vcpkgPathReady) {
if (this.configurationIncomplete && this.defaultIncludes && this.defaultFrameworks && this.vcpkgPathReady
&& this.nodeAddonIncludePathsReady) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This variable is no longer needed. It was only needed when the applyDefaultConfigurationValues was being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, obvious when i see it. you have a much better handle on all the moving pieces than i do - thanks for spending time on this.

const configuration: Configuration | undefined = this.CurrentConfiguration;
if (configuration) {
this.applyDefaultConfigurationValues(configuration);
Expand Down Expand Up @@ -317,6 +321,7 @@ export class CppProperties {
} else {
configuration.includePath = [defaultFolder];
}

// browse.path is not set by default anymore. When it is not set, the includePath will be used instead.
if (isUnset(settings.defaultDefines)) {
configuration.defines = (process.platform === 'win32') ? ["_DEBUG", "UNICODE", "_UNICODE"] : [];
Expand Down Expand Up @@ -386,6 +391,70 @@ export class CppProperties {
}
}

public nodeAddonIncludesFound(): number {
return this.nodeAddonIncludes.length;
}

private async readNodeAddonIncludeLocations(rootPath: string): Promise<void> {
let error: Error | undefined;
let pdjFound: boolean = false;
const package_json: any = await fs.promises.readFile(path.join(rootPath, "package.json"), "utf8")
.then(pdj => {pdjFound = true; return JSON.parse(pdj); })
.catch(e => (error = e));

const pathToNode: string = which.sync("node");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be inside the try { below (avoids calling which.sync when it's not needed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point.

const nodeAddonMap: { [dependency: string]: string } = {
"nan": `${pathToNode} --no-warnings -e "require('nan')"`,
bmacnaughton marked this conversation as resolved.
Show resolved Hide resolved
"node-addon-api": `${pathToNode} --no-warnings -p "require('node-addon-api').include"`
};

if (!error) {
try {
for (const dep in nodeAddonMap) {
if (dep in package_json.dependencies) {
const execCmd: string = nodeAddonMap[dep];
let stdout: string = await util.execChildProcess(execCmd, rootPath);
bmacnaughton marked this conversation as resolved.
Show resolved Hide resolved
if (!stdout) {
continue;
}

// cleanup newlines
if (stdout[stdout.length - 1] === "\n") {
stdout = stdout.slice(0, -1);
}
// node-addon-api returns a quoted string, e.g., '"/home/user/dir/node_modules/node-addon-api"'.
if (stdout[0] === "\"" && stdout[stdout.length - 1] === "\"") {
stdout = stdout.slice(1, -1);
}

// at this time both node-addon-api and nan return their own directory so this test is not really
// needed. but it does future proof the code.
if (!await util.checkDirectoryExists(stdout)) {
// nan returns a path relative to rootPath causing the previous check to fail because this code
// is executing in vscode's working directory.
stdout = path.join(rootPath, stdout);
if (!await util.checkDirectoryExists(stdout)) {
error = new Error(`${dep} directory ${stdout} doesn't exist`);
stdout = '';
}
}
if (stdout) {
this.nodeAddonIncludes.push(stdout);
}
}
}
} catch (e) {
error = e;
}
}
if (error && pdjFound) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be something like

if (error) {
if (pdjFound) {
...
}
} else {
this.handleConfigurationChange();
}

i.e. to skip this.handleConfigurationChange when it's not needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or checking this.nodeAddonIncludes.length > 0 (i.e. in case one of the cases errors?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it is (conceivably) possible that both node-addon-api and nan are referenced in package.json, both requires succeed, but one of two util.checkDirectoryExists() fail.

but i am changing it as i think it's 1) not possible now given what node-addon-api and nan return and 2) a really tiny edge case going forward because it means the package has to return an invalid directory.

thank you.

// only log an error if package.json exists.
console.log('readNodeAddonIncludeLocations', error.message);
}
this.nodeAddonIncludePathsReady = true;
this.handleConfigurationChange();
}

private getConfigIndexForPlatform(config: any): number | undefined {
if (!this.configurationJson) {
return undefined;
Expand Down Expand Up @@ -627,6 +696,10 @@ export class CppProperties {
const configuration: Configuration = this.configurationJson.configurations[i];

configuration.includePath = this.updateConfigurationStringArray(configuration.includePath, settings.defaultIncludePath, env);
if (settings.addNodeAddonIncludePaths) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

configuration.includePath is checked/modified in the code below under certain settings. The "unmodified" configuration.includePath should be saved to some variable here to be used later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if (!configuration.includePath && !!this.defaultIncludes) {
                            configuration.includePath = this.defaultIncludes;
                        }

should check the unmodified/original variable and then do something like

                const includePath: string[] = configuration.includePath || [];
                configuration.includePath = includePath.concat(this.defaultIncludes);

Copy link
Contributor Author

@bmacnaughton bmacnaughton Jan 13, 2021

Choose a reason for hiding this comment

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

thanks for this; i feel like a dog watching tv when i work in this codebase. everything is new and my focus is on a very tiny slice of what is going on.

const includePath: string[] = configuration.includePath || [];
configuration.includePath = includePath.concat(this.nodeAddonIncludes.filter(i => includePath.indexOf(i) < 0));
}
configuration.defines = this.updateConfigurationStringArray(configuration.defines, settings.defaultDefines, env);
configuration.macFrameworkPath = this.updateConfigurationStringArray(configuration.macFrameworkPath, settings.defaultMacFrameworkPath, env);
configuration.windowsSdkVersion = this.updateConfigurationString(configuration.windowsSdkVersion, settings.defaultWindowsSdkVersion, env);
Expand Down
1 change: 1 addition & 0 deletions Extension/src/LanguageServer/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ export class CppSettings extends Settings {
public get preferredPathSeparator(): string | undefined { return super.Section.get<string>("preferredPathSeparator"); }
public get updateChannel(): string | undefined { return super.Section.get<string>("updateChannel"); }
public get vcpkgEnabled(): boolean | undefined { return super.Section.get<boolean>("vcpkg.enabled"); }
public get addNodeAddonIncludePaths(): boolean | undefined { return super.Section.get<boolean>("addNodeAddonIncludePaths"); }
public get renameRequiresIdentifier(): boolean | undefined { return super.Section.get<boolean>("renameRequiresIdentifier"); }
public get defaultIncludePath(): string[] | undefined { return super.Section.get<string[]>("default.includePath"); }
public get defaultDefines(): string[] | undefined { return super.Section.get<string[]>("default.defines"); }
Expand Down