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

gyp: add common targets #1389

Merged
merged 1 commit into from
Oct 24, 2023
Merged

gyp: add common targets #1389

merged 1 commit into from
Oct 24, 2023

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Oct 9, 2023

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:

{
  "targets": [
    {
      "target_name": "my_node_addon",
      "sources": [
        "src/my_node_addon.cc"
      ],
      'dependencies': [
        "<!(node -p \"require('node-addon-api').targets\"):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.

  • update the document
  • add a test

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'),
Copy link
Contributor

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.

Copy link
Contributor

@gabrielschulhof gabrielschulhof Oct 18, 2023

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.

Copy link
Member Author

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"
]

Copy link
Member Author

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.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson merged commit ab14347 into nodejs:main Oct 24, 2023
26 checks passed
@legendecas legendecas deleted the gyp-targets branch October 25, 2023 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling C++ exceptions on Windows is hell
3 participants