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

change node-addon-api build to cmake #422

Closed
DaAitch opened this issue Dec 29, 2018 · 14 comments
Closed

change node-addon-api build to cmake #422

DaAitch opened this issue Dec 29, 2018 · 14 comments

Comments

@DaAitch
Copy link
Contributor

DaAitch commented Dec 29, 2018

This is not a request to change everything in one day, but a possible improvement/POC of the build.
What are your feelings about a cmake setup?

improvements:

  • get rid of node-gyp one day (like me, what I've read community is not very happy with it)
  • no config-only rigid builds
  • cmake features
    • build is code
    • cross-os compilation
    • actively maintained
    • standard build tool for C/C++ (support ninja/Makefile)

disadvantages:

  • maybe cannot drop node-gyp support for the first time

Other improvements are:

  • I could simplify how tests are compiled/executed: *.test.cc files are compiled, *.test.js files are executed, so simply add files
  • logging/documentation: I love to read code as documentation, so that I can understand how sth. works. With node-gyp it's like "understand node-gyp configuration or die", and with cmake it's easy to understand what happens, maybe not for a beginner to write, but to also to read and understand
DaAitch added a commit to DaAitch/node-addon-api that referenced this issue Dec 29, 2018
DaAitch added a commit to DaAitch/node-addon-api that referenced this issue Dec 29, 2018
@gabrielschulhof
Copy link
Contributor

@DaAitch we need to ensure that CMake is available on all the machines of https://ci.nodejs.org/job/node-test-node-addon-api/. I'm totally in favour of switching to CMake though.

@DaAitch
Copy link
Contributor Author

DaAitch commented Dec 29, 2018

@gabrielschulhof: cool, so that should not be a big thing, right :-) ?

@gabrielschulhof
Copy link
Contributor

Well, it's a modification to the CI infrastructure. We need to see if a switch to CMake is already in work on https://github.com/nodejs/build/.

@DaAitch
Copy link
Contributor Author

DaAitch commented Dec 30, 2018

what I could found on nodejs and builds is:

@mhdawson has been involved to some issues. maybe he can help us here?

@mhdawson
Copy link
Member

mhdawson commented Jan 2, 2019

There has been some exploratory work to look at using cmake with node core, but its not gotten to the point where there is consensus or a view on when it might/might not happen.

I think it is interesting it exploring using cmake instead. Is it possible that we could support both at the same time?

@DaAitch
Copy link
Contributor Author

DaAitch commented Jan 2, 2019

@mhdawson I did some refactoring of the build, but in general if I hadn't remove the gyp files, we could support both the same time. No change of the sources, so: yes.

@mhdawson
Copy link
Member

mhdawson commented Jan 2, 2019

Can you share what it looks like with cmake support added? Would be good to look at the changes to better understand.

@DaAitch
Copy link
Contributor Author

DaAitch commented Jan 2, 2019

sure, it's already attached above: 98cc0c3

@mhdawson
Copy link
Member

mhdawson commented Jan 2, 2019

Opened nodejs/TSC#648 to rekindle the discussion for using cmake in node core.

@DaAitch
Copy link
Contributor Author

DaAitch commented Jan 3, 2019

@mhdawson would a PR makes sense, that touches nothing, but adds an experimental CMakeLists.txt?
Maybe we can run cmake build on CI as well?

The commit I did was a mix of many small things I found (maybe I will create a PR for them as well).

@mhdawson
Copy link
Member

mhdawson commented Jan 4, 2019

I'm not sure yet. It would be good to have a more complete picture on the suitability of cmake for building addons in general. A distinct PR might make it easier to see the changes that it introduces but I'm not sure we'd be ready to land it.

@DaAitch
Copy link
Contributor Author

DaAitch commented Jan 4, 2019

@mhdawson okay, so we can retrigger this after nodejs/TSC#648. I'll close this.

@DaAitch DaAitch closed this as completed Jan 4, 2019
@refack
Copy link

refack commented Jan 20, 2019

FYI: there is https://github.com/cmake-js/cmake-js

@bkotzz
Copy link

bkotzz commented Oct 23, 2019

Can we revive this at all? Would anyone be interested in a PR that simply adds a CMakeList for the library, and a CI job that makes sure it builds, but we keep using gyp for now?

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

No branches or pull requests

5 participants