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

POC: src: add addon ABI declaration option #25539

Closed
wants to merge 3 commits into from

Conversation

gabrielschulhof
Copy link
Contributor

Add macro NODE_MODULE_DECLARE_ABI to give addon authors the option of
providing a list of versions for various parts of the ABI against which
to check the addon at load time.

Re: nodejs/TSC#651

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 16, 2019
@gabrielschulhof gabrielschulhof added the addons Issues and PRs related to native addons. label Jan 16, 2019
doc/api/addons.md Outdated Show resolved Hide resolved
Add macro `NODE_MODULE_DECLARE_ABI` to give addon authors the option of
providing a list of versions for various parts of the ABI against which
to check the addon at load time.

Re: nodejs/TSC#651
doc/api/addons.md Outdated Show resolved Hide resolved
doc/api/addons.md Outdated Show resolved Hide resolved
src/node_binding.cc Outdated Show resolved Hide resolved
@gabrielschulhof
Copy link
Contributor Author

@mcollina
Copy link
Member

what's the semversiness of this change? I think it should be semver-major.

doc/api/addons.md Outdated Show resolved Hide resolved
@addaleax
Copy link
Member

@mcollina I’m not quite sure about the use cases here/if it is worth the added complexity (because it most likely only changes later-occurring errors into earlier ones), but it seems semver-minor as it is an opt-in feature for addons?

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 19, 2019
@refack
Copy link
Contributor

refack commented Jan 19, 2019

but it seems semver-minor as it is an opt-in feature for addons?

The addition of the macro seems semver-minor to me as well. But is it possible that the refactoring of node.h has semver-major impact?

@gabrielschulhof
Copy link
Contributor Author

@refack #include <node.h> after this change will define exactly the same things as now, plus the NODE_MODULE_ABI_* macros and the NODE_MODULE_DECLARE_ABI macro. Unless copying of the node.h header into your own project is part of our semver covenant, this is semver-minor.

@gabrielschulhof
Copy link
Contributor Author

I'm thinking that, for NODE_ABI_ENGINE_VERSION we may want to structure the number as a bit field to accommodate both engine type (V8/Chakra) and engine version. Or should we split it into two variables?

@refack
Copy link
Contributor

refack commented Jan 22, 2019

Unless copying of the node.h header into your own project is part of our semver covenant, this is semver-minor.

I agree. Considering that, does this change have a benefit before the next major version bump? If not maybe it's worth waiting just to be extra cautious? If it does, consider my comments non-blocking.

@gabrielschulhof
Copy link
Contributor Author

@refack so far in node_api.h we've pretty much been copying large-ish parts of node.h because node.h is C++ and node_api.h is C:

If we don't start factoring the C++-agnostic things out of node.h the overlap between the macros in node.h and node_api.h will get even larger. For example, the NODE_MODULE_ABI_*_VERSION have to be in a C++-agnostic header, otherwise they'd need to be copied into node_api.h with NAPI_MODULE_ABI_*_VERSION.

Having said this, we might simply wait until the next major version to land this feature. I mean, it'd make sense that the next major version of Node.js now has the new, cool Addon ABI Declarations™ feature.

@fhinkel
Copy link
Member

fhinkel commented Oct 28, 2019

I'm cleaning out a few old PRs 🧹. I'm closing this due to inactivity. Please re-open if needed!

@fhinkel fhinkel closed this Oct 28, 2019
@gabrielschulhof gabrielschulhof deleted the abi-version branch January 28, 2021 00:14
@gabrielschulhof gabrielschulhof restored the abi-version branch January 28, 2021 05:40
@gabrielschulhof gabrielschulhof deleted the abi-version branch February 3, 2021 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants