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

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

0003-Move-to-new-MaybeLocal-based-GetFunction.patch.txt
0002-Move-to-new-MaybeLocal-based-NewInstance.patch.txt
0001-Move-from-v8-Handle-to-v8-Local.patch.txt

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).

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

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.

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 ?

@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.

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.

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.

@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.

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.

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.

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.

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

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.

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

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

is there any update or ETA for this?

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.

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

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).

@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 ?

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)

Ping @wsfulton ?

FTR, tried opening a PR #1702 but it seems that no travis job is actually getting scheduled. Lots of them are created, nothing made any progress in the last 3-4 hours.

FTR, tried opening a PR #1702 but it seems that no travis job is actually getting scheduled. Lots of them are created, nothing made any progress in the last 3-4 hours.

It seems it was just Travis being slow: this actually started to build.

There are still Travis oddities I cannot figure out and honestly my window for time to hack on that is narrowing fast. Any help to move #1702 is welcome, and as much as I tested it was more or less working locally.

where can we check the travis errors? El vie., 28 feb. 2020 20:24, lissyx notifications@github.com escribió:
…
There are still Travis oddities I cannot figure out and honestly my window for time to hack on that is narrowing fast. Any help to move #1702 <#1702> is welcome, and as much as I tested it was more or less working locally. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1520?email_source=notifications&email_token=AAIFN43LCGIKODCNS7W35LLRFFQFRA5CNFSM4HILMZPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENJ23RI#issuecomment-592686533>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIFN44ENV326YXYBXZYIKTRFFQFRANCNFSM4HILMZPA .

Travis link on the linked PR ?

@wsfulton Thanks for making this move forward! FTR, I've updated my branch on our use for DeepSpeech there: https://github.com/lissyx/swig/tree/merge-tc and as you can see, it differs from your master only for TaskCluster bits and my Travis tentatives.

Our CI is having some troubles with our macOS builders, but aside from that I can report that as of https://community-tc.services.mozilla.com/tasks/groups/eJU72JgJQg2-jkVpV2Twag we have all NodeJS tests upto v15.x that are green.

Now, our tests only covers our usecases so I can't advocate for the general level for SWIG, but it's likely going to unblock a lot of people! Thanks again @wsfulton

I'm sorry it took so long. The main problem is not having a Javascript maintainer. @lissyx , if you are maintaining a minor fork of SWIG, I invite you to please instead consider taking over being the official SWIG Javascript maintainer to benefit all Javascript users.

Your Javascript tests look like they run SWIG's test-suite with additional Javascript targets, plus you have some tweaks for electron including source code changes for this.

I'm sorry it took so long. The main problem is not having a Javascript maintainer. @lissyx , if you are maintaining a minor fork of SWIG, I invite you to please instead consider taking over being the official SWIG Javascript maintainer to benefit all Javascript users.

Your Javascript tests look like they run SWIG's test-suite with additional Javascript targets, plus you have some tweaks for electron including source code changes for this.

I agree in general, however i dont have enough v8 knowledge and time available to really step up as maintainer, sadly.

Your Javascript tests look like they run SWIG's test-suite with additional Javascript targets, plus you have some tweaks for electron including source code changes for this.

Yeah, well, with each new version of NodeJS I discovered limitations in my original patches, so I would really take the results of our test suite only for the coverage of deepspeech.

I'm sorry it took so long.

Just to be clear, I was not trying to complain or anything about the duration, maintainership is a hard problem and I would really have loved being able to help more than I did. If you felt offended, please accept my apologies.