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

why the src/node_api.h and node_api_types.h head files were moved to the node repository #855

Closed
oshunter opened this issue Nov 27, 2020 · 25 comments
Labels

Comments

@oshunter
Copy link

The N-API is a good common abstract for native addons and it's common used on some other projects.
Such as the:
https://github.com/jerryscript-project/iotjs/tree/master/include
https://github.com/yodaos-project/ShadowNode/tree/master/include

And the latest Release 3.x.x,the N-API head files are removed to node repository src folder.
Is it possible to keep the head files on the node-addon-api repository for N-API part for other project use and as part of js/C/C++ addon mechanism and more JS engine were supported(V8/QuickJS/JerryScript)?

@oshunter oshunter changed the title why the src/node_api.h and node_api_types.h head files were moved to the nodejs project why the src/node_api.h and node_api_types.h head files were moved to the node repository Nov 27, 2020
@NickNaso
Copy link
Member

NickNaso commented Nov 27, 2020

Hi @oshunter,
N-API was exported from the core of Node.js like C API from its first release (Node v8.0.0). We maintained an alternative release in this repository to allow the usege of N-API with the previous version of Node.js. When Node 8.0.0 arrived at his end-of-life we provide to remove the N-API implementation from the node-addon-api because we support only all active LTS version of Node.js.
For now you could download the headers like others build tools already do (node-gyp,cmake-js). They take the headers downloading them from https://nodejs.org/dist/v{NODEJS-VERSION}/node-v{NODEJS-VERSION}-headers.tar.gz
I like your idea to distribute the head files because it avoid you to implement the automation that allow you to get them.
I want to discuss about this in the next N-API meeting that will happen today if you want to partecipate you are welcome.

Node.js Calendar

The link zoom to partecipate: https://zoom.us/j/363665824

@mhdawson
Copy link
Member

As a note for the discussion. I think it would be a challenge if we wanted to include experimental features as it is possible that each Node.js version has different experimental features available. A copy of the header file for each N-API version should be easier since that is fixed and should not change.

I'm not sure if the node-addon-api is the right repository for the headers, as @NickNaso mentioned it is already possible to get them from the headers.tar.gz but I can see that might not quite be what you want if you want headers for a specific N-API version.

@oshunter
Copy link
Author

Hi @NickNaso
Thanks for your proposal.

Hi @mhdawson
You are right, and as my samples, the requirement is use the N-API part only and do not need all node project API head files.
As the website https://nodejs.org/api/n-api.html , take the N-API as part of engine abstraction specification standard for C/C++ export to JS, and implement and wrap to the other JS engines.
Is it possible to have a pure N-API release package(or repository)and other project can import it directly?

@mhdawson mhdawson transferred this issue from nodejs/node-addon-api Nov 30, 2020
@mhdawson
Copy link
Member

@oshunter as an FYI transferred to the abi-stable-node repo so that I could put on our agenda to discuss in the meeting this Friday. I can transfer back after that.

@mhdawson
Copy link
Member

We discussed this in the N-API team meeting today is this what you need:

  1. Be able to install the headers from npm

  2. Have the npm package contain the following files:
    js_native_api.h
    js_native_api_types.h
    node_api_types.h
    node_api.h

Which define the functions/types available through N-API

  1. Experimental functions may be documented in the files, but the #define should not be used to expose them as they might change/be different over time.

  2. There is one set of files and the user specifies the N-API version they want to be exposed by using the appropriate #define

  3. A new version would be published after each time a new version of N-API is defined (ex N-API 6, N-API 7 etc.)

@mhdawson mhdawson transferred this issue from nodejs/abi-stable-node Dec 11, 2020
@mhdawson
Copy link
Member

@oshunter let us know if what we describe is what you need.

@mhdawson
Copy link
Member

mhdawson commented Dec 11, 2020

Possible package name: node-api-headers, maybe use scope @nodejs/node-api-headers

@mjmacleod
Copy link

Thanks for linking me to this.

It would be useful, to me at least though I imagine some others as well, if this package could also contain a .def file (or similar list of symbols that can easily be transformed into a .def file) containing all exported symbols (as per #720) alongside the headers.

This allows for build servers, CI and so on to build an addon without actually having to e.g. have node.exe/electron.exe present to find the symbols from.

Its possible, with some trial/error/hacks to create such a file from the headers, but it would be ideal for one central repository to simply have such a file instead of every project in a similar situation to mine implementing their own build system hacks for this.

@mhdawson
Copy link
Member

May be a silly question but is a .def file cross platform/operating system or does each platform/os have its own equivalent?

@mjmacleod
Copy link

As far as I understand it .def is windows specific.

I'm not 100% sure what the linux/mac/bsd etc. equivalent is however on these platforms it is possible to ignore undefined symbols, so its possible to just do this and link without the executable.

This is of course not 100% ideal because it also ignores other actually undefined symbols in the library, however you do at least get a warning/error about this at runtime, so as long as you test its nothing but a minor headache.
I imagine theres some way to pass ld a list of symbols to ignore but I'm not sure what it is...

Anyway, in short a .def is windows specific but similar files for other platforms are not strictly necessary, it is at least possible to build for them without them, while windows builds (if e.g. cross compiling or compiling with msys) are not really possible without them.

@NickNaso
Copy link
Member

Hi everybody,
I had a similar experience trying to create a N-API binding for Go. On the build process at some point the linker will complain about undefined symbols.
On Unix system you could set some flags to ignore undefined symbols. In my use case it could be a good choice because N-API is the only C library that I use.

  • Linux: -Wl,-unresolved-symbols=ignore-all
  • macOS: -Wl,-undefined,dynamic_lookup

Anyway on Windows I don't have the possibility to set this kind of flags using gcc because they are not supported so what I did is to create a fake / stub N-API library with all the symbols that I needed.

From my experience an idea could be a package that provides:

  • N-API headers file.
  • CLI that help to print the symobls exported by N-API (you can use them on your own build process).
  • Maybe expose the API use by the CLI tool if necessary.

Here I want to share with you my first draft of N-API bindings for Go https://github.com/napi-bindings/go-napi-sys/blob/master/napisys/napisys.go

@mhdawson
Copy link
Member

From the discussion in the meeting it sounds like a file which just had the list of symbols would have covered the case that @NickNaso had and I think @mjmacleod said the same.

It should be easy to maintain as it would just be additions when there was a new version of N-API and we believe the list is the same across platforms.

It does mean that the consumers would need to build the specific file they need (for example windows .def file from the list but that seems to be ok.

@mhdawson
Copy link
Member

Next step is to request new repo nodejs/node-api-headers following the process (request though admin repo).

@mhdawson
Copy link
Member

@NickNaso is going to create the request in the admin repo.

@NickNaso
Copy link
Member

I create the issue now here the ref. nodejs/admin#576

@oshunter
Copy link
Author

@NickNaso
Hi, NickNaso
When the node-api-headers repo will be created?
And is this repo will have the release version related with the node release version?

@oshunter
Copy link
Author

@oshunter let us know if what we describe is what you need.

Yes, I think it's great to have a separated repo for the pure headers with release version.

@NickNaso
Copy link
Member

@NickNaso
Hi, NickNaso
When the node-api-headers repo will be created?
And is this repo will have the release version related with the node release version?

Hi @oshunter,
usually we need to wait at least 72h after the request for the creation of the repo hoping that tere is no objection.
In my opinion in this repo we need to be related only with the N-API version.

@oshunter oshunter reopened this Dec 22, 2020
@oshunter
Copy link
Author

@NickNaso
Hi, NickNaso
When the node-api-headers repo will be created?
And is this repo will have the release version related with the node release version?

Hi @oshunter,
usually we need to wait at least 72h after the request for the creation of the repo hoping that tere is no objection.
In my opinion in this repo we need to be related only with the N-API version.

Thanks!

@mhdawson
Copy link
Member

@oshunter to set expectations I think we want to do a good amount of review on the first iteration on this as it will be much harder to change later on. That means it may take us a little while, particularly since many people are on holiday over the next few weeks.

@NickNaso
Copy link
Member

Hi everybody,
I made a PR nodejs/node-api-headers#1 on node-api-headers repo trying to provide a first implementation. Please if you have time make a review and give me a feedback to go forward.

@github-actions
Copy link
Contributor

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

@github-actions github-actions bot added the stale label Apr 30, 2021
@mhdawson mhdawson removed the stale label Apr 30, 2021
@mhdawson
Copy link
Member

Removed stale as the PR above has not yet landed.

@github-actions
Copy link
Contributor

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

@mhdawson
Copy link
Member

nodejs/node-api-headers#1 landed so closing out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants