-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Changes from all commits
96c64c9
e3797bd
9412739
9fb66ac
ae9b88d
1dc03a7
5b28559
1eabe77
0b3c932
892987c
04fc19c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,6 +130,7 @@ export class CppProperties { | |
private defaultWindowsSdkVersion: string | null = null; | ||
private vcpkgIncludes: string[] = []; | ||
private vcpkgPathReady: boolean = false; | ||
private nodeAddonIncludes: string[] = []; | ||
private defaultIntelliSenseMode?: string; | ||
private defaultCustomConfigurationVariables?: { [key: string]: string }; | ||
private readonly configurationGlobPattern: string = "c_cpp_properties.json"; | ||
|
@@ -155,6 +156,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)); | ||
} | ||
|
||
|
@@ -317,6 +319,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"] : []; | ||
|
@@ -386,6 +389,72 @@ 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)); | ||
|
||
if (!error) { | ||
try { | ||
const pathToNode: string = which.sync("node"); | ||
const nodeAddonMap: { [dependency: string]: string } = { | ||
"nan": `"${pathToNode}" --no-warnings -e "require('nan')"`, | ||
"node-addon-api": `"${pathToNode}" --no-warnings -p "require('node-addon-api').include"` | ||
}; | ||
|
||
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) { | ||
if (pdjFound) { | ||
// only log an error if package.json exists. | ||
console.log('readNodeAddonIncludeLocations', error.message); | ||
} | ||
} else { | ||
this.handleConfigurationChange(); | ||
} | ||
} | ||
|
||
private getConfigIndexForPlatform(config: any): number | undefined { | ||
if (!this.configurationJson) { | ||
return undefined; | ||
|
@@ -627,6 +696,12 @@ export class CppProperties { | |
const configuration: Configuration = this.configurationJson.configurations[i]; | ||
|
||
configuration.includePath = this.updateConfigurationStringArray(configuration.includePath, settings.defaultIncludePath, env); | ||
// in case includePath is reset below | ||
const origIncludePath: string[] | undefined = configuration.includePath; | ||
if (settings.addNodeAddonIncludePaths) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
should check the unmodified/original variable and then do something like
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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[] = origIncludePath || []; | ||
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); | ||
|
@@ -663,8 +738,9 @@ export class CppProperties { | |
if (!configuration.windowsSdkVersion && !!this.defaultWindowsSdkVersion) { | ||
configuration.windowsSdkVersion = this.defaultWindowsSdkVersion; | ||
} | ||
if (!configuration.includePath && !!this.defaultIncludes) { | ||
configuration.includePath = this.defaultIncludes; | ||
if (!origIncludePath && !!this.defaultIncludes) { | ||
const includePath: string[] = configuration.includePath || []; | ||
configuration.includePath = includePath.concat(this.defaultIncludes); | ||
} | ||
if (!configuration.macFrameworkPath && !!this.defaultFrameworks) { | ||
configuration.macFrameworkPath = this.defaultFrameworks; | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
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.