-
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 10 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,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"; | ||
|
@@ -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)); | ||
} | ||
|
||
|
@@ -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) { | ||
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. This variable is no longer needed. It was only needed when the applyDefaultConfigurationValues was being used. 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. 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); | ||
|
@@ -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"] : []; | ||
|
@@ -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"); | ||
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. This should be inside the 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. good point. |
||
const nodeAddonMap: { [dependency: string]: string } = { | ||
"nan": `"${pathToNode}" --no-warnings -e "require('nan')"`, | ||
"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) { | ||
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. I think this should be something like
i.e. to skip this.handleConfigurationChange when it's not needed. 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. Or checking this.nodeAddonIncludes.length > 0 (i.e. in case one of the cases errors?). 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. 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; | ||
|
@@ -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) { | ||
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[] = 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); | ||
|
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.