-
-
Notifications
You must be signed in to change notification settings - Fork 310
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
Feature - Bring back node support #217
Feature - Bring back node support #217
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this @jutaz !! 👏
We maintain a library / server based on the Mapbox GL JS bindings, and have been hopeful that someone would be able to made appropriate updates here so we can move to newer NodeJS versions.
I had one question below that impacts the request handlers that users pass in as part of the options, but otherwise this looks like it should be a fairly seamless update for end-users, once the packaging gets worked out. Is that correct, or are there other interface changes here that users should be aware of? I'm assuming the libuv
work here is entirely internal and doesn't impact use as a NodeJS package, correct?
(not qualified to review the NodeJS C++ bindings...)
@brendan-ward yeah, this change should mostly be free from any client-side changes. My most important thing is to make sure this works in existing systems (which is why I’m making this change in the first place). |
Looks like we've needed |
Previously added to CircleCI, which is no longer in use
Hi @jutaz, I'm still a bit sceptical that you covered all of the CMake changes. Do you mind that I try to setup a test also for the Qt build over the weekend and then we re-run that everything works as expected? |
Sure thing @ntadej! Please do, I'd appreciate it |
I am interested in keeping the TileServer-GL stuff working, so this is great to see progress here. My notes and feedback based on attempting to test Node bindings using the notes from #217 (comment) (above) Feedback:
Node bindings build log ❌-- The CXX compiler identification is AppleClang 13.0.0.13000029 -- The C compiler identification is AppleClang 13.0.0.13000029 -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ - skipped -- Detecting CXX compile features -- Detecting CXX compile features - done -- Detecting C compiler ABI info -- Detecting C compiler ABI info - done -- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc - skipped -- Detecting C compile features -- Detecting C compile features - done -- Configuring GL-Native with OpenGL renderer backend -- Found OpenGL: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/System/Library/Frameworks/OpenGL.framework -- Found PkgConfig: /usr/local/bin/pkg-config (found version "0.29.2") -- Checking for one of the modules 'glfw3' -- [Node.js] Downloading Node.js version list... -- [Node.js] Downloading Nan 2.10.0... -- [Node.js] Downloading headers for Node v17.4.0... -- [Node.js] Downloading headers for Node v16.13.2... -- [Node.js] Downloading headers for Node v15.14.0... -- [Node.js] Downloading headers for Node v10.24.1... -- [Node.js] Downloading headers for Node v8.17.0... -- Configuring done -- Generating done -- Build files have been written to: /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/build [1/618] Building CXX object test/CMakeFiles/mbgl-test.dir/util/bounding_volumes.test.cpp.o [2/618] Building CXX object test/CMakeFiles/mbgl-test.dir/util/dtoa.test.cpp.o : snip : [321/618] Building C object CMakeFiles/mbgl-core.dir/platform/darwin/src/native_apple_interface.m.o [322/618] Building CXX object platform/node/CMakeFiles/mbgl-node.abi-102.dir/src/node_logging.cpp.o FAILED: platform/node/CMakeFiles/mbgl-node.abi-102.dir/src/node_logging.cpp.o ccache /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DBUILDING_NODE_EXTENSION -DGL_SILENCE_DEPRECATION -DMODULE_NAME=mbgl_node -DRAPIDJSON_HAS_STDSTRING=1 -D_DARWIN_USE_64_BIT_INODE=1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Dmbgl_node_abi_102_EXPORTS -I/Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/include -I/Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/platform/default/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/build/headers/node/v17.4.0 -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/build/headers/nan/2.10.0 -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/extras/expected-lite/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/deps/geojson-vt-cpp/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/deps/geojson.hpp/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/deps/geometry.hpp/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/deps/jni.hpp/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/deps/optional -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/deps/pixelmatch-cpp/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/deps/shelf-pack-cpp/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/deps/supercluster.hpp/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/deps/variant/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/deps/cheap-ruler-cpp/include -isystem /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/vendor/mapbox-base/extras/rapidjson/include -g -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk -mmacosx-version-min=10.11 -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -std=c++11 -std=c++14 -MD -MT platform/node/CMakeFiles/mbgl-node.abi-102.dir/src/node_logging.cpp.o -MF platform/node/CMakeFiles/mbgl-node.abi-102.dir/src/node_logging.cpp.o.d -o platform/node/CMakeFiles/mbgl-node.abi-102.dir/src/node_logging.cpp.o -c /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/platform/node/src/node_logging.cpp In file included from /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/platform/node/src/node_logging.cpp:1: In file included from /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/platform/node/src/node_logging.hpp:10: In file included from /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/build/headers/nan/2.10.0/nan.h:202: In file included from /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/build/headers/nan/2.10.0/nan_converters.h:67: /Users/roblabs/Documents/github/mapbox/tmp/robinpowered/maplibre-gl-native/build/headers/nan/2.10.0/nan_converters_43_inl.h:22:1: error: no viable conversion from 'Local' to 'v8::Isolate *' X(Boolean) ^~~~~~~~~~ Build Environment used for Node testing
|
As far as I can tell - yes. We might be able to cut that down, but I'd like to make that a follow up rather than a requirement from a get-go. I assume you've been able to run the following commands to get the deps?
Nope no XCode for this stuff, just pure C++ compilation. I also have a handy FROM debian:bullseye-slim AS builder
RUN apt-get indextargets
ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update && apt-get dist-upgrade -y && apt-get autoremove --purge -y && apt-get clean
RUN apt-get install -y ca-certificates git gzip ssh tar colorized-logs python3-scipy && apt-get autoremove --purge -y && apt-get clean
RUN apt-get install -y awscli ccache clang-9 clang-format-9 clang-tidy-9 cmake doxygen fonts-noto g++-10 libc++-9-dev libc++abi-9-dev mesa-common-dev ninja-build nodejs npm pkg-config python3-bs4 python3-pip python3-requests python3-git python3-github software-properties-common valgrind xvfb zip && apt-get autoremove --purge -y && apt-get clean
RUN pip3 install cmake-format==0.5.5
RUN apt-get install -y libcurl4-openssl-dev libgl1-mesa-dev libgles2-mesa-dev libglfw3-dev libicu-dev libjpeg62-turbo-dev libpng-dev libuv1-dev zlib1g-dev && apt-get autoremove --purge -y && apt-get clean
RUN git clone --recursive https://github.com/google/bloaty.git /tmp/bloaty && cd /tmp/bloaty && git checkout 3cf5c3feca15 && cmake . && make -j $(nproc) bloaty && cp bloaty /usr/local/bin/ && rm -rf /tmp/bloaty && rm -rf ~/.ccache
# RUN apt-get install -y qdoc-qt5 qt5-default && apt-get autoremove --purge -y && apt-get clean
# RUN apt-get install -y coreutils curl openjdk-8-jdk-headless unzip && apt-get autoremove --purge -y && apt-get clean
RUN apt-get install -y g++-10 && apt-get autoremove --purge -y && apt-get clean
# RUN apt-get install -y python-yaml && apt-get autoremove --purge -y && apt-get clean
RUN /usr/sbin/update-ccache-symlinks
RUN mkdir -p /src
WORKDIR /src
FROM builder AS buildable
COPY . /src/
RUN cmake . -B build -G Ninja -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_C_COMPILER=gcc-10 -DCMAKE_CXX_COMPILER=g++-10
RUN ccache --zero-stats --max-size=2G
RUN cmake --build build -j $(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null)
RUN ccache --show-stats |
I still use the multiple node versions for the npm publish, since I am only doing that once. that one publish uploads all the versions, so it works out good. |
I'd love to see some of my tileserver-gl fork get merged back, but I fear I may have diverged to far too since I converted it to a module and changed all the requires to imports and reworked it to make that work. we will see though once I figure out the last label trimming issue I have #284 |
For EGL vs GLX, there was an old pull request ( mapbox/mapbox-gl-native#5843 ), is that pretty much what you tested? On the tileserver-gl side they where talking about EGL and using it to run headless here ( maptiler/tileserver-gl#71 ) |
As a point of reference, I'm using EGL rather than GLX for pymgl (Python wrapper around maplibre-gl-native for static rendering); this has been working fine for Ubuntu 20.04 but still requires using Xvfb for headless rendering (not enough of an expert on how GLX vs EGL works to know if it shouldn't need Xvfb). See GH test action in case it helps. |
I saw that noted in the tileserver-gl thread I linked too.
|
Does the "MBGL_WITH_EGL" build option work. Is that switch all thats needed to compile this with EGL instead of GLX? |
We use that option for our intern server side tile renderer to work around a problem we had with the installed xvfb and the default method to get a display. We still need xvfb to start the server but it does use headless_backend_egl instead of headless_backend_glx on the linux platform which worked around our problem. |
Yeah I was thinking we could perhaps compile using |
I had this issue from dwoznicki where he was having issues with GLX on his amd graphics card trying to use a real X Server instead of xvfb. I was just wondering is maybe MBGL_WITH_EGL would work around his issue. He also had this issue here about it #282 |
@jutaz I was wondering about the motivation behind 9c85518 (Simplify node loop). My understanding is that |
Hey @mnutt, I'm interested in trying this out and wondering, should I begin this, is there any Nodejs documentation or any material to help describe the APIs made available? |
Oh if you find some node API docs let me know. I would like to publish them on our website or so... |
@mnutt thanks for working on this! I tried to install your version It appears to work for nearly all our tests (our label tests fail due to labels being positioned differently now), but I get a ton of errors on my console: profiling: /Users/runner/work/maplibre-gl-native/maplibre-gl-native/build/CMakeFiles/mbgl-core.dir/src/mbgl/actor/mailbox.cpp.gcda: cannot open: No such file or directory
profiling: /Users/runner/work/maplibre-gl-native/maplibre-gl-native/build/CMakeFiles/mbgl-core.dir/src/mbgl/actor/scheduler.cpp.gcda: cannot open: No such file or directory
... |
I saw those too when terminating node. They look like the result of profile-guided optimization; not sure why it’s turned on but should be turned off by some cmake config. |
@wipfli if there are docs for the mapbox version they should be the same for this. |
I had a real hard time finding anything documentation wise. For my label issue ( #284 ) test application, I basically searched until I found some example code that worked, like https://gist.github.com/ezheidtmann/c869f89b6c286082e84c5615311ebd80 . I was also comparing with the tileserver-gl code and a few other examples, but this one ended up to be the one that worked for me. Then for that test application, I was trying to get two tiles that were side by side. For that I basically had to do manual math to calculate two centerpoints. Basically I used 'Tile width |
I believe what you're describing is also what tileserver-gl does: https://github.com/maptiler/tileserver-gl/blob/c89a5ae029f5e9ef0855292cfcce2f72894a8fff/src/serve_rendered.js#L381-L387 |
It is, I was trying to show an issue I was having with my tileserver-gl that uses maplibre node |
Sorry yes I just read through #284, you can disregard my comment. |
The problem was I updated tileserver too much, so I was no longer able to use it with the mapbox-gl-native package. I wrote this test app just to be able to show the issue in a testable way and be able to switch between the mapbox/maplibre version and see the results in both. Anyway, my main point was not to ruthlessly promote my issues...haha... but more, the documentation out there that I found was not very good. Maybe someone knows where there is some, but I pretty much had to go through old examples to figure it out. Edit: are there actually docs for the mapbox version somehwere? |
I did not observe these when building this PR locally (see above) on macOS 10.15. Perhaps something additional is turned on in the configuration you used? I didn't see anything obvious in your changes but perhaps the (sidenote: is there a different issue where we should take up NodeJS module publishing related issues, as this PR has been closed a while now, and these are follow-on issues?) |
The MapLibre Governing Board would like to give a thank-you payment to the people who helped with bringing back node support. I already reached out to some of you on slack but I could not find @mnutt and @METACEO. If you are interested in receiving a payment, please contact me on Slack. You can join the slack channel here: https://osmus-slack.herokuapp.com/ Thanks for helping with this! I really appreciate it... |
This PR brings back support for node.js later versions, as well as actually making it work. Maps now render, properly, using node v10, v12, and v16.
Do the docs need some updating? https://github.com/maplibre/maplibre-gl-native/tree/main/platform/node
If you try and install this in a regular linux container you can install, but you can't require the code:
We had to install all of these libraries to get it functional:
We also had to run it with I realize you decided against statically linking all these files. It could be really useful to have a docker container pre-configured with everything you need for all this? |
I'd say it needs updating. Currently we are running the node binary builds on node 18 and we are creating binaries for Node 14,16,18 . I'm not sure what the actual minimum is considered, but I'd say at least Node 16 is recommended. If you are using npmjs package with the pre-built binaries, the binaries are built on Ubuntu 20.04 amd64/arm64 and MacOS 12 amd64/arm64. The linux binaries end up with some hard dependencies that require packages only available in 20.04, so they kind of require Ubuntu 20.04 right now. I'm not sure if MacOS 12 binaries have issues like that, but I assume they won't work on older systems. To use the package in a workflow, you need it to be ubuntu 20.04 and have installed server prerequisite packages. For that GLX error it would be fixed by installing libgles2-mesa-dev. For example, I install these packages in my Tileserver-gl test workflow, which uses maplibre-native node https://github.com/maptiler/tileserver-gl/blob/master/.github/workflows/ct.yml#L34-L36 |
By the way, the preconfigured docker image is basically how tileserver-gl does it. I made this workflow to make a x64/arm64 release in a similar way |
This PR brings back support for node.js later versions, as well as actually making it work. Maps now render, properly, using node v10, v12, and v16 (these are the ones I've tested, sure others work too).
Testing
A good way to test is to build this on your platform, and then use sample code from
platform/node/README.md
to render the map. Additionally, here's a simple way to build on macos (x86 at the moment):Questions
What would it take to build and publish
@maplibre/maplibre-gl-native
to NPM, equivalent to what exists from Mapbox today?Credits
It's been inspired and heavily relied on work in this fork: https://github.com/mnutt/maplibre-gl-native/tree/node-14, specifically the
libuv
integration.