All: Add binary support for node 4 and 6 #103

gabrielschulhof · 2017-08-05T01:04:18Z

This is the first pass at achieving never-compile-again. I've tested it on Linux and I've made sure that it continues to work as before on Windows.

Many things can still be improved. For example, there is no need to build n-api-implem.node except when building via prebuild, however, I cannot distinguish in binding.gyp between a build initiated via npm install and a build initiated via prebuild.

gabrielschulhof · 2017-08-05T01:07:54Z

@mcollina

mcollina · 2017-08-05T10:56:03Z

This is fantastic work. If we could support windows as well, it would be superb. I will test this asap, have you got the prebuilts uploaded somewhere?

gabrielschulhof · 2017-08-05T12:11:31Z

@mcollina prebuilds are kind of a problem, actually. As it stands, the binaries containing the N-API implementations for 4 and for 6 would show up as tarballs at https://github.com/nodejs/node-addon-api/releases and the npm install for this package would download them from there.

When I was testing this I forked the repo and changed all references to https://github.com/nodejs/node-addon-api in package.json to my personal clone at https://github.com/gabrielschulhof/node-addon-api, so when I ran prebuild it put the tarballs into the releases section of my personal clone.

I do not wish to pollute the release stream for this upstream repo with tarballs unless we all agree that we should perform the test.

So, if you wish to test this, I would recommend that you

clone the source repo for this PR (my personal clone at https://github.com/gabrielschulhof/node-addon-api), including the branch called "binary" into https://github.com/mcollina/node-addon-api, but not using github's "Fork", but rather by creating a new repo on Github and then pushing this to it.
Change all https://github.com/nodejs/node-addon-api in package.json to https://github.com/mcollina/node-addon-api
Follow the steps in https://github.com/gabrielschulhof/node-addon-api/blob/binary/PREBUILD.md to upload the prebuilds to the releases stream of your personal clone
Have the module with which you wish to test this depend on

...
"node-addon-api": "git+https://github.com/mcollina/node-addon-api#binary"
...

and then simply do this with your module:

nvm use 4
npm install
node -p 'something that causes your module to load and output something'
nvm use 6
node -p 'something that causes your module to load and output something'
nvm use 8
node -p 'something that causes your module to load and output something'

You should be able to run npm install on any node version 4, 6, or 8, and move to any other node version 4, 6, or 8.

gabrielschulhof · 2017-08-05T12:23:37Z

I guess we need more than just this PR. We also need a workflow to accompany the creation of a new node-addon-api release, which includes producing the prebuilds and storing them on the repo's "Releases" page.

mcollina · 2017-08-05T17:36:19Z

@gabrielschulhof yes! Also, we need some way to make all of this setup work in Node releases between 8.0.0 and 8.x.0, when we drop the flag. But we can think about those releases later on.

I will test this on Mac OS X early next week.

gabrielschulhof · 2017-08-05T23:04:38Z

@mcollina

I have managed to simplify the usage a bit. You need no longer make your own clone. It is now sufficient to depend on

...
"node-addon-api": "git+https://github.com/gabrielschulhof/node-addon-api#test-release"
...

When you do that, it will still do some compiling, but only the loader that does the dlopen(3) "magic". Aside from that, it will fetch the N-API implementations as stored on github.

A further refinement could be to store the loader on github in binary form as well, so that npm install of node-addon-api does not entail any compilation at all.

gabrielschulhof · 2017-08-07T10:44:41Z

@mcollina I have now uploaded prebuilds for OSX. I built them using the procedure for which I added documentation in the PR.

So,

...
"node-addon-api": "git+https://github.com/gabrielschulhof/node-addon-api#test-release"
...

followed by

nvm use 4
npm install
node -p 'something that causes your module to load and output something'
nvm use 6
node -p 'something that causes your module to load and output something'
nvm use 8
node -p 'something that causes your module to load and output something'

should now Just Work™ on OSX as well.

mcollina · 2017-08-08T10:43:54Z

So it's working, but I'm hitting #101 as well when switching versions, it seems working fine, as I can run a binary built on Node 4 on Node 6.

For me it's a big 👍 and LGTM really.

We need to sort out the prebuilt story for this, ideally before landing. Can we do the builds using our CI? cc @mhdawson.

I've opened prebuild/prebuild#184 to track down how to remove the Personal Access Token requirement, it does not seem hard or impossible.

mhdawson · 2017-08-17T22:06:42Z

I'd like to validate on AIX as well.

mhdawson · 2017-08-17T22:08:35Z

I'm guessing this relies of linux exporting all symbols by default, AIX does not do that so it will probably require some extra steps like we had to do for the node executable itself.

mhdawson · 2017-09-08T18:24:37Z

@gabrielschulhof can we put this on the agenda to discuss in the next meeting, would like to better understand and define next steps that we need to complete.

gabrielschulhof · 2017-09-08T21:15:35Z

Absolutely!

…

On Fri, Sep 8, 2017 at 9:24 PM, Michael Dawson ***@***.***> wrote: @gabrielschulhof <https://github.com/gabrielschulhof> can we put this on the agenda to discuss in the next meeting, would like to better understand and define next steps that we need to complete. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#103 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0aBNUAG-qE6FOnOOeAyH-mHHeCOSks5sgYZmgaJpZM4OuUd0> .

gabrielschulhof · 2017-10-18T17:22:28Z

@nodejs/addon-api, @mcollina I made it upload binaries to https://github.com/napi-binary-upload/upload/. That's pretty secure. We now need to test extensively with many different architectures. I couldn't get prebuild to work with Node.js between 8.0.0 and before 8.6.0, because a subsequently loaded shared library cannot eclipse the symbols of the loaded executable.

Here's the built/runs result for the node-addon-api test suite:

Linux	⁠	⁠	⁠	⁠	⁠
⁠	runs on	4.0.0	6.0.0	8.0.0	8.7.0
built on	⁠
4.0.0	⁠	✓	✓	✗	✓
6.0.0	⁠	✓	✓	✗	✓
8.0.0	⁠	✗	✗	✓	💣
8.7.0	⁠	✓	✓	✗	✓

Windows	⁠	⁠	⁠	⁠	⁠
⁠	runs on	4.0.0	6.0.0	8.0.0	8.7.0
built on	⁠
4.0.0	⁠	✓	✗	✗	✗
6.0.0	⁠	✗	✓	✗	✗
8.0.0	⁠	✗	✗	✗	✗
8.7.0	⁠	✗	✗	💣	✓

I renamed the pretest npm script to preparetest so it doesn't rebuild the test bindings whenever it runs npm test. That way we can switch node versions and run npm test again without rebuilding.

I used the following procedure for testing:
edit: `Follow the prebuild instructions to upload prebuilds for the given platform/arch, or make sure that prebuilds for that platform/arch are available. Then,

nvm use 4.0.0
git clean -x -d -f -f && npm install && npm preparetest
npm test
nvm use 6.0.0
npm test
nvm use 8.0.0
npm test
nvm use 8.7.0
npm test

gabrielschulhof · 2017-10-18T19:13:09Z

On Linux, building on 8.0.0 and running on 8.7.0 is successful, but there's an unresolved symbol.

On Windows, building on 8.0.0 fails at the link stage, because the same symbol is available in two places.

gabrielschulhof · 2017-10-19T13:58:49Z

OSX	⁠	⁠	⁠	⁠	⁠
⁠	runs on	4.0.0	6.0.0	8.0.0	8.7.0
built on	⁠
4.0.0	⁠	✓	✓	💣	✓
6.0.0	⁠	✓	✓	💣	✓
8.0.0	⁠	✗	✗	✓	💣
8.7.0	⁠	✓	✓	💣	✓

gabrielschulhof · 2017-10-19T14:01:06Z

Note that for each table the result on the diagonal is the same as without prebuild. Only the off-diagonal checkmarks are made possible by using the prebuilds.

I also edited the test procedure to mention the fact that prebuilds for the given platform/arch must be available before testing can be done.

gabrielschulhof · 2017-10-19T16:08:28Z

This is a departure from the way node-addon-api currently works because it never links node-api.a into the module unless it's on Windows or on Node.js between 8.0.0 and less than 8.6.0. Conversely, outside of Windows or on Node.js between 8.0.0 and less than 8.6.0 it always downloads the prebuilds and loads the module via the RTLD_GLOBAL mechanism, no matter how the module was built.

* Introduce infrastructure for creating prebuilds and uploading them to a github repo. * Use prebuilds on all platforms except Windows and except Node.js >= 8.0.0 and < 8.6.0. * Update documentation to reflect requirement to load native addon `loadModule()` provided by index.js instead of `require()`. * Update tests to use `loadModule()`.

mhdawson · 2018-04-09T21:51:42Z

I believe we discussed this in the last N-API meeting and this is no longer applicable, closing. @gabrielschulhof please re-open if that is not the case.

gabrielschulhof requested review from addaleax and jasongin August 5, 2017 01:08

gabrielschulhof force-pushed the binary branch from 153e049 to b2c326b Compare August 5, 2017 22:54

gabrielschulhof force-pushed the binary branch from b2c326b to b7c78fd Compare August 9, 2017 10:13

mcollina mentioned this pull request Aug 17, 2017

drop --napi-modules earlier than dropping the "experimental" status nodejs/vm#9

Closed

gabrielschulhof force-pushed the binary branch 8 times, most recently from 627cf2e to 24bb8c6 Compare October 18, 2017 17:02

gabrielschulhof force-pushed the binary branch from 24bb8c6 to 8e2fc37 Compare October 19, 2017 13:56

gabrielschulhof mentioned this pull request Oct 19, 2017

Need an AIX machine nodejs/build#891

Closed

gabrielschulhof force-pushed the binary branch from 8e2fc37 to e216cec Compare October 20, 2017 20:59

mhdawson closed this Apr 9, 2018

gabrielschulhof deleted the binary branch December 27, 2018 18:01

gabrielschulhof restored the binary branch December 28, 2018 20:27

gabrielschulhof deleted the binary branch February 13, 2019 23:28

Uh oh!

All: Add binary support for node 4 and 6 #103

All: Add binary support for node 4 and 6 #103

Uh oh!

Conversation

gabrielschulhof commented Aug 5, 2017

Uh oh!

gabrielschulhof commented Aug 5, 2017

Uh oh!

mcollina commented Aug 5, 2017

Uh oh!

gabrielschulhof commented Aug 5, 2017

Uh oh!

gabrielschulhof commented Aug 5, 2017

Uh oh!

mcollina commented Aug 5, 2017

Uh oh!

gabrielschulhof commented Aug 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielschulhof commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Aug 8, 2017

Uh oh!

mhdawson commented Aug 17, 2017

Uh oh!

mhdawson commented Aug 17, 2017

Uh oh!

mhdawson commented Sep 8, 2017

Uh oh!

gabrielschulhof commented Sep 8, 2017 via email

Uh oh!

gabrielschulhof commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielschulhof commented Oct 18, 2017

Uh oh!

gabrielschulhof commented Oct 19, 2017

Uh oh!

gabrielschulhof commented Oct 19, 2017

Uh oh!

gabrielschulhof commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Apr 9, 2018

Uh oh!

Uh oh!

gabrielschulhof commented Aug 5, 2017 •

edited

Loading

gabrielschulhof commented Aug 7, 2017 •

edited

Loading

gabrielschulhof commented Oct 18, 2017 •

edited

Loading

gabrielschulhof commented Oct 19, 2017 •

edited

Loading