eclipse / mraa

Linux Library for low speed IO Communication in C with bindings for C++, Python, Node.js & Java. Supports generic io platforms, as well as Intel Edison, Intel Joule, Raspberry Pi and many more.
http://mraa.io
MIT License
1.37k stars 614 forks source link

[javascript][v8] Support for swig 4.1.0 #1064

Closed nxhack closed 2 years ago

nxhack commented 3 years ago

Support for swig 4.1.0

In swig 4.1.0, the complicated handling of "SWIG_V8_VERSION" has been cleaned up a bit. I made the same changes as in this swig.

Signed-off-by: Hirokazu MORIKAWA morikw2@gmail.com

Propanu commented 3 years ago

Thanks for submitting a fix for #1040 and #969. I'd be really happy if someone can see this all the way so we can reenable node.js bindings. My only question, does this also need the SWIG changes to work?

nxhack commented 3 years ago

@Propanu

SWIG 4.0.2 does not work with node v12.x (v8 7.8) & higher, so I verified it with node v10.x (v8 6.8). I have also verified that it works with SWIG 4.1.0. No problems were found in either case.

node v10.24.1 (v8 6.8.275.32) (EoL)

swig v4.0.2
mraa v2.2.0 + patch   -> OK
upm v2.0.0 + patch    -> OK
swig v4.1.0 ((in progress)
mraa v2.2.0 + patch -> OK
upm v2.0.0 + patch  -> OK

I will test node v12.x v14.x v16.x. It may take some time.

nxhack commented 3 years ago

I'm adding modifications that I found while testing to the mraa and upm patches.

node 12.22.1 (v8 7.8.279.23)

swig v4.1.0  (in progress)
mraa v2.2.0 + patch -> OK
upm v2.0.0 + patch  -> OK

node v14.16.1 (v8 8.4.371.19)

swig v4.1.0  (in progress)
mraa v2.2.0 + patch -> OK
upm v2.0.0 + patch  -> NG (need WERROR=OFF)

node v16.1.0 (v8 9.0.257.24)

swig v4.1.0  (in progress)
mraa v2.2.0 + patch -> NG (need -std=c++14 flag) (Change CMakeLists.txt)
upm v2.0.0 + patch  -> N/A (Set CMAKE_CXX_STANDARD=14 & Change CMakeLists.txt)

upm: ref https://github.com/eclipse/upm/pull/703

nxhack commented 3 years ago

@Propanu

My only question, does this also need the SWIG changes to work?

I've heard from the SWIG maintainers and while I'm not convinced about the need to support the obsolete V8, I understand it.

SWIG is still as it is for now. The handling of V8_VERSION is complicated. I have modified the V8 related code to work with SWIG on the MRAA side and the UPM side.

Propanu commented 3 years ago

@nxhack thank you for checking the latest node.js versions. You will need to accept the Eclipse Committer Agreement (ECA) in order for us to merge these changes. It would also help to add a patch file in the MRAA project with the SWIG changes so we can apply that during our CI checks, but that's something we can worry about later.

nxhack commented 3 years ago

@Propanu I agree with ECA, CI is now green.

nxhack commented 3 years ago

@Propanu I will not make any changes to SWIG, as I have heard their implementation policy. In the upcoming SWIG 4.1.0, MRAA v8 related code with this PR will be able to build successfully.

g-vidal commented 3 years ago

I am very sorry not to be able to follow regularly your work. I am building raspberryPi debian Oses with mraa and upm at the moment. I can spend some time on tests. I feel that we are very close to a clean build. Let me know what tests you want me to make; my goal is to build in a bullseye with the most recen node and swig if possible.

Test OK node16 commented in #1040

g-vidal commented 3 years ago

@Propanu I can see it is approved but I confirm that the patch works perfectly with the following context Many many thanks again :+1: to @nxhack for the great work and for the git training session ;-) . Here are the version used and what I did :

Linux myPi 5.12.6-v7 #1 SMP Mon May 31 18:19:07 CEST 2021 armv7l GNU/Linux
SWIG Version 4.1.0
gcc version 10.2.1 20210110 (Debian 10.2.1-6)
node v16.2.0
npm 7.15.0

cmake .. -Wno-dev -DCMAKE_INSTALL_PREFIX=/usr -DENABLEEXAMPLES=0 -DBUILDSWIGNODE=ON -DSWIG_EXECUTABLE=/usr/local/bin/swig -DV8_ROOT_DIR=/usr/local/include/node -DNODE_ROOT_DIR=/usr/local/include/node     -DPYTHON2LIBS_FOUND=FALSE        -DPYTHON2INTERP_FOUND=FALSE   -DWERROR=OFF -DCMAKE_CXX_STANDARD=17
g-vidal commented 2 years ago

Checked the patch for mrra with node 16.11.0 . Works perfectly