-
Notifications
You must be signed in to change notification settings - Fork 465
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
gyp: add common targets #1389
gyp: add common targets #1389
Conversation
index.js
Outdated
@@ -5,7 +5,7 @@ const includeDir = path.relative('.', __dirname); | |||
module.exports = { | |||
include: `"${__dirname}"`, // deprecated, can be removed as part of 4.0.0 | |||
include_dir: includeDir, | |||
gyp: path.join(includeDir, 'node_api.gyp:nothing'), | |||
gyp: path.join(includeDir, 'node_addon_api.gyp'), |
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.
Is it all that harmful to leave this as-is and introduce a new key? If there's even a remote chance that it'll break folks, it may be simplest to not reuse this, but introduce a new key, and document what it does.
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.
Couldn't folks simply write the following?
...
'dependencies': [
"<!(node -p \"require('node-addon-api').include_dir\")/node_addon_api.gyp:node-addon-api-except"
]
...
Then we would need only document that node_addon_api.gyp is the way to add node-addon-api to one's binding.gyp.
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.
It is unnecessary, but having a single gyp entry point would be good. And using the include_dir
works -- nothing is wrong. It is simpler to have a gyp file short name, like
'dependencies': [
"<!(node -p \"require('node-addon-api').include_dir\")/node_addon_api.gyp:node-addon-api-except"
]
// versus...
'dependencies': [
"<!(node -p \"require('node-addon-api').gyp\"):node-addon-api-except"
]
Would require('node-addon-api').targets
sound good to you? For instance,
'dependencies': [
"<!(node -p \"require('node-addon-api').targets\"):node-addon-api-except"
]
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.
I've updated the PR to name the entrypoint as require('node-addon-api').targets
.
1bbdeea
to
bd2d790
Compare
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
Fixes: #1379
As documented at https://gyp.gsrc.io/docs/InputFormatReference.md#processing-order, a dependency's setting is processed after merging the root target_defaults, and variable expansion is allowed in dependency specifiers. This allows addons to include node-addon-api and common configs with a single line of gyp config. For example, when the addon needs c++ exception feature, they can simply declare the dependency on
node-addon-api-except
:No need to copy a paragraph of configs into addons' binding.gyp like documented at https://github.com/nodejs/node-addon-api/blob/9aaf3b1/doc/setup.md#installation-and-usage anymore.