Add node-gyp setup for NodeJS bindings #43
Conversation
It's all right. What is needed is understanding how does node.js/npm infrastructure works and what are common expectations from its users. Down to business. We have a two-sided coin. From one side we have node.js/npm users' expectations, and from the other side we have blst-specific build steps that can be at odds with node-gyp, most notably calling swig and compiling libblst.a. Natural expectation would be that it would be solved with custom build steps. Unfortunately referred documentation is silent about them. Is it so that it's simply undocumented, or is it totally unsupported? Can you tell? |
bindings/node.js/binding.gyp
Outdated
| { | ||
| 'targets': [ | ||
| { | ||
| 'target_name': 'blst-node-binding', |
There was a problem hiding this comment.
Shouldn't 'target_name' be just 'blst'? My impression is that this how the shared library will be named, and it's also the name it will be referred by in the 'require' statement in the script...
bindings/node.js/binding.gyp
Outdated
| '../libblst.a' | ||
| ], | ||
| # Is 'libblst.a' a library or a source? | ||
| 'libraries': [], |
There was a problem hiding this comment.
libblst.a is certainly a library. There is a caveat trough. It most likely have to be named differently on Windows. In other words this might have to be OS-conditional...
bindings/node.js/binding.gyp
Outdated
| '-DBUILDING_NODE_EXTENSION', | ||
| ], | ||
| # node-gyp has both cflags_cc and cflags. Not sure which one to use | ||
| 'cflags_cc': [ |
There was a problem hiding this comment.
Sounds like 'cflags_cc' are C++ flags and 'cflags' are C. So that above -std-c++11 and -DBUILDING_NODE_EXTENSION should go here.
There was a problem hiding this comment.
-DBUILDING_NODE_EXTENSION appears to be added by node-gyp automatically, there is no need to bother.
bindings/node.js/binding.gyp
Outdated
| ], | ||
| 'cflags': [ | ||
| '-bundle', | ||
| '-bundle_loader $NODE' |
There was a problem hiding this comment.
As for -shared, -bundle, -bundle_loader flags, my understanding is that node-gyp should take care of those. At least the whole idea behind it is to liberate you from thinking about such platform specifics... So just remove them as starting point. BTW, -Wl,-Bsymbolic is effectively optional. Everything should work without it, but it works slightly better with. In other words it can also be omitted as starting point.
|
Following binding.gyp appears to be sufficient to build It naturally requires blst_wrap.cpp to be available. Which is not ideal, one wants custom build step to generate it. And it obviously won't work on Windows, because you can't use assembly.S. One can wonder if node-gyp supports file globs such as |
|
I've added the ascetic version of binding.gyp and would suggest to re-purpose this PR (or open new one) to work on system-dependent parts. For now I've masked possible errors with |
|
@dot-asm Thanks for the feedback! Will investigate further or reach out to node-gyp devs.
A SWIG generated |
|
@dot-asm I've tested in this repo to build the SWIG file on multiple NodeJS versions and the result is the same. Is that expected? You can inspect the resulting files in the artifact binding.node |
A given SWIG version generates same blst_wrap.cpp irregardless node.js version or platform. Question is if you can compile it for your node.js. This is the question that stands in the way for providing pre-generated wrapper. Because formally speaking user might have legitimate need to use specific SWIG version of their choice. As simplest example, Ubuntu 20 is fully self-consistent in sense that if you use vendor-provided node.js and vendor-provided SWIG, it works all the way. Yes, blst_wrap.cpp pre-generated with custom-patched SWIG version works with vendor-provided node.js too, but it's really circumstantial... And argument goes other way too. Output from next-next SWIG version can be absolutely preferred. What one can do is to query |
Should we expect then that all javascript users of this library to have SWIG installed? I don't fully follow your answer; in what cases will the blst_wrap.cpp will be different? |
blst_wrap.cpp differs with the SWIG version. And there is no guarantee that blst_wrap.cpp generated with specific SWIG version will work with specific node.js version. Hence we can't provide pre-generated one, it's unmanageable in the long run. It would have to be something for users to keep up with, pushing us with emerging problems, as well as SWIG if it turns to be their problem. Like outstanding one in #32. |
In Lodestar context we cannot expect users to install SWIG. They should install |
|
I would suggest to concentrate on the matter here, binding.gyp. Does it work on MacOS X? What does it take to make it work on Windows? First things first. |
|
I managed to get binding.gyp work on Windows, see 0f91a05. At least on my virtual computer. This last sentence means that I don't really have confidence that it would work universally. Trouble is that when you install node.js, it asks if you want to install additional development tools, and it looks like it's installing some compiler components. But since I have even newer Visual Studio installed, it might end up using that one, and it might happen that it's the one that actually works. In which case me successfully building it isn't actually reassuring that it would work on a computer without newer or full-blown Visual Studio installation. In other words, feedback would be appropriate;-) As well as from Mac... |
dapplion
force-pushed
the
node-gyp
branch
3 times, most recently
from
101d643
to
ebf9403
Compare
Thanks for pushing this ❤️ I've rebased this branch on top of your work and setup a Github Action to test the node-gyp file on ubuntu-latest, macos-latest, windows-latest. In macos-latest, windows-latest SWIG fails to be set up. Do you know how would it be setup in those environments? In our blst-ts wrapper I build blst_wrap.cpp first in ubuntu only and then run node-gyp on all 3 OS. However the build still fails. You can see the failures per OS macos-latestSeems a build error that I can't understand what's the exact problem
windows-latestLower NodeJS versions fail. I can't understand the build error as it doesn't seem to contain much info
If you want I can recreate this two stage build where SWIG is only run for ubuntu on this branch |
It's the |
Hmm... Note that first error from compiler points at v8.h. It's as if the node.js header file is broken. It's unlikely to be the case, so one should wonder if one can actually use blst_wrap.cpp generated by patched swig with node.js 10.x. At the very least I know following. Ubuntu 20 is equipped with node.js 10.x by default and stock swig 4.0.1, and they do work together. I can check if I can intermix on Ubuntu, but don't hold your breath that it will yield solution for Windows. Secondly, it issues "C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc" warning. This is Windows equivalent of -fexceptions, except that it's a warning, not a fatal error. But node-gyp is supposed to pass it, so apparently same problem as with MacOS? I get v7.1.2 if I execute Just in case, there are also warnings about cast between |
I never tried, so I can't say that I know. If I had to guess, first I'd wonder if there is equivalent of Linux |
|
Was I dreaming? Didn't https://gyp.gsrc.io/docs/UserDocumentation.md mention |
|
There is another update to binding.gyp committed. Based on combing through node-gyp source code. Check it out. I don't understand why they did it that way. Normally you'd want to provide a unified way to control compilation on multiple platforms, but node-gyp seems to encourage the division among developers... |
I can. Moreover, I can intermix them on my Windows [virtual] machine. So that failure on Github Actions is a mystery. |
100% Not sure if you could provide any help, but in our wrapper the bindings tests for macos-latest NodeJS version 11 very often fail with https://github.com/ChainSafe/blst-ts/pull/14/checks?check_run_id=1469860354#step:6:64 There is no more info nor logs. It's very strange because it happens in NodeJS v11 exclusively and only sometimes... Thank you so much for the fixes! ❤️ Now node-gyp works for all macos-latest nodejs versions
Have you tried with NodeJS v10 in your Windows virtual machine? |
Yes, that's what I actually meant to say. [Just in case, reference to "virtual" merely means that it's not my platform of choice, and as result I might fail to recognize common problems that would appear obvious to others, or fail to tell if there is anything special about my setup that does any specific trick.] |
Yeah I'm on the same boat, haven't used Windows in +15y and recently only through Github Actions ✌️ |
Can you reproduce it on your computer? Common way to troubleshoot such problems is to start application under debugger, crash it and collect stack back-trace... But I don't quite understand. It says SIGABRT in the middle of what appears to be a line of successful tests. Is it correct impression? If so, is it like there are multiple scripts executed one after another, and crash is observed as some particular script finishes? If so, I'd first wonder if there are some subtleties in memory management that might have to be mitigated in blst.swg. Or it might as well happen that this is how incompatibilities between swig and node versions manifest themselves... One can wonder what happens if you take blst_wrap.cpp generated by swig 4.0.1, which does support node pre-v12. It should also be noted that blst_wrap.cpp compiles with warnings, and they might give a clue [which might turn to be swig's problem]. But aren't odd node.js version kind of "throwaway"-s? I mean they are maintained only for short times and serve as kind of playground for new features? In other words, if the problem is actually specific to v11 and nothing else, then it might be appropriate to ignore? |
Cool! And now binding.gyp has a custom action and template blst_wrap.py script. I assume that binging.gyp doesn't actually have to reside where it is. For example you might be able to copy it to location of your choice, re-bias relative paths and so to say "take over"... |
I don't own any MacOS so unfortunately not. For my own Linux it does not happen.
Yeah, this can be totally ignored and users should not be using this version anyway. Just checking if you had any idea. If the error is persistent I will just drop support for macos-node11
Since I'm referencing supranational/blst as git submodule I would rather not have to modify anything for the convenience of just pulling changes easily. I'm down to push any setup changes upstream so everyone can benefit from an improving setup The last valuable item would be to be able to generate the prebuilt bindings for Windows on Node v10 because I imagine it's a very common environment. |
|
I've rebased to current master and modified the CI workflow to build with SWIG once and re-use the result for ubuntu, macos and windows |
|
Sorry about the delay. So node v10 build on Windows fails again. As already mentioned, it looks as if headers are corrupted. We'll have to see if it depends on node's minor version number... |
|
Microsoft blip blip bliiiiiiiiiiiiiippppp bllllliiiiiiiiiiiiiiiiiippppppppppp blip.... Blip blip! Blip... I mean COME blip-ing ON! "We support c++14." Then why don't they predefine freaking |
Not that I have access to actual standard, but all the drafts stipulate that |
hahaha, totally justified market share... The files
Are generated by you locally? Could both be generated on CI on this PR? |
|
After adding |
I'd argue there is value in exercising both, because it allows to tell apart node-gyp-specific problems. I mean if run.me succeeds, you know it's something with node-gyp. Though it might make sense to separate the steps, so that one knows which log file to look at from single glance... |
Yes.
First of all, why two files. If you test v12 file, you can find that it actually works even with earlier node versions, so why bother with v8 at all. Well, v12 is generated by unofficial patched version, so that formally speaking it's "not there." But as soon as swig catches up and specifies which node versions are supported by new version, it might be possible to switch to single pre-generated wrapper. Now, with this in mind, is it possible to generate them in Github Actions? Should be possible if one gets creative enough. The v8 file was generated on Ubuntu 20 with stock swig 4.0.1. So I suppose if one spins up ubuntu-20 image instead of ubuntu-latest(*), one can generate v8 as is, and then v12 with ~/swig/bin on modified $PATH... (*) Though they plan switch ubuntu-latest to ubuntu-20 soon. |
|
@dot-asm Thank you! I'm trying to replicate this setup in blst-ts (= run SWIG through node-gyp) but I can't get blst_wrap.py to work, I'm not sure why it doesn't find This is the CI run with the error https://github.com/ChainSafe/blst-ts/pull/19/checks?check_run_id=1530028228#step:8:46 |
|
@dot-asm Would you mind pushing this fix to master so I can update our git submodule? https://github.com/supranational/blst/pull/43/files#diff-7009d5c1156c3b0d41aff8545e89c58b863b63fae7286c43e1f5a4bf066bdf97R28 I confirm it fixes the build issues in windows node 10,11 so thank you so much!! ❤️ 🎉 |
|
As already mentioned/implied, I'm opposed to the idea of keeping pre-generated wrappers in the main tree. It's not sustainable in long run. |
I agree, the idea is to generate those on CI and distribute them over NPM but not to keep them in src. Also since blst-ts references supranational/blst as a submodule I would like not to do any modification at all (like moving blst_wrap.py around) since it makes it harder to track. Do you think this is a good strategy to wrap your src or maybe there's a better path? |
|
I've bumped supranational/blst git submodule in our repo and I'm unable to build with node-gyp on any OS https://github.com/ChainSafe/blst-ts/pull/13/checks?check_run_id=1553743489#step:5:112 I see your note in binding.gyp about customizing the files but I believe the optimal strategy is that the upstream repo works without any modifications while we continue to use git submodules. blst_wrap.py will run swig even if it finds a blst_wrap.py file? |
|
But let me pick your brain. I'd expect to find references to bingings.gyp or a build script in package.json. I see reference to dist/scripts/install.js, but where is it? If it's generated from src/scripts/install.ts, then install procedure doesn't look at package.json? At least not at first? In such case why have it at all? Is it correct understanding that installation procedure will try to download blst.node binary, and if suitable is not found, it will try to build from source? If so, why not attempt to download blst_wrap.cpp prior build attempt? |
|
Are there environment variables set by any of the building tools involved? The reason I ask is that if there is one pointing at your project root, it might be appropriate to have blst_wrap.py look for pre-generated wrappers in a directory relative to your root. If no variable is set, we can just make one up, and you can set it in [your] build script... |
We can choose to specify directory where to look for |
One can also argue that |
|
Thanks for the explanation! I've successfully integrated latest master into blst-ts, and it works fine for all OS and NodeJS versions including windows-latest node v10. Also worker-threads work fine. I've opened a PR with the updated blst_wrap.py #48 |
|
Given that the blst_wrap.cpp works fine for all environments I will skip generating two versions (v12, v8 you proposed). Let me know if you forsee and issue with that. |
|
But could you walk me through your "behind-the-scene" build procedure? This is for general education... |
If you refer to the CI runs to get prebuilds: Run 1 job
Run matrix job
The Typescript scripts main logic is here https://github.com/ChainSafe/blst-ts/blob/master/src/scripts/install.ts which tries in order
Let me know if that answers your question |
|
Closing to not pollute the repo since the purpose of this PR is fulfilled :) |
The interest is more general than that:-)
Is it correct understanding that "bootstrap" is a reference to the "bootsrap" line in package.json? Then the "bootsrap" line in package.json calls Some additional questions. Is src/bindings.ts file manually composed? If so, question is how do we keep it up to date with blst.hpp as it evolves. Best would be if we had a copy to play with in blst/bindings/node.js, along with an ascetic sanity test. This way we can make changes at own pace, ensure that they are syntactically correct, and then ping you for feedback and integration. Can you help with this? The ascetic test that is. No rush:-) I've tried to simply run |
Yeah that's exactly what's going on. They key thing here is that for regular users that consume the library, it pulls the build content from NPM and runs the install script afterwards
Yes it's manually composed. Right now there are tests for every single method to ensure they exist and take those specific arguments. I can help with it of course. Do you think current blst-ts as-is is a good starting point?
If you just run or To run the project's tsc version |
Cool! Thanks!
Well, I, a typescript illiterate, am not exactly right person to ask:-) There were some additions to blst.hpp recently, but it would be good exercise for me to add them. So yes, let's go with what you've got as starting point.
Oh! There is per-project culture... Well, in my mind it's only more reasons to aim for previous version:-) For the intended purposes minimalistic test could simply perform syntax analysis, say with |
You can always run |
If you can accept two json files with 4 lines total this is the minimal thing I could came up with |
Kickstarter to setup node-gyp to build NodeJS bindings. This setup should be identical and ideally replace
run.me. I have very little experience with c and c++, and I've expressed my questions as comments to the file. I've granted permissions to the forked repo contributors to edit this branch so please feel free to push fixes to the setup I've started.I've modified the Github Actions workflow to see current build errors on CI
node-gypis the standard NodeJS addon builder, maintained by NodeJS team. Link to user documentation if necessary https://gyp.gsrc.io/docs/UserDocumentation.md