Skip to content

Conversation

@nxhack
Copy link
Contributor

@nxhack nxhack commented May 3, 2021

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 [email protected]

@Propanu
Copy link
Contributor

Propanu commented May 3, 2021

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
Copy link
Contributor Author

nxhack commented May 6, 2021

@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 nxhack force-pushed the cleanup_V8_MAJOR_VERSION branch from fda8d88 to 454bd9a Compare May 8, 2021 01:48
@nxhack
Copy link
Contributor Author

nxhack commented May 8, 2021

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 eclipse-upm/upm#703

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 <[email protected]>
@nxhack nxhack force-pushed the cleanup_V8_MAJOR_VERSION branch from 454bd9a to f119e36 Compare May 8, 2021 08:41
@nxhack
Copy link
Contributor Author

nxhack commented May 9, 2021

@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
Copy link
Contributor

Propanu commented May 20, 2021

@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
Copy link
Contributor Author

nxhack commented May 20, 2021

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

@nxhack
Copy link
Contributor Author

nxhack commented May 20, 2021

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

Copy link
Contributor

@Propanu Propanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying this will work with the latest versions of Node.js and SWIG 4.1.0 as is.

@g-vidal
Copy link
Contributor

g-vidal commented May 30, 2021

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
Copy link
Contributor

g-vidal commented Jun 3, 2021

@Propanu I can see it is approved but I confirm that the patch works perfectly with the following context
Many many thanks again 👍 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
Copy link
Contributor

g-vidal commented Oct 10, 2021

Checked the patch for mrra with node 16.11.0 . Works perfectly

@tingleby tingleby merged commit 046bdd0 into eclipse:master Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants