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

SWIG support for NodeJS v12 #1520

Open
lissyx opened this issue Apr 25, 2019 · 24 comments
Labels

Comments

@lissyx
Copy link

@lissyx lissyx commented Apr 25, 2019

I understand NodeJS v12 is new, but its release includes changes that break badly even current master of SWIG: nodejs/node@765766b#diff-3c97d28212ec6ec5600be248eb430994R271

This defines makes v8::Handle not mapped anymore to v8::Local https://github.com/nodejs/node/blob/f0df222e821b0e3f55e8fdb7682d0d8dd9c69117/deps/v8/include/v8.h#L321-L325

I don't know SWIG codebase nor interactions with V8 API. I can try and hack something, but it might be not as good as one would like to.

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented Apr 25, 2019

This using has been introduced back in nodejs/node@70d1f32#diff-42f385dff7890b3c213d9699bcc350ccR336 i.e., with V8 4.4.63.9

According to https://nodejs.org/en/download/releases/ that means before NodeJS v4.0, with io.js v3.0.0

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented Apr 25, 2019

I have some local (likely not nice) patches that gets me something that works for our use

@wsfulton

This comment has been minimized.

Copy link
Member

@wsfulton wsfulton commented Apr 26, 2019

Could you put these into a pull request? Our integration testing on Travis will test various Node versions from 0.10 onwards and if they pass I'll merge them in. I'm afraid that the changes need to be backwards compatible in order to be accepted. BTW, the swig-4.0.0 release is going out in two days. If some older versions of Node can no longer be supported in order to make these changes there is a very small window for this as major releases like 4.0 only happen every few years (where old versions support can be dropped).

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented Apr 26, 2019

Our integration testing on Travis will test various Node versions from 0.10 onwards and if they pass I'll merge them in. I'm afraid that the changes need to be backwards compatible in order to be accepted.

That's the kind of constraint I was expecting, I guess it can be properly handled through preprocessor, but obviously it'll make the patch a bit messier.

Would opening a PR be enough for travis to run automagically and test that, without me being a collaborator on the repo ?

I'm unsure I can make it in the 4.0.0 window release anyway

@wsfulton

This comment has been minimized.

Copy link
Member

@wsfulton wsfulton commented Apr 26, 2019

Yes, just create a pull request and the Travis tests will automatically run. It is very compute intensive running the test-suite over 80 times for various different target language configurations, so please at least check the test-suite is okay with at least your version of Go beforehand.

Maybe some using statements to make the old versions look like the new versions? The preprocessor is bound to be required in some way.

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented Apr 26, 2019

It is very compute intensive running the test-suite over 80 times for various different target language configurations, so please at least check the test-suite is okay with at least your version of Go beforehand.

Hm I understand the need to avoid wasting resources, but this is for NodeJS, what's the link with Go ?

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented Apr 26, 2019

@wsfulton Before sending PR to travis, is there a better way to check for the compatibility than NODE_MODULE_VERSION ? I see a lot of places checking against V8_MAJOR_VERSION but to my understanding the move against v8::Handle has been done on NodeJS side rather than V8 side.

@wsfulton

This comment has been minimized.

Copy link
Member

@wsfulton wsfulton commented Apr 26, 2019

I'm afraid I'm not too familiar with the Javascript implementation, the Javascript versioning looks quite complicated, but there is something about it in the manual, see the Javascript.html file.

@mochizk

This comment has been minimized.

Copy link
Contributor

@mochizk mochizk commented Apr 26, 2019

Hi @lissyx. Thanks for creating the patch.
I tested your patch enabling node 12 test on Travis using my cloned repository.
https://travis-ci.org/mochizk/swig/builds/525054834
Node 0.10 and v8 are failing because these are older than v8 4.4.63.9. Node 12 is also failing because of other deparecate warnings.

BTW, the swig-4.0.0 release is going out in two days. If some older versions of Node can no longer be supported in order to make these changes there is a very small window for this as major releases like 4.0 only happen every few years (where old versions support can be dropped).

@wsfulton How about stop supporting the older version than Node 4? It makes very easier to maintain code. Node 0.12 End-of-life is 2016-12-31. So I think It does not cause any issues.
https://github.com/nodejs/Release#end-of-life-releases

If it is acceptable, we should also update v8 version used in CI as well as removing node 0.10 test. But I cannot find the newer version of v8 binary because apt v8 packages seem not actively maintained (https://packages.ubuntu.com/search?keywords=libv8) and there is no official prebuilt binary.

@mochizk

This comment has been minimized.

Copy link
Contributor

@mochizk mochizk commented Apr 26, 2019

@wsfulton Before sending PR to travis, is there a better way to check for the compatibility than NODE_MODULE_VERSION ? I see a lot of places checking against V8_MAJOR_VERSION but to my understanding the move against v8::Handle has been done on NodeJS side rather than V8 side.

In my understanding, NODE_MODULE_VERSION is not defined if using v8 directly without node, so it is better to check V8_MAJOR_VERSION.

@wsfulton

This comment has been minimized.

Copy link
Member

@wsfulton wsfulton commented Apr 27, 2019

Hm I understand the need to avoid wasting resources, but this is for NodeJS, what's the link with Go ?

Sorry, my brain was somewhere else, I meant NodeJS, not Go.

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented Apr 27, 2019

Thanks @wsfulton and @mochizk for your feedback

Failures on test-suite for node 0.10, v12 and v8 are expected, since it's a quick hack to unblock our usecase.

I'll try to make a better patch next week.

@wsfulton

This comment has been minimized.

Copy link
Member

@wsfulton wsfulton commented Apr 27, 2019

Hi @lissyx. Thanks for creating the patch.
I tested your patch enabling node 12 test on Travis using my cloned repository.
https://travis-ci.org/mochizk/swig/builds/525054834
Node 0.10 and v8 are failing because these are older than v8 4.4.63.9. Node 12 is also failing because of other deparecate warnings.

BTW, the swig-4.0.0 release is going out in two days. If some older versions of Node can no longer be supported in order to make these changes there is a very small window for this as major releases like 4.0 only happen every few years (where old versions support can be dropped).

@wsfulton How about stop supporting the older version than Node 4? It makes very easier to maintain code. Node 0.12 End-of-life is 2016-12-31. So I think It does not cause any issues.
https://github.com/nodejs/Release#end-of-life-releases

I think I'm beginning to understand the versioning now. https://nodejs.org/en/download/releases/ and https://en.wikipedia.org/wiki/Google_Chrome_version_history are useful tables of version numbers. The proposal is to support v8 >= 4.4.63.9, implying dropping support for NodeJS 0.12.x (end of life 2016-12-31 seems sort of reasonable), but support NodeJS>=4.0.0). It seems that NodeJS 4.0.0 is an important point in history as the io.js fork and NodeJS were merged to create this version.

The SWIG docs say that v8 didn't have a way to detect the v8 version before v8 4.3.0. 4.3.0 is quite close to the 4.4.63.9 version. Dropping the non-conventional approach of specifying the version to support when running SWIG, by running swig -DV8_VERSION is horrible, but can be scrapped if we limit v8 support to >= 4.3.0 or >=4.4.63.9. So I think we should remove this option too at the same time.

So in summary the proposal is to support v8 >=v4.4.63.9 which means NodeJS > 4.0.0. Pinging @oliver---- original Javascript maintainer to please contributes some wisdom please!

If it is acceptable, we should also update v8 version used in CI as well as removing node 0.10 test. But I cannot find the newer version of v8 binary because apt v8 packages seem not actively maintained (https://packages.ubuntu.com/search?keywords=libv8) and there is no official prebuilt binary.

Yes we need the CI tests for v8 to work before accepting the patch. If v8 binaries >= 4.4.63.9 can't be found maybe no-one uses v8 by itself??? Is it possible to test v8 by itself by installing NodeJS? Do we really need to support the -v8 option at all?

The 4.0.0 release is ready to go this weekend, but my dilemma is still what to do about Javascript, so any more insights gratefully accepted asap.

@mochizk

This comment has been minimized.

Copy link
Contributor

@mochizk mochizk commented Apr 28, 2019

I'm not sure -v8 option should be supported. node is static linked with v8, so we cannot use node to test v8.

@wsfulton

This comment has been minimized.

Copy link
Member

@wsfulton wsfulton commented Apr 28, 2019

Okay, so no clear simple solution. I'm going to release swig-4.0.0 tonight from master so for the swig-4.0.x series NodeJS 0.10 and above need to remain working. Alas the patch for adding NodeJS 12 support will thus be more complex for inclusion into swig-4.0.1 due to this extra backwards compatibility burden.

Dropping -v8 can be done in 4.1.0 if it makes sense. I don't have a full understanding on why it is needed or not needed.

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented May 5, 2019

I may try to find some time in the next week to augment my PR

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented May 21, 2019

Sorry, I have not yet been able to find time :/

@murillo128

This comment has been minimized.

Copy link

@murillo128 murillo128 commented Jul 8, 2019

is there any update or ETA for this?

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented Aug 20, 2019

is there any update or ETA for this?

Sorry, I'm busy with a lot of other things. Patches are shared above, and we have no problem running that, so it should just be a matter of applying those patches locally and polishing to get CI to pass.

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented Aug 20, 2019

@murillo128 I think the swig team can welcome help, if you can take my patches and get them in mergeable-state

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented Oct 9, 2019

FTR, working on supporting ElectronJS v6 revealed some more changes required to my patches. I've pushed updated version to https://github.com/lissyx/swig/tree/nodejs-v12-master but I'm unsure how to properly test for full coverage.

@wsfulton if you have any hints on how I could do that, my understanding is that tests on CI are ran with a v8 binary, and I have honestly no idea where to find that (I quickly tried to, but failed).

@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented Oct 9, 2019

@wsfulton There's just one element I really have a question about, cdata->handle.MarkIndependent();

While looking for what to use for newer versions, I stumbled upon https://monorail-prod.appspot.com/p/chromium/issues/detail?id=923361#c11 that would, according to my understanding, suggest that we don't need that anymore. Do you share that ?

@vadz vadz added the Javascript label Nov 5, 2019
@lissyx

This comment has been minimized.

Copy link
Author

@lissyx lissyx commented Nov 5, 2019

I've just force-pushed my branch with a newer version, there was some other items broken for ElectronJS v7.0 (seems to use V8 v7.8)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.