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

'CMakeJS.cmake' API proposal - initial #326

Open
wants to merge 140 commits into
base: master
Choose a base branch
from

Conversation

nathanjhood
Copy link

@nathanjhood nathanjhood commented Jan 30, 2024

Related #310

Here is my CMakeJS.cmake API proposal (MIT), more or less "complete", in as much as I feel it is presentable and also (potentially) solves many issues raised in other discussions and tickets.

Constructed over on my demo project; just dropping it into a new 'share' directory right here. Could go at the project root, or anywhere really, as long as builders are informed so that they may append the location to their CMAKE_MODULE_PATH and then include(CMakeJS).

It doesn't interfere with any existing source code, since there currently isn't any other CMake in this codebase to interfere with.

The most simple example of how this works for end-users in it's current presentation (as per the provided docs, and pretty much as per CMake conventions):

# CMakeLists.txt

cmake_minimum_required (VERSION 3.15)

# path to CMakeJS.cmake
list (APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/node_modules/cmake-js/share")

# include the module
include (CMakeJS)

project (demo)

cmakejs_create_napi_addon (
  addon # NAME
  src/demo/addon.cpp # SOURCES
)

The above should build and run, as long as the consuming project has a corresponding package.json with cmake-js as a dependency:

// package.json
{
    "name": "@demo/addon",
    "dependencies": {
        "cmake-js": "^7.3.2" // a proposed future version which carries 'CMakeJS.cmake'
    },
    "scripts": {
        "install": "cmake-js install",
        "configure": "cmake-js configure"
        "build": "cmake-js build"
    // ...
}

This demo project gives a full example; only difference is the location of CMakeJS.cmake. My suggestion is that it should be shipped in the cmake-js package tree (as presented), so that end-users will always find it under ${PROJECT_SOURCE_DIR}/node_modules/cmake-js/share - or, wherever.

cmake-js' CLI could also add another flag to it's configure runs, making the API instantly available when running from cmake-js. The native CMake CLI arg would obviously be something like "-DCMAKE_MODULE_PATH:PATH=path/to/cmake-js/share", but I have no suggestions on how to implement that in the context of cmake-js. Currently, it isn't actually necessary to make any such changes anyway, as long as the user adds the module to their path manually, somehow (like in the example).

Those of us who are really familiar with CMake will usually expect that a module offers some targets. This one offers four interface targets (no compilation units of their own), with each one depending on the previous one, per the Addon API dependency chain. With this, builders can choose to manually link with a specific level of the whole dependency API chain (dev files, C API, C++ API, and CMakeJS API):

cmake-js::node-dev        // The NodeJS developer files
cmake-js::node-api        // The C Addon API
cmake-js::node-addon-api  // The C++ Addon API
cmake-js::cmake-js        // The full set of configured Addon API dependencies

Or, they may ignore these libs entirely and just call cmakejs_create_napi_addon() like we did above (which links with cmake-js::cmake-js, under the hood).

There are examples of the extended API's functionality in the demo project. There also tests and typings, corresponding to the built addon.

The provided documentation, as well as the demo project linked above, should hopefully provide further insights into some of the design considerations that have been carefully planned, tested, and thought about.

I mentioned on the linked discussion (v8 ideas) that there are some issues around namespacing clashes. There are several means of addressing that, some which could be worked into this design to handle the whole issue (I believe). I will add a further note that vendors usually reserve their namespaces in CMake target land as well, so this API should not aim to create either a cmake::js nor a node::api; but, cmake-js::node-api is likely going to be acceptable, per the proposed API.

I really look forward to getting some feedback from @Julusian and/or any other interested parties in how to make an even more powerful, elegant experience out of building with this API - or something like it - as a proposed part of cmake-js.

Thanks for reading!

Nathan J. Hood

@nathanjhood nathanjhood mentioned this pull request Jan 30, 2024
@Julusian
Copy link
Collaborator

Julusian commented Jan 30, 2024

Thanks for helping with this. I've been meaning to properly look over what you have done, its similar but probably better written than my attempt #325.
I'll read through this soon, expect a bunch of questions on things :)

Honestly, my cmake knowledge is not amazing. It has mostly been from figuring out a few existing projects and improving them, so I am probably lacking a lot of knowledge especially in advanced things and in conventions.

One key thing is that I don't think that this new cmake file should pay any attention to the CMAKE_JS_* variables. I intend to remove those in v8 as this script will need to be able to generate them another way. And having two routes to get the 'same' values is just going to result in extra complexity and bugs.
In theory this script could land in a minor version release, as it won't break anything as long as the old flow is also kept around.


My first question, what is the reason for doing:

list (APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/node_modules/cmake-js/share")
include (CMakeJS)

instead of the simpler: include("${PROJECT_SOURCE_DIR}/node_modules/cmake-js/CMakeJS.cmake")?

To me it feels nicer to have it as a single line, but perhaps there is a good reason I am not aware of to do it the other way?

@nathanjhood
Copy link
Author

nathanjhood commented Jan 30, 2024

Thanks for helping with this. I've been meaning to properly look over what you have done, its similar but probably better written than my attempt #325. I'll read through this soon, expect a bunch of questions on things :)
To me it feels nicer to have it as a single line, but perhaps there is a good reason I am not aware of to do it the other way?

Hey, thanks for getting back to me!

I'm more than happy to answer the bunch of things best I can. There will surely be things!

There is no real reason for the 'share' dir. I was just trying to offer a clean presentation to make it more enticing for you. One slight consideration is that, often with CMake modules, the mechanism might be split across more than one file: there is often a FindCMakeJS.cmake module which fetches all the executables and variables coupled with one's CMake module, but for now I've just done all of that in the one file. It's something that might come up later on.

Along with the above, I don't know if you might like to hold things like the documentation, and possibly some example/test project(s) alongside the module, all in one out-of-the-way dir. It's your prerogative, in the end.

End-users can append the module to their path, or just include it directly. At the end of the day, it will be up to them, no need to force anyone to do it a certain way.

Perhaps it might actually be more 'conventional' to place a CMakeLists.txt in the root of cmake-js, containing the contents of my proposed module. But since there is nothing to actually build (these INTERFACE lib don't produce a compiled output), it shouldn't really matter.

CMake has a little bit of an art to it; when it's right, it is really flexible. Otherwise, it's really brittle and seems like every little change breaks something. You have absolutely the right idea with the beginnings or your CMake module but there are "bits" to get right, and I saw a chance to give back here.

Keep the questions coming,

Best
Nath

share/CMakeJS.cmake Outdated Show resolved Hide resolved
endif()

# Resolve NodeJS development headers
# TODO: This code block is quite problematic, since:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I don't think this should be done. In my mind, if node_modules is missing that means that cmake-js and therefore this cmake file will not be available and so is a non-issue, or for safety it should simply fail here

Copy link
Author

Choose a reason for hiding this comment

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

Fair - if the logic holds up with the codeblock removed, better than adding complexity with the add_custom_target() idea.

I'll let you know if any issues do arise in my tests, but I see what you mean!

Copy link
Author

Choose a reason for hiding this comment

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

In deeper reflection (sorry not at my machine rn):

That codeblock is the bit that makes it all work without running from cmake-js.

Users can build everything with just normal CMake CLI commands - it works because of that hacky line. I obviously feel the solution needs a lot of work, but it does build without cmake-js!

Check my demo commands.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is ultimately going to come down to how you install cmake-js, its not necessary to be able to use plain cmake

If you install it globally on your machine, then this block might be useful but as you point out risks performing a build as part of the yarn install.

If you install it as a dependency of the project, then this block is not needed as this file has already been loaded from the node_modules so it must exist.

Ideally we should support both cases, so the question becomes how much to do automatically (in the first iteration).
I personally would be ok with simply failing saying that the directory doesnt exist. This is no different to other cmake projects which rely on system dependencies to be installed.

And if someone feels strongly opposed, then this could easily be added later on. Perhaps the package manager problem can be solved by reading it from a variable? I don't know how to solve it might trigger another build, my first thought was using an environment variable which would tell cmake-js to skip the build but that would cause issues if there was another cmake-js based dependency to be installed.


Side note, I have never been a fan of installing things like this globally. Wouldn't everyone end up having issues once different projects require different versions of whatever you installed globally?

Copy link
Author

Choose a reason for hiding this comment

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

Side note, I have never been a fan of installing things like this globally. Wouldn't everyone end up having issues once different projects require different versions of whatever you installed globally?

Globally installing with cmake-js "just works" for me, and usually fixes any missing headers etc that I've ever encountered. Gladly, it works perfectly in sync with nvm - so, my cmake-js libnode-dev filetree version is in sync with my runtime node version, without any effort from my side. It does what it does well, but one slight issue with this is that I don't necessarily want a '.cmakejs' dir in my home folder, containing multiple filetrees. Would be nicer if these could all be kept project-local, either in 'node_modules' or the build dir, or anywhere where we can easily just blow them away when closing down the project.

But yeah, nvm and cmake-js always worked well here!

Copy link
Collaborator

@Julusian Julusian Jan 31, 2024

Choose a reason for hiding this comment

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

It does what it does well, but one slight issue with this is that I don't necessarily want a '.cmakejs' dir in my home folder, containing multiple filetrees.

I believe that the .cmakejs folder contains a cache of the 'full' headers that have been downloaded, to avoid needing to constantly redownload them. If they are only cached inside of the project folder, then there will be a lot more cache misses. (when switching project, when doing a new clone, when cleaning out all uncommitted files)

But these days this is only done when not using node-api

share/CMakeJS.cmake Outdated Show resolved Hide resolved
# Generate the identifier for the resource library's namespace
set(ns_re "[a-zA-Z_][a-zA-Z0-9_]*")

if(NOT DEFINED ARG_NAMESPACE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this require the NODE_API_MODULE to be called inside a c++ namespace? Thats not something I've done before, so might need to be optional

Copy link
Author

Choose a reason for hiding this comment

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

I don't think NODE_API_MODULE actually cares about what namespace it is in. In fact, if you were to define it in a different namespace than the Addon functions, it might even fail to build unless the module specifier carries the namespaces too:

NODE_API_MODULE(addon, demo::addon::Init)

NAPI_CPP_CUSTOM_NAMESPACE is part of the Node Addon API (defined in napi.h). It is demonstrated in the source file attached. Implementing it is entirely up to the user (I personally never knew about it 'til recently).

One of these bug-bears about namespaces, is that your code and your CMake targets should have consistent namespaces. You might be compiling a class found in vendor::library::class, in which case the CMake target should also be vendor::library::class (usually done as an ALIAS to the class target).

I mostly tested within the defined namespaces and getting it right was tricky :)

Will try a test project with no namespace in the source code and see if I can break it with the NAMESPACE arg, and also vice-versa.

share/CMakeJS.cmake Outdated Show resolved Hide resolved
node-addon-api
cmake-js
)
export (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont have any idea what this and below is doing right now. Not things I have encountered before

Copy link
Author

Choose a reason for hiding this comment

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

Check the build directory 'share' dir.

It's all in the official topic.

That guide is very useful.

@nathanjhood
Copy link
Author

nathanjhood commented Jan 30, 2024

Loving this feedback, thanks for your time and thoughts so far @Julusian !

I'll step away for a minute so we don't keep cross posting.

Meanwhile, if you haven't done so before, do check out MS's vcpkg, the C++ package manager which is kind-of taking on an npm-like role in C++ world. You just clone it to HOME (or use it as a git submodule), and it contains a vcpkg.cmake file, which users pass in as -DCMAKE_TOOLCHAIN_FILE:/path/to/vcpkg/share/vcpkg.cmake. Users define a vcpkg.json at the project root and specify their CMake project's dependencies, and boom - when 'configure' runs, the user's build dir contains built binaries, libraries, headers, CMake files, and other stuff, which allows one to work in a much more NodeJS-like way at dev time. The whole thing is built on top of CMake, and indeed most of it is written in CMake files.

(DISCLAIMER: I "borrowed" some vcpkg ideas to work on adding the full MSYS2 toolchain to vcpkg... maybe I'll finish it one day)

The entire premise works because all of the packages in the vcpkg registry all contain a CMakeLists.txt which defines and exports it's targets. There actually already are some packages in there - unofficial-node-api and unofficial-node-addon-api, if memory serves correctly? - which do the whole node -p require('headers') and then make an INTERFACE unofficial-node-api::node-api-headers library and so forth.

Part of my proposal of this CMakeJS.cmake API is to actually resolve that same dependency workload using cmake-js, such that this new API could even be packaged up and shipped to the vcpkg registry and behave like those do - but, all in one place, and with all the Javascript side powering everything.

The potential of getting cmake-js into the CMake-sphere is pretty exciting. I keep saying there are things that need to be considered, and mostly I'm thinking about "how to ship cmake-js to vcpkg" :)

EDIT: It's also about allowing cmake-js users to ship their addon projects to vcpkg, if they wish. cmake-js has to do it's part, like probably exporting some interface libraries, for that to work for end-users.

Hope that may give some context to some of the suggestions I've made!

@nathanjhood
Copy link
Author

nathanjhood commented Jan 30, 2024

Those last two commits are just a quick vcpkg demo - as a demo you may add -DCMAKE_TOOLCHAIN_FILE:/path/to/vcpkg/scripts/buildsystems/vcpkg.cmake", run configure on the CMakeLists.txt (just a copy of the module with an added project(cmakejs) to kick off), and have a build dir with all the dev, C, and C++ headers, thanks to vcpkg. Entirely optional.

I suggest I remove this for the moment and hopefully revisit vcpkg much later, to keep a clear focus on the actual API proposal for now. This hopefully just gives a bit more insight into all my concerns about "convention" - it would pay off immensely in the long run to understand now, early on, what to work towards, IMO.

It's up to you how much of this do you feel is worth taking on, potentially bringing in an influx of C++/CMake-side visibility, etc. Those users would be able to just add cmake-js to their vcpkg.json dependencies, and they will just have all the headers in the build dir, plus all those INTERFACE libs and the cmakejs_create_napi_addon() available to build with, no fuss. It seems a lot, but we're already close to this functionality now. If it is any encouragement for you, then I can offer support also.

Over to you @Julusian , I'll be around!

@nathanjhood
Copy link
Author

A quick word on the CMake-side interface for how these types of modules might usually be implemented.

Below is a 'typical' end-user's CMake project, consuming the CMakeJS.cmake API, constructed according to the user guide on dependencies

cmake_minimum_required(VERSION 3.15)
project(MyAddonProject VERSION 1.0.0)

find_package(CMakeJS REQUIRED)
add_library(MyAddon SHARED addon.cpp)
target_link_libraries(MyAddon PRIVATE cmake-js::cmake-js)  # deps are resolved!
set_target_properties(MyAddon PROPERTIES PREFIX "" SUFFIX ".node")
  • usage of find_package() should probably be encouraged, the user can add the module to their path a few ways, per the link above.
  • one slight difference between this. and just include("/path/to/CMakeJS.cmake") is that with the former, the module is freely callable but is not executed until find_package() gets called on it. The latter will execute the module inline.

While I don't really feel a strong preference for any particular inclusion mechanism, I think it would be sensible to align with expected behaviours.

I will take a look at a few CMake modules in vcpkg (some examples I like are fmt, ableton-link, curl...) to get a generalized idea of any concepts missing or misplaced in the currently-proposed API design.

Perhaps a test or set of tests should be established, built around the boundaries of what you feel should be supported at this time, and what should not.

@Julusian
Copy link
Collaborator

Julusian commented Jan 31, 2024

Below is a 'typical' end-user's CMake project, consuming the CMakeJS.cmake API,

Won't some guidance need to given to cmake on where to find cmakejs still? From reading those docs it is looking at CMAKE_PREFIX_PATH and CMAKE_MODULE_PATH, and I don't know how cmake-js would end up in either of those paths.

In another pure c++ with cmake project, I never entirely got along with cmake using find_packages. I think this was mostly because of windows and 'fetching' dependencies on windows, but it never felt nice to me to be including a bunch of FindX.cmake files which needed occasional updates to handle new distro or lib versions. Might just be me though 🤷


Perhaps I should clarify that I am primarily thinking about libraries which will be published to npm. While prebuilds mean that in most cases compiling can be avoided, if it does end up being compiled I don't want users to have to think about anything (other than installing cmake on their system).
If not publishing to npm, then I think it more reasonable that there could be more for users to do (such as in this scenario expecting cmake-js to be on the path or something isnt unreasonable)

share/CMakeJS.cmake Outdated Show resolved Hide resolved
@Julusian
Copy link
Collaborator

Julusian commented Feb 5, 2024

I wrote most of this on saturday, and haven't looked over any commits/comments made since then, so some of it could be commenting on things that have been changed already.

Except one correction: I didnt put anything on the 'install' script at all.

By 'install' I mean the various cmake calls to install( and tbh Im not sure what else.

Our CMakeLists, our CTest, and our CPack are nothing but a demo This represents what "our" users can do with their addons. It's not trying to be "our" addon that we share with the world. Not our CTest, CPack... it's a demo, showing what others can do with our API. Nothing more!

I've been mostly ignoring the demo. So far I've only really read the CMakeJS.cmake and that is what I have been focussed on. This could mean I am missing some context for things. But it seems like a chunk of this file is setting things up for users to be able to use cpack or 'install' or whatever.

The benefit of making sure our 'install(targets)' works is not for cmake-js, but for it's users and their users.

Again, I think I am just missing examples I can run/inspect of how this might be used in practise. I have had a couple of npm libraries using cmake-js for years, and other than people not knowing what cmake is and that they might need to install it, everything else has worked. and there isn't anything to do with install( of whatever in cmake.

For example https://github.com/Julusian/node-jpeg-turbo/blob/main/CMakeLists.txt, where the cmake file is less than 50 lines long. I dont remember having any issues reported there which were caused by cmake-js or the naive cmake usage.
That said, there is code outside of cmake-js used to move the single binary per platform/target around on disk to make it npm friendly. (basically just copying the build/Release/my-lib.node somewhere)

Also keep in mind that nothing has changed for anybody who doesn't 'include(CMakeJS)' - nothing at all will be different for these people.

For now you are correct, but in cmake-js v8 this will be the only supported way to do things.

In which case I kindly ask you to find an experienced CMake dev whom you do trust and seek a verdict on the point of all this. Everything I've done is just standard CMake, direct from the manual.

This could well be the root of the issue of that I don't think I know anyone experience in cmake, nor many who write much c++. My c++ usage could almost be labelled as hobby usage given how little of it I do.
I do much more js, so my instinct would be to use js tooling to package/distribute an addon from cmake-js, as that is the ecosystem I know and the ecosystem that users of my library will know.

PS keep in mind that C++ CMake devs fully expect their dev files to be in the binary dir, not in node_modules or some arbitrary home folder. You need to understand why this is; Not that they prefer it, but their tools do. Addons are built in C++ with CMake, not in Javascript with node.

Sure, but the aim of this library/package is to help bridge c++ and js, so it should be expected to have some js-ification of some c++ concepts (eg, every type node-api uses). In particular, seeing as I (as a user of cmake-js) added node-addon-api to my package.json, it would then be installed into my node_modules (so far normal js things), it then seems a little odd to copy that to the build dir.
I wouldn't expect cmake to start copying headers from my /usr/include into the build dir either.

Will these copied headers be updated correctly when updating npm packages, followed by a re-run of cmake-js build?

Getting a this file is prefixed in the source directory. This is because of the most critical var in this entire API, which is CMAKE_JS_INC.

As I said somewhere earlier, those CMAKE_JS_ vars are going to be removed in v8 of cmake-js. So I would really prefer for this new flow to not rely on them now. Either cmake-js v8 will be released with just this new flow sometime, or it will land in v7.x as an opt-in new flow, with a temporary cmake-js2 binary. Then when v8 is released, the old flow will be removed and the binary renamed. The less this new flow depends on the old flow the better, as it gives confidence that it is correct doesn't rely on anything the old flow did.

But yes, I was going to comment the other day on how it seemed weird to be copying the includes like it does, until disabling it revealed that error. So while I don't particularly like that it does a copy, it has a purpose and is necessary so is fine.

Good to know that libnode-dev is not the critical header set our users need anyway - although I am still certain that the entire Napi Addon API won't work without these files being available somewhere.

No, you do not need them at all. cmake-js tries to detect if the project is node-api (there is no good and 100% reliable way to do this though, one of the points on my v8 list was to swap the default behaviour to assume node-api unless proven otherwise), and depending on what it concludes, it sets different paths into CMAKE_JS_INC

cmake-js/lib/cMake.js

Lines 227 to 258 in 3922fb8

async getCmakeJsIncludeString() {
let incPaths = []
if (!this.options.isNodeApi) {
// Include and lib:
if (this.dist.headerOnly) {
incPaths = [path.join(this.dist.internalPath, '/include/node')]
} else {
const nodeH = path.join(this.dist.internalPath, '/src')
const v8H = path.join(this.dist.internalPath, '/deps/v8/include')
const uvH = path.join(this.dist.internalPath, '/deps/uv/include')
incPaths = [nodeH, v8H, uvH]
}
// NAN
const nanH = await locateNAN(this.projectRoot)
if (nanH) {
incPaths.push(nanH)
}
} else {
// Base headers
const apiHeaders = require('node-api-headers')
incPaths.push(apiHeaders.include_dir)
// Node-api
const napiH = await locateNodeApi(this.projectRoot)
if (napiH) {
incPaths.push(napiH)
}
}
return incPaths.join(';')
}
. Unless you directly invoke cmake-js install, it won't even download the headers
if (!this.options.isNodeApi) {
await this.dist.ensureDownloaded()

They are needed for 'old style' native modules (not using node-api), but that is another different challenge to tackle. For those we should also be providing the nan library as a target. I am thinking of circling back to this later, once there is an initial version of this working and merged.

I dont do dev stuff in ${HOME}/ and I definitely don't keep multiple versions of C++ libs in there... but that is beyond the CMakeJS.cmake API remit)

Yeah, but yarn/npm does put a cache in there somewhere, so this is not entirely inconsistent ;)

The consumer must have no CMake burden to bear; it will have mostly been done by cmake-js, and the rest by the cmake-js user. No passing of CMake responsibilities to downsteam users.

I agree, and that is how it works today, other than needing cmake to be installed.

@Julusian
Copy link
Collaborator

Julusian commented Feb 5, 2024

Thinking about the globbing of headers, from what I managed to find its a problem mostly because it wont detect new files being added.
But is that a real concern for us? if node-api-headers add a new header in v1.99.0, when the direct user of this library does an update then with globbing it wouldnt pick up the new file. But if the files are explicitly listed, until I do a release of cmake-js and they update their cmake-js too, then it wont find the new file.
This could even manifest as their project wont build at all (not without some overriding of versions), as we have a dependency on some 1.x version of node-api-headers, so if that change happens in 1.99, npm/yarn may start installing that and result in a header they need to be missing or not be referenced.
This will probably happen more in projects which are 3+ levels deep in dependency trees too, as when doing the yarn add someones-cmake-lib will install the newest versions (which are compatible according to semver rules), less likely for the library maintainer who will already have dependencies installed.

In this scenario I am envisioning that they add a new header file which is added a dependency of an existing header file. (ie napi.h gets a new line #include "new-header.h")

Maybe there are other problems with globbing that google didnt reveal (the first few answers I found didnt explain why it was bad practise), but I think it is something worth thinking about.


I am also wondering about having cmakejs_setup_node_addon_api and similar instead of the proposed link-level parameter (and related bool properties).
Because if we consider non node-api projects (which are likely to be using nan instead), then there are a couple of dependency trees to consider

  1. cmake-js (every library should link this to get the node.exe delayhook behaviour for windows), this doesnt require any of the other libraries
  2. node-api & (optionally) node-addon-api
  3. node-dev & (optionally) nan

maybe these node-api and node-dev should depend on cmake-js? that way we can be sure that the users who don't know they need to link the delayhook will get it implicitly

a cmakejs_setup_node_addon_api function could call cmakejs_setup_node_api and then create the node-addon-api interface. and crucially, it could throw an error if the node-addon-api npm library is not found (when I last looked, that was lacking, instead it would result in a compile failure)

I feel like this would make it easier to understand what link targets are available, as only the ones you call the cmakejs_setup_* function for will exist. That is more work for consuming projects, but makes it explicit what it is looking for and allows us to fail if the target will be broken/incomplete


Perhaps it would make sense to move the root cmakelists and demo code to be a project under the tests with the others? That will help clarify that it is only for testing purposes of this project

Copy link
Collaborator

@Julusian Julusian left a comment

Choose a reason for hiding this comment

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

I've had a read through the tests/api and tests/demo projects.

Ignoring the tests/targets for now, as they will need to change when reworking --link-levels


#else // !__has_include(<napi.h>) || !BUILDING_NODE_EXTENSION
#warning "Warning: Cannot find '<napi.h>' - try running 'npm -g install cmake-js'..."
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I'm a fan of this pattern. But equally, it doesn't matter, these are example projects.

That should say to do npm install node-addon-api though, as that is where the header comes from

Copy link
Author

Choose a reason for hiding this comment

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

Hi! Sure, I really don't mind. I thought it may be useful, when newcomers pull an addon project off git and open it in their IDE, to not greet them with a page full of red intellisense warnings about stuff missing. C++ is garish enough as is! Of course it does not fix or break anything. The 'good' stuff is just to get some function examples lighting up for them, this is no biggie for me.

"dependencies": {
"cmake-js": "https://github.com/nathanjhood/cmake-js#cmakejs_cmake_api",
"node-addon-api": "^7.1.0",
"node-api-headers": "^1.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't technically needed, as this is already a dependency of cmake-js, but perhaps it should be moved so that projects which use cmake-js should also be providing it as a dependency.

Copy link
Author

Choose a reason for hiding this comment

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

Yea I haven't quite had the fortitude to remove these lines, for fear that it all breaks! But in theory I had thought as you say. If we can drop deps (visual complexity) and not break stuff, then even better!

// This small codeblock in your root-level index.js allows others to consume
// your addon as any other NodeJS module

const addon = require(`./build/lib/addon.node`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This path is different to before, which will be problematic. Most people use a library to do this loading, to ensure it is cross-platform and handles other variances. The most common one is https://www.npmjs.com/package/bindings which looks like it will not find this.
Other tooling for distributing prebuilds of libraries on npm will also likely not find these

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I dont like the bindings package a whole lot myself. I mean, it's well made sure, but I've found myself needing to overwrite it's implementation in some builds and that became such a huge headache that I turned to using custom bindings.

There is a custom bindings generator function in the API now; it works fantastic until it doesn't (experimental) - it's a good demo of doing 'configured' code generation with CMake, which I am sure might inspire more ideas in you. The trick is the @ONLY var, which means that the vars surrounded by @@ will get evaluated when the function runs, but the regular-looking CMake vars will remain as string content.

Do as you please of course; my focus is mostly the CMake.


console.log(addon.hello())

// If I swap my package.json dependency for
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps this demo should do both, to show this off as well as talk about it?

Copy link
Author

Choose a reason for hiding this comment

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

sure!

tests/api/hello_consumer/README.md Show resolved Hide resolved
tests/api/hello_with_curl/tests/hello/tests.cmake Outdated Show resolved Hide resolved
Comment on lines +16 to +19
namespace Napi
{
namespace NAPI_CPP_CUSTOM_NAMESPACE
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a good idea to do?
This looks to me like you are defining your methods inside the Napi::hello namespace, which is also where the Napi types reside. Meaning this could result in collisions if you try to use names they are already using.
I could be wrong, I didn't read their header that closely

Copy link
Author

@nathanjhood nathanjhood Feb 5, 2024

Choose a reason for hiding this comment

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

I copied it from napi.h

It's their definition, not mine, and this is what it is for. It isn't necessary but the header makes it's usage quite clear.

What I will say is: napi.h provides this definition, so we should offer a hook into it, IMO. Optional, of course, which it is within napi.h as well.

I actually believe from the notes in their header, if I'm not reading it badly, that they semi-suggest using the namespace precisely because stuff is already defined in there, so you will not be able to create a function named 'Object()', since that already exists and is very difficult to overload. I probably am wrong, but napi.h appears to be suggesting that this is what this optional var is for.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Annoyingly they have no documentation on this other than some comments in the header which dont explain how to use it.

But this part:
https://github.com/nodejs/node-addon-api/blob/ea712094e3cfd0ce973631683a1c3275e451fa32/napi.h#L163-L166

It reads to me as they are creating the symbols under a namespace, then they are reexporting the symbols under the Napi namespace. In other words, usage in user code is identical to if it isnt set.

I have tried this, and I didn't need to be inside any namespace or use any 'using' for Napi values to still work

Comment on lines +53 to +58
// Export your custom namespace to outside of the Napi namespace, providing an
// alias to the Napi Addon API; e.g., '<vendor>::<addon>::Object()', along with the
// functions defined above, such as '<vendor>::<addon>::Hello()'.
namespace NAPI_CPP_CUSTOM_NAMESPACE::CMAKEJS_ADDON_NAME {
using namespace Napi::NAPI_CPP_CUSTOM_NAMESPACE;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Im curious, what is this solving?

Copy link
Author

Choose a reason for hiding this comment

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

Now I have Nathan::Addon::Hello().

Let's say I put a second addon on my CMakeLists.txt, with it's own source file. Link the first one to the second one. Now, Nathan::Addon::Hello() is available in both source files.

I might even call that function in my second addon and pass one Napi::Env to another.

Is this possible? I really don't know, haven't tried yet, but I think context-aware addons probably can do this.

Again, it will not break my heart if this line of code was wiped. Just more "stuff folks can do".

tests/api/hello_with_testing_and_packing/CMakeLists.txt Outdated Show resolved Hide resolved
tests/api/hello_with_curl/CMakeLists.txt Show resolved Hide resolved
@nathanjhood
Copy link
Author

nathanjhood commented Feb 6, 2024

@Julusian one wider remark tied our overall convo and not a specific code change:

I believe we can make a decent compromise about "relocatable targets" and whether we should be moving stuff around or not.

The API was working just as nicely (as far as 'config/build') when we had our targets staying in one place. cmake-js has always done this; and not only do I see no reason to fix what isn't broken, I am also unnerved about breaking something for someone somewhere as a result of them expecting some behaviour which we changed drastically.

I believe lots of CMake projects carry a few universal-ish vars, things like BUILD_TESTS which tend to show up in many different projects as some sort of adopted semi-standard.

There is a var like this called something like <project>_INSTALL_TARGETS which is a boolean.

We can probably quite easily "walk back" our relocatability far enough to retro-fit an option/logic switch around it.

We dont want to define or rely on <project> at API level, it isn't correct. I'm scared of suggesting CMAKE_JS_INSTALL_TARGETS in case that var namespace disappears, per your suggestions. But I think the idea of making that relocatable step completely optionable is probably workable.

I have no qualms setting default to 'off' i.e., old behaviour, as long as it's in there somewhere. This default will do more to ensure existing users are protected from potentially breaking changes (and more "where's napi.h?" tickets) which is really crucial from my side of this proposal.

I can have a go at retrofitting such an option in the coming days, but by all means, and particularly if you have a specific thought or idea on this, be welcome to tear into the task yourself if you wish.

It is reassuring to know that you're seeing what I am getting at here, whether or not you see the merit in supporting it. As code owner, I will respect whatever you decide of course, but I am starting to feel like the case has been made and trust that whatever you do decide to do, you will have weighed your decisions from the perspective of users such as me also, even if we don't have like-for-like workflow, needs, and end goals. What was really getting me anxious was the idea of seeing cmake-js turning away from a whole lot of possibilities that it wasn't even fully aware of.

And once you get so far into your CMake project, detangling is such a nightmare that you really only want to try do this once and do it well enough to be rock solid for years to come.

I am deeply sorry for the massive PR here (I'm a bit green in some regards...), but I do feel we have a really tactile developer experience on our hands, a merging not just of two languages but two workflows, and the CLI already did so much lifting that I've been able to discover new ideas in the API as I've gone (i.e., the link level arg... feel free to rename this if you like, maybe 'API level'? up to you...)

I'm feeling more confident now that I know you are at least aware of what untapped potential cmake-js still has in it.

@nathanjhood
Copy link
Author

nathanjhood commented Feb 6, 2024

Thinking about the globbing of headers...

I will give a bit more detail on the remainder of this post from my perspective - after giving you a chance to digest this round of commentary (no API commits though! Feeling good).

I just want to highlight something about a certain strategy that you really must avoid, from a security point of view.

I beg your pardon that you might surely know much of this, but as always, a bit of context makes it all the more clear.

Context:

  • when you call 'target_add_sources( )', the function evaluates down in to the equivalent of this very psuedo gcc-ish compiler command: $ gcc -x <file> - right?
  • when you call 'target_include_directories( )', the equivalent psuedo-gcc is: $ gcc -I/dir - right?

The elements prefixed with -I are just paths (usually you will see every include path your target carries, all prefixed with this flag). Your headers are - not always, but somewhat generally - just symbolic, in a sense very much similar in some ways to .d.ts declaration files in Typescript; they need not carry any actual function definitions, they just carry the function signatures. C/C++ compilers don't really care whether a function was defined or not yet at a point in your code, so long as atleast the signature was declared already - meaning, the params and return type and function name. So, the headers are basically just used for symbol 'look up' early in the game; in this typical scenario, headers do not get compiled - they get shipped as raw files, for devs to use, under sudo apt install libnode-dev and such. Or, they don't get shipped at all, like when you sudo apt install node. (CPack is offering both of these options btw!)

The elements passed in with the -x arg - or whatever it is for your toolchain - should be source files, i.e., have the '.c/.cc/.cpp' type file extension. These are passed directly in to the compiler and are built into binaries like libs and exe's. Of course, you know that. So here's the point.

file(GLOB) does not copy/paste a file wholemeal from one place to another. No, it actually reads in the entire contents of the passed-in file(s), and parses them all into a massive CMake string, in memory. More or less the same as those big multi-line strings I used for the console readouts at the end of the API runs.

That is extremely slow. CMake also keeps a thing in the binary dir like 'verifyglobs.txt' which keeps a tab on things changing, and runs the entire globbing expression again whenever it evaluates to be necessary.

If that means you're globbing a huge header fileset within your own source dir, and make one tiny change like a typo, the entire globbing regex might re-run when the change is detected by CMake. It's slow... I had it working on the libnode-dev header fileset nicely, but the configure speed was unpresentable. The targets file actually did have the entire nodelib header set all ${_IMPORT_PREFIXED} and everything. The cmake-js::node-dev target's file list was blanked out by my intellisense due to being over the max linecount! Heh.

That is actually not my biggest concern here though.

What if your remote server is running cmake-js and building your addon, but a malicious user has injected a file into your compile line? Something like malicious_in_disguide.exe.cpp.

How?

$ yarn cmake-js configure --CDNODE_ADDON_API_FILES=malicious_in_disguide.exe.cpp

If that file is being file(GLOB)bed and fed directly into 'target_add_sources()', it's being read in as-is, byte by byte, and passed directly in to $ gcc -x malicious_in_disguide.exe.cpp.

Uh oh.

It might not quite be that easy, but I'd be willing to bet that someone will try it.

It might not happen to CMake users at home with no internet, but in NodeJS land? You should see my public server logs... I'm not prepared to let anything have a chance.

So you can see I've done battle with that implementation in order to be as sure as possible that things are being well-processed and managed to the best of CMake's functionalities.

Oh, I'm passing all the headers into target_sources(), I know... but that's what FILESET HEADERS takes care of - CMake itself will go, "oh ok, those ones go on -I because they are a HEADERS FILESET, not on -x for compiling..."

source_group just makes the headers appear in one tab on Visual Studio. It doesn't process the files in any way besides IDE integration.

Now that I hope this is a bit more clear, you can probably feel a bit more comfortable breaking my work down as you see fit, knowing a bit better about why I did certain things in certain ways (even when other easier means seemed to be available).

CMake offers these newer functionalities for reasons. I say use them, let kitware do what they know is best with it. Once you're in their API, they are taking care of it "all working properly, all the time, everywhere" so that we don't have to. You just have to do their dance a little bit to get those benefits.

@nathanjhood
Copy link
Author

nathanjhood commented Feb 6, 2024

Perhaps it would make sense to move the root cmakelists and demo code to be a project under the tests with the others? That will help clarify that it is only for testing purposes of this project

Tbh we may remove the root CMakeLists.txt, my demo.js/.ts/.d.ts under 'lib', my commands from package.json, and also the typescript/node types dep I added (that was purely to activate the intellisense for the demo, obviously not something we truly need).

Now that we have example projects, we can test stuff out on those. It is rather handy, though, to use that CMakeLists to quickly jot down a function you just added to API, and give it a few quick tests right there.

I don't know if you have thoughts on any 'actual' CMakeLists project under the cmake-js name. I mean, there is scope for anything really; 'add_executable' could put together a really simple native CLI for adding a bit of control over the header package downloads... perhaps? Perhaps not. I'm just pointing out the cmake-js still has scope for it's own CMake project, should the need ever arise. That was the intent behind sticking our API off to the side under 'share' instead of being a root-level file. It can stay out of the way when not needed. IMO, pre-existing projects should probably be shielded from unintentional contact with the new API, ideally.

In any case, everything under the 'cmake-js' CMake project space in the current proposal is fully removable without breaking anything in the API.

Your ideas for more functions sounds very welcome IMO. Keep in mind that includers of our API will also gain these functions for their own customary usage, if they wish.

project(my_custom_addon)

include(CMakeJS) 
# NOTE: If we add a 'project()' call in the API, then it becomes imperitave that users only include us *after* their own 'project()' call, otherwise we are forcing them into building a sub-project of cmake-js, which is really not the correct thing to force on builders. This is why I never defined our own 'project()' in the API, but instead in an off-side file for internal usage.

cmakejs_acquire_napi_cpp_files()
cmakejs_setup_napi_things()
cmakejs_create_napi_addon()
# ...

The above was my reasoning for refactoring some logic into separate functions. Not to mention, that by functionising it's gonna be much easier to debug individual things in isolation. Not to mention, to isolate the changes we inevitably make to these functions, over time. Also note the usage of PARENT_SCOPE when inside a function. I'm not sure if it may be better to swap functions for macro(), which doesn't have it's own internal scoping...

As long as the functionality is quite clear then I think those functions are quite cool and handy to have around, as a user. Especially for stuff like tricky CI/CD runs with permissions hell going on. Perhaps one platform just needs the little extra helper in order to build...

About link-level and optional targets... I honestly haven't even thought about NaN (where on earth do they get these names??)

I spy a new ticket on the issues page which may provide us some further insights into things people want to do with cmake-js. We totally have the opportunity right now to account for these types of users. You may be totally right about not needing link level, but do be weary about how our create_addon function is currently expecting to link to cmake-js::cmake-js, and of course be sure to account for that in any changes you make. The inter-dependencies between targets is something I tried to hard-code away from end-users, and alleviating the strictness of this may lead to further tickets from confused users.

For consideration!

Ciao for now
Nath

* (Be aware that CMake projects do not exist side by side. Once the first 'project()' is called, everything hance forth is a sub-project of that project, and so on. This is why CMake generates a <project>IS_TOP_LEVEL var on every run. CMAKE_SOURCE_DIR is the 'top-level' master project, and CMAKE_CURRENT_SOURCE_DIR is whatever the most recent 'project()' call was. We definitely do not want to use PROJECT* in our API! It is for downstream users to control, not for us)

@mmomtchev
Copy link

@nathanjhood @Julusian pardon me for crashing this party, I am the author of [SWIG JSE] (https://github.com/mmomtchev/swig) which generates JS bindings from C/C++ headers. It supports dual build setups where it generates identical bindings for Node.js native and browser/WASM. Currently, my example project uses node-gyp which I know very well. I would like to add cmake-js support. I see that I am coming here at the right moment.

I have some additional requirements:

  • cross-compilation (WASM)
  • distribution of pre-built binaries (I am also the author of @mmomtchev/node-pre-gyp-github which should probably be adapted for this task
  • conan compatibility (I am also the author of magickwand.js, a SWIG-generated project, which uses conan for both native and WASM

I will try to follow this discussion (so much prose...), but me neither I do not know CMake well enough to design the node-addon-api layer. I have lots of free time on my hands (I am unemployed) and I can probably help with something.

@Julusian
Copy link
Collaborator

Julusian commented Feb 6, 2024

We dont want to define or rely on at API level, it isn't correct. I'm scared of suggesting CMAKE_JS_INSTALL_TARGETS in case that var namespace disappears, per your suggestions. But I think the idea of making that relocatable step completely optionable is probably workable.

Possibly. I'm tempted to see how it goes with the relocatable stuff first, that behaviour can always be added later if it is actually a problem.

Re globbing:

OK. Im not convinced that is a realistic concern (if they were able to get a new file into the globbed path, wouldnt they be able to modify the CMakeJS.cmake to inject the file too?) But I'm fine with leaving it with them manually specified for now.
Again, this can be reconsidered if it becomes an issue of not including the correct headers.
And if I do change node-api-headers so that it is defined by the users, then it becomes if it does break then the user (of cmake-js) can change the version they rely on to be one that works. This probably should be done anyway, so that they are in control of the version of those headers as that defines what node-api 'level' is available

@nathanjhood
Copy link
Author

nathanjhood commented Feb 6, 2024

re: globbing, just want to check you saw my edit re: $ yarn cmake-js configure --CDNODE_API_FILES=something_malicious.exe.cpp

Or, if we are not validating the collected headers, someone might possibly sneak this in using something like sed: #define false true

Uh oh!

It seems that the idea of calling project() in the API might have arisen again, unless I am mistaken.

Not calling project() and not using PROJECT_* vars was of course highly intentional, and is critical in order to not break end-users' CMake experience.

May I demonstrate in psuedo CMake, someone making an addon with our API:

project(i_am_a_cmakejs_user)

include(CMakeJS) # imagine this contains a 'project(cmakejs)'

create_addon(my addon ....)

message(STATUS "PROJECT_NAME is ${PROJECT_NAME}") # output: "PROJECT_NAME is cmakejs"

Not only are all of the PROJECT_* vars now completely irreparably broken for the cmake-js user, but also, since CMake projects exist under a 'top-level project'/'sub-project' hierarchy, all of these vars will also be immediately broken for our API users:

- ```CMAKE_BINARY_DIR``` - ouch!!!
- ```CMAKE_SOURCE_DIR``` - users cannot build addons if this is not pointing at their dir!
- ```CMAKE_CURRENT_SOURCE_DIR``` - this would *also* not be pointing at their dir, but ours!

I admit that I have not attempted to try this and see if everything breaks, but I want to make sure that it is understood, why calling project() would almost definitely cause breakages in cmake-js users' CMakes.

Remember that CMake users know how to use CMake, and know how I expect those vars to behave in their CMake projects. Nobody else breaks these, I believe cmake-js should also not break them.

Consider that vcpkg is an entire C++ package manager, written almost entirely in CMake. They never once call project(vcpkg) in their entire codebase; probably for this reason.

re: cmake_min_required

Without this, you have to use cmake_policy() instead.

Nobody in the developer world has a logical reason to use anything other than the latest Release version of CMake. Kitware are very clear on this. CMake is one of the golden examples of backwards compatibility in the software world, even CMake users complain that kitware refuse to break anything, even if for the greater good of today's developers. Banks are running COBOL and ancient compiler instructions, somebody is enabling them to do it.

The above also pertains to why it is definitely worth going along with their tricks. By plugging in to their full interface feature set, we get all those benefits too.

@Julusian
Copy link
Collaborator

Julusian commented Feb 6, 2024

It seems that the idea of calling project() in the API might have arisen again, unless I am mistaken.

No, no intention of doing that

Nobody in the developer world has a logical reason to use anything other than the latest Release version of CMake

I use the version that is installable from my system package manager 🤷 (currently 3.25, Im using ubuntu 22.04)
As one of the aims is to make this 'just work' on machines of users who don't know or care what cmake is, the less pickier we are about the version the better.

@Julusian
Copy link
Collaborator

Julusian commented Feb 6, 2024

@nathanjhood I'm currently doing some reshuffling, mostly to get it to be using the function based feature flow.
https://github.com/cmake-js/cmake-js/blob/cmakejs_cmake_api/share/cmake/CMakeJS.cmake
I don't think I have broken the exports, but I am not 100% sure just yet (I need to re-run the before so I can compare the output)
Almost everything is still there, just reshuffled and with some of the large comments removed

There is some renaming (they prefer calling things node-api instead of napi these days).
But the main part of the change is that the hello cmakelist now looks like:

cmake_minimum_required(VERSION 3.15)

project(hello)

list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/node_modules/cmake-js/share/cmake")
include(CMakeJS)

# cmakejs_setup_node_api_c_library() # This can be used, but is called by cmakejs_create_node_api_addon as it depends upon it
cmakejs_setup_node_api_cpp_library() # This is the new bit, to opt into searching for and linking to node-addon-api

cmakejs_create_node_api_addon(
  addon
  "src/hello/addon.cpp"
)

In other words, by using cmakejs_create_node_api_addon it does mostly the same as before, but only links to cmake-js::cmake-js and cmake-js::node-api by default, and cmake-js::cmake-js doesn't depend on the other targets anymore. But once cmakejs_setup_node_api_cpp_library is called, it will also link to cmake-js::node-addon-api.
This might be too magical for some tastes, I am wondering if I should make that part be opt-out.
But I think this is a good default behaviour. Because now when cmakejs_setup_node_api_cpp_library and cmakejs_setup_node_api_c_library are called, they will fail if they are unable to find the headers that they need. And these codepaths are only entered if the user wants the targets that need them.

This is largely preparation so that cmakejs_create_node_v8_addon (I'm not sure what to call this yet) can exist for users who are using the 'old' module api. Which will also have a cmakejs_setup_node_nan_library to enable searching and linking to nan.

@nathanjhood
Copy link
Author

nathanjhood commented Feb 6, 2024

Cool, what I'm just up to is new branches on my other repos, where I'm trying 'migration' from my old CMakeLists over to the new API, so thus I can have fun with it while staying off your commit radar :)

As per my own last commit on this PR, every single project under tests/api and tests/targets was working fully. The absolute verification is:

  • all the CPack outputs should behave as expected. Source output dir is clean, with no weird surprises, and the other output should contain the depenencies it used (the node-dev target won't have any Napi Addon headers at all, the cURL project will also have cURL headers... etc)
  • the CMake files in CPack's output should contain a CMakeJSTargets.cmake which shows no absolute paths pointing at your filesystem. CPack cannot be sending out people the paths from your filesystem!

Actually that very last one is probably the crux of this entire 'relocatable' business. How can we addon builders send a CPack archive of our addon project files out, if it contains absolute paths?

Broken record here, sorry - I am pretty sure this is all making sense now. Phew :)

list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/node_modules/cmake-js/share/cmake")
# cmakejs_setup_node_api_c_library() # This can be used, but is called by cmakejs_create_node_api_addon as it depends upon it
cmakejs_setup_node_api_cpp_library() # This is the new bit, to opt into searching for and linking to node-addon-api

Are any of these lines user requirements for the implementation to work? I don't object to passing simple function calls to newcomers, but we probably agree that we don't want to ask people to do things that they don't fully understand, we don't want to need to write lots of documentation just to get a newbie's addon online. But if it's a demo of what intermediate/customary workflows can achieve? I am all for providing these functions, yeah. Especially helpfully where somebody has a weird edge-case of CMake build failure, due to being on some weird edge-case platform. They can use logic to optionally do additional config like that.

Additionally, I gave existing users a little extra protection by wrapping everything in those options at the top. I think there is some rationale to providing an 'easy' target level which just sorts out everything, and the others are more customary for people who only need a specific thing. The question is, which level of user should have which things provided to them, and which things should they also not have? Obviously, you as codeowner will have the best vantage point on this, in the end.

This is largely preparation so that cmakejs_create_node_v8_addon (I'm not sure what to call this yet) can exist for users who are using the 'old' module api. Which will also have a cmakejs_setup_node_nan_library to enable searching and linking to nan.

I did already build some of these from the current API - did you check the source files under tests/targets? There is one written using each level of API, so one is in (not-quite-pure) C using napi_stuff, and one is written as a Node extension using v8::Things.

As long as none of those example projects ever break from their functional state of my last commit here, then I don't really have any opinion on how you implement anything. Those projects are the experience I want from a cmake-js CMake API, and if that stuff all keeps working that way (maybe you will still improve on it somehow too) then I can go back to being a happy builder, who is now shipping my addons to my happy consumers, and everything is really sussed out for us all.

Let me know if you want me to confirm anything per the above regarding any changes you make or propose. I appreciate your patience on this mega journey but I am quite happy to hand over the reigns, as long as my examples don't break (without a very compelling reason).

My commit reference containing my completed proposal is:

$ git rev-parse cmakejs_cmake_api

f427db8f706da4f02a4aacc1e46763306d9d3a51

I guess it will be preserved on my fork at https://github.com/nathanjhood/cmake-js#cmakejs_cmake_api.

I will probably deprecate the above fork in due course. I'd like to have it around for preservation though.

Cheers and good luck!
Nathan

@Julusian
Copy link
Collaborator

Julusian commented Feb 6, 2024

Are any of these lines user requirements for the implementation to work?

list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/node_modules/cmake-js/share/cmake")
That is necessary, but is unavoidable if users want to have working intellisense in their ide. I think that it makes sense for it to be required always so that users will get working intellisense as well as a working build, and not get frustrated trying to figure out why it doesnt work and then end up adding this anyway.

cmakejs_setup_node_api_cpp_library() this is needed if they are using node-addon-api.
If we don't require this line, then we need to try finding it and fail, and then if we fail to find node-addon-api they will get a compiler error instead as we couldn't safely give them an error earlier on.

I did already build some of these from the current API - did you check the source files under tests/targets

Yeah I did, but having removed the node-dev layer because it isn't needed for node-api projects means that some of those broke, and then with the other changes the rest broke 😆

I have fixed most of them up, but until I fixup the node-dev library (which needs some thought on how to source the headers without the CMAKE_JS_INC variable) it wont work.
Oh, and for windows, we need to use a different node.lib to the one we are currently generating for that too.

I will probably deprecate the above fork in due course. I'd like to have it around for preservation though.

Whatever state that branch is in when it gets deleted, will be preserved as part of this PR.

@nathanjhood
Copy link
Author

nathanjhood commented Feb 6, 2024

Yeah I did, but having removed the node-dev layer because it isn't needed for node-api projects means that some of those broke, and then with the other changes the rest broke 😆

Right? CMake will fight you every step of the way, if you don't quite go along with it. This is why I can't always give you a better reason other than "CMake convention"... you sort of learn that once you just go along with it's ways, that is when suddenly a CMake project, no matter how complex, "just works".

Even if you fix it for yourself in your own test addon, does it work for 'hello_consumer'? Obviously they should have a silent build experience

The dependencies chapter, cmake-js already took care of this with its CLI. We didn't do any of this in the API because cmake-js already retrieves our dependencies (headers etc) but it's a good short little reference to skim, for context.

Importing and exporting. Modern CMake. So, the CMakeJS API is exporting itself. This allows builders to import it. The import section is what users will expect to do with our API. The exporting is to be provided by the API... else, the user must export export it themselves to leverage this.

no need at all to read them all - more rather, when you have specific queries, if isn't about dependencies or importing/exporting/relocating, then there is probably a nice digestible chapter on it in one of the other two links. At a pinch, just refer to 'Mastering CMake's chapters, the others for reference.

Best of luck, I'll stay tuned in!

@mmomtchev

This comment was marked as off-topic.

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.

None yet

3 participants