Add Node 7.x aka V8 5.2+ support #968
Conversation
* Use WeakCallbackInfo instead of WeakCallbackData * Use GetPrivate instead of GetHiddenValue * Adopted new signature for SetWeak to support destructor calling * SetAccessor deprecation fixed * Proper version checks where applicable
|
Provides a more complete solution for #804 |
|
@p2k no offence taken - my commit was labelled a hack ;-) This is really cool, I'll test it and see if it works for my usecase next week. |
|
@p2k thanks for taking over and getting this done right. No offense taken. I'm certainly not an expert with v8, I'm just excited to get my wrapper to work properly. |
The proposed changes targetted at 5.2 (or 5.4 to be more precise, since there is no Node release with V8 5.2 or 5.3) work for lower versions as well and bust the deprecation warnings there.
|
I've had hours and days (added up) of fun with the oh so great V8 and I'm more than happy to have finally found SWIG which ends my glue code nightmares all at once. As soon as you have multiple projects that require Node wrappers (or any language) it gets obvious that it's easier to keep a code generator maintained than fixing everything over and over again. |
|
Thanks, we used this PR for our project and it builds and works fine. It would be great it that was merged soon, because the SWIG Node.js generator, as it currently ships from many sources, is broken and can't be used to build Node.js modules with distributed Node.js / V8 versions. I think this is a nice and proper fix, and even it there was some issue, it would still be better than not being able to build at all. Version checking macros ensure that the behavior remains the same with older V8 versions. Having this in master would also give it more visibility so it would be tested by more people before an eventual release. |
|
This PR also fixes our problem we had with node 7.x. |
|
+1
… On May 30, 2017, at 6:59 AM, Mislav Novakovic ***@***.***> wrote:
This PR also fixes our problem we had with node 7.x.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@ojwb, could you please tell when this PR will be merged to the master? |
|
The patch looks sane to me, but I know next to nothing about nodejs, and ideally I'd like to see CI coverage for this so I have confidence that it works. That'd also help ensure it doesn't get accidentally broken by a future patch being applied. As best I can make out, travis is currently testing a really old version: (At least I assume 0.10.36 < 7.x!) That build appear to be on Ubuntu precise still - adding |
|
You could use nvm to manage to install different node versions for CI. |
|
Successfully tested in Buildroot (swig 3.0.12 and Node.js 8.5). |
|
Is there anyone with C/C++ and Javascript experience who would like to step forward and maintain the Javascript module? Without a maintainer, it is hard to know if the various Javascript improvements like this one can't be added. |
Recent Node.js versions have removed some depreciated API calls, that swig still relies on. Patches taken from this PR [1] fix this issue. [1] swig/swig#968 Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
|
Hi! current support for node 7 & 8 is completely broken, so why not apply this patch and see what happens? I understand that it is far from ideal, but it won't work worse than now. |
|
+1 |
incorporating swig#968
|
@murillo128 I think we really need CI coverage for the newer node versions. Otherwise "what happens" will be that someone will propose a patch that accidentally breaks support for them, and we'll merge it and not spot the breakage until some some time later when it's harder to deal with. Ideally someone would step up to actually maintain the node backend, but without someone in that role CI coverage is particularly important. |
|
@ojwb I agree, but in your worst case we will be at the same place as we are currently.. it can't get worse than now that it is broken.. ;) |
|
@p2k Indeed, thanks. Note that @nshmyrev has opened #1208 for sorting out travis, so you should probably coordinate to avoid duplicated work there. We try to tag issues and PRs which are target language specific, so you can see all the open issues for a particular target language (or at least those that have been tagged): There's only a "Javascript" tag at present, but in practice these are almost all about node. |
|
Can do. I also did see the PR and I'm going to put further conversation about the topic there. "Javascript" basically covers the v8 engine which is used in Node.js, Electron and Safari plugins. I have worked with all three of these, so I see no problems there, should a specific issue arise. Also I'll simply be honest if I don't know something and can't do the research myself. So I'll basically monitor the Javascript tag and any tasks that are assigned to me or where I get mentioned. Github sends me an email and depending on timezones/vacation/weekend I can probably react within 24h. That's what I can offer. Details follow on the mailing list tomorrow. |
|
The SWIG manual says that SWIG's Javascript backend also supports "JavascriptCore, the Javascript engine used by Safari/Webkit" which seems to be an independent Javascript implementation to v8. It seems to be hooked up in the CI. Do Safari plugins really use a different Javascript engine to Safari itself? That seems surprising (and I can't find evidence of it online). |
|
I'm going to double check this asap. v8 is courtesy of Google, but open source; it might still not be a part of WebKit/Safari in the near future. PS: A project in my company recently escalated, which forces me to delay my application on the SWIG mailing list. I should be ready in one or two days. |
|
AIUI, Safari's Javascript engine is also FOSS but derives from KDE's KJS not v8. But really having someone actively maintaining only the v8-based Javascript targets in SWIG would be a big improvement, and the CI should at least ensure that JSC support doesn't get accidentally broken by v8-related changes. |
|
hello any process about this PR. NODE 9 can not build. |
|
Seems odd that the official reason this isn't being merged is the lack of tests ("it might not work") when the bindings swig currently generates are useless for recent node versions ("it definitely does not work"). Forgive me if I'm wrong, but this looks more like the maintainers don't use nodejs and therefore don't care if this gets merged or not. Why not at least merge this as a separate "node7+" target so that people can use it without having to merge a patch and compile swig from source? |
|
It's also stuck, because the guy that wants to become maintainer (and does use nodejs and does care) is still under arrest by high work load in his company. |
|
This is simply a nonsense. SWIG does not support neither current nodejs version (v9) nor LTS (v8). Claiming that this patch breaks compatibility with unsupported node version is the worst excuse ever. If you have a node<v7 project it is expected that things stop working for you, swig or any other dependency. |
|
All we're asking for is to have CI coverage for the new node versions that this patch aims to support in order to demonstrate that the patch works, and to ensure that this support isn't broken by future patches. I'm frankly amazed at the amount of time and energy people seem prepared to put in to arguing for us to merge this patch without having any CI coverage - far more in total now than it would have taken for someone to just sort the necessary CI changes. |
|
FWIW node 10.0.0 it's going or tomorrow.. So swig will not support node 7,8,9 and 10. Regarding CI , is there any support for node right now? What CI tests do you expect to be added and what should they cover? How can the CI changes be tested easily? |
|
We use Travis and Appveyor for CI. The Linux Travis environment is preferred for testing, so if someone can provide the 'sudo apt-get install' command for installing node 7,8,9 and 10 on Ubuntu 14.04 (Trusty Tahr), this can be set up in the Travis environment easily as follows. An addition to the block of code at Line 54 in d612c6a env: SWIGLANG=javascript ENGINE=node VER=7
sudo: required
dist: trustyand then a change to install the package is required in swig/Tools/travis-linux-install.sh F438 Line 40 in d612c6a "node")
if [[ -z "$VER" ]]; then
travis_retry sudo apt-get install -qq nodejs node-gyp
else
# sudo-apt-repository etc ...
# apt-get install etc...
# or whatever is required to install nodejs version ${VER}
fi
;;Testing on a local box ought to be done first to check... Run the node install commands on a 14.04 machine, grab SWIG and run the usual configure and build and test-suite like this: ./autogen.sh && ./configure && make -s && make check-javascript-examples ENGINE=node && make check-javascript-test-suite ENGINE=nodeIf you click on the "Show All Checks" link in this pull request, you can navigate your way to the nodejs test run by this pull request (currently https://travis-ci.org/swig/swig/jobs/221819686). You will see among the output the versions installed which is the default installed by the install commands mentioned in travis-linux-install.sh: |
|
Node 0.10.36 is from 2015... The nodejs packaged in ubuntu is unusable. The best way of installing specific node versions is using nvm (which do not require root): https://github.com/creationix/nvm Then installing & using an specific node version is: So the question is, is that enough? |
Ideally we're looking for an updated and tested But the information you provide above is certainly a step forwards, so thanks for that. |
|
I don't have enough knowledge of travis to make the modifications neither I know how to actually test the changes. I can help you with the node setup info, but can't provide the full PR, sorry (I would already have provided it otherwise) |
|
so, for someone who just looked at swig and node-gyp for the first time today and is wrapping a rather complex shared object, is there an easy way to fix this on my local box? I am fairly certain that I compiled the latest distribution. I'd love to jump in and see what I can do with travis and ci coverage, cause I planned on experimenting with that technology here as well. But, alas, I must find a way to consume this shared object in my electron app as a top priority. really don't want to go the FFI route based on what I have read and the time I spent making the choice between the two. Thanks! |
|
@RebelSyntax just copy the updated swg files to your swig installation. |
|
node 6 was messed up because there's a bug #elifif instead of #elif in the 4'th file to be patched. |
|
Merged via #1290. Please check using SWIG master. |
This pull request is similar to the one from @tomleavy (i.e. @arfoll) but with less "hack" (no offence). It also fixes more warnings and resolves the apparent memory leak from not calling destructors. Credits go to the mentioned users for their inspiration.