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

Add support to node 12.x and Gazebo 11 #205

Merged
merged 19 commits into from
Feb 9, 2021

Conversation

FelipeGdM
Copy link
Contributor

@FelipeGdM FelipeGdM commented Jan 22, 2021

Hello everyone,

From v8 version 6.0 (shipped w/ node 10) to v8 7.0 (shipped w/ node 12), there was a major change in API methods that prevented the project to run with node 12. Unfortunately, these changes break compatibility with node 8.x and 10.x as well, maybe it is interesting to release a major version of gzweb.

This PR addresses this problem and changes the calls to v8 API following the new syntax. No logic or functional changes were made.

Another change is the update to Gazebo version 11. It was necessary to pass the flag -std=c++17 in order tho compile the dependency sdf format version 9.0

Thank you everyone! =)

Useful references:

v8/node deprecated APIs and how to handle them bcoin-org/bcrypto#7
APIs removed in V8 7.0 and native addons nodejs/node#23122
[v10.x] deps: increase V8 deprecation levels nodejs/node#23159
Properly handle the requirement of C++17 at the CMake exported target level gazebosim/sdformat#251

Closes #204
Closes #203
Closes #174

@iche033
Copy link
Contributor

iche033 commented Jan 30, 2021

thanks for the pull request!

This works for me when building with node v12 + gazebo9. My only question is if we could ifdef the changes based on node version so users can still choose v10 if that works for them?

@FelipeGdM
Copy link
Contributor Author

Hi @iche033!

I have done some research in order to find the best way to achieve multi-version support (I am not an expert on writing node C++ addons 😝 ) and the cleanest way to do that seems to use the native abstractions for nodejs. From the project's README

Thanks to the crazy changes in V8 (and some in Node core), keeping native addons compiling happily across versions, particularly 0.10 to 0.12 to 4.0, is a minor nightmare. The goal of this project is to store all logic necessary to develop native Node.js addons without having to inspect NODE_MODULE_VERSION and get yourself into a macro-tangle.

A small refactor across the file will be necessary, but the code will be easier to maintain along node updates.

What do you think @iche033 ?

If you send a thumbs up, I may update the PR with these refactor, otherwise I may add ifdef statements to guard the problematic parts

@newcanopies
Copy link

@FelipeGdM @iche033

Thanks very much to both.
I applied:

d58203c
6d926df
9b49579
e907a39

however, building v12.20.0 against Gazebo11/Foxy/Focal does not work. Any clues?

`

npm start
[email protected] start /home/user/gzweb
if [ $npm_config_port ]; then port=$npm_config_port; fi; cd gzbridge && ./server.js $port

internal/modules/cjs/loader.js:818
throw err;
^

Error: Cannot find module './build/Debug/gzbridge'
Require stack:

  • /home/user/gzweb/gzbridge/server.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:815:15)
    at Function.Module._load (internal/modules/cjs/loader.js:667:27)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at Object. (/home/user/gzweb/gzbridge/server.js:9:18)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12) {
    code: 'MODULE_NOT_FOUND',
    requireStack: [ '/home/user/gzweb/gzbridge/server.js' ]
    }
    npm ERR! code ELIFECYCLE
    npm ERR! errno 1
    npm ERR! [email protected] start: if [ $npm_config_port ]; then port=$npm_config_port; fi; cd gzbridge && ./server.js $port
    npm ERR! Exit status 1
    npm ERR!
    npm ERR! Failed at the [email protected] start script.
    npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR! /home/user/.npm/_logs/2021-01-31T10_14_17_397Z-debug.log
`

@FelipeGdM
Copy link
Contributor Author

@newcanopies, it seems that the error is happening when you build the project, may you you post the output of npm run deploy?

I would be happy to help, but I would ask you to post this within an issue (like #204), as troubleshooting is not the main purpose of a pull request

This is a pure cosmetical change.

From the docs,

"Handle is an alias for Local for historical reasons"

https://v8docs.nodesource.com/node-10.15/d4/da0/v8_8h_source.html#l00428
Several API calls from V8 were deprecated in version 7.0
(that ships w/ node 12), this commit replaces then

Refs:

nodejs/node#23122

nodejs/node#23159

bcoin-org/bcrypto#7
Sdf format version 9.0 requires c++17 to be compiled

gazebosim/sdformat#251
@iche033
Copy link
Contributor

iche033 commented Feb 1, 2021

cleanest way to do that seems to use the native abstractions for nodejs.

thanks for looking into this. This also led me down to some light reading on the different abstractions for nodejs. So there is NAN, and there is also N-API and its C++ wrapper node-addon-api. My understanding is that NAN is more mature and supports older verions of nodejs but more projects are starting to transition to using N-API (or its C++ wrapper). The downside of using N-API is that it's only available in nodejs version 8 (introduced as experimental) and above.

I'm fine with either of these options. I'm not sure how much effort it'll take but if you're up for the task, I'm happy to review the changes and get it into gzweb!

@FelipeGdM
Copy link
Contributor Author

Since both options solve our problem, compatibility with older versions of node is a desirable feature and the work needed to migrate to nan is considerably simpler than migrating to N-API (due to the high similarity between nan and plain v8 calls), I decided to migrate to nan

As stated in this StackOverflow answer

use nan if you want something mature and very backwards-compatible

This article from MongoDB developers illustrates the work needed to migrate from nan/v8 to N-API

That said, the code structure remains the same, but several method calls were replaced by its nan counterpart.

I was able to run with node v8.17.0, v10.23.2, v12.20.1 and v14.15.4

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

Sounds good to me. Let's go with nan

I tested with node v8, 10, and 12 with gazebo9 on Bionic and they all work fine!

@iche033 iche033 merged commit d6c8fbf into osrf:master Feb 9, 2021
@rubenanapu
Copy link

Awesome. I was struggling to have a custom gzweb compiled for Gazebo 11 until I found this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants