-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
src: add support for locally installed headers #2964
Conversation
Some linux distros allow headers to be installed through tools like rpm. If the runtime sets process.config.variables.use_prefix_to_find_headers, look for matching headers based on the directory set for the prefix in process.config.variables.prefix Signed-off-by: Michael Dawson <[email protected]>
Not sure if this should be added to doc, if so just let me know and I'll look for the right place. |
Upstream PR in Node.js to allow setting use_prefix_to_find_headers - nodejs/node#51525 |
Note that the addition to test/test-configure-python.js was needed so that the tests would run ok when applied to other streams, and does not adversely affect testing in head |
From the test failure on windows, I do see the test is too linux specific will take a look. |
Signed-off-by: Michael Dawson <[email protected]>
Guess it was more than just the path, spinning up a windows machine to investigate on. |
Signed-off-by: Michael Dawson <[email protected]>
Ok, windows all fixed up :) |
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.
LGTM! One minor suggestion to only expose readFileSync
.
Checklist
npm install && npm run lint && npm test
passesDescription of change
Some linux distros allow headers to be installed through tools like rpm. If the runtime sets
process.config.variables.use_prefix_to_find_headers, look for matching headers based on the directory set for the prefix in process.config.variables.prefix