Build Node bindings on Windows #6334

SiarheiFedartsou · 2022-08-25T16:12:37Z

Issue

This PR extends CI to build NodeJs bindings for Windows.

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation)
review
adjust for comments
cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

SiarheiFedartsou · 2022-09-09T14:11:52Z

.github/workflows/osrm-backend.yml

+    - name: Prepare environment
+      shell: bash
+      run: |
+        PACKAGE_JSON_VERSION=$(node -e "console.log(require('./package.json').version)")


I was not able to check how it actually works with tag(because in order to do that I actually need to push a tag), but it at least sets PUBLISH=Off

SiarheiFedartsou · 2022-09-09T14:12:16Z

.github/workflows/osrm-backend.yml

+        node test/nodejs/index.js | faucet
+    - name: Build Node package
+      if: ${{ env.PUBLISH == 'On' }}
+      run: bash ./scripts/ci/node_package.sh


Checked that it works via manually setting PUBLISH=On

Probably worth removing the conditional like you've done in #6360?

Agree, will do 👍🏻

SiarheiFedartsou · 2022-09-09T14:13:10Z

.github/workflows/osrm-backend.yml

+      run: bash ./scripts/ci/node_package.sh
+    - name: Publish Node package
+      if: ${{ env.PUBLISH == 'On' }}
+      uses: ncipollo/release-action@v1


Couldn't it check as well, but I expect that it should work as on other Linux/macOS jobs.

You can test it on your own fork by publishing some dummy versions. I'll give it a try on mine.

Hmm, well the Windows build was published, which is great.
https://github.com/mjjbell/osrm-backend/releases/tag/v9.90.22

But the Linux packaging failed.
https://github.com/mjjbell/osrm-backend/actions/runs/3063972998/jobs/4946640880

Not sure if it's related to these changes, looks like it might be a Conan issue.

I think I fixed it in #6360 (via RPATH changes, tbh I am not sure that it is caused by Conan, but may be)

Yep, looks like that fixes it: https://github.com/mjjbell/osrm-backend/actions/runs/3073294832

SiarheiFedartsou · 2022-09-09T14:13:36Z

.github/workflows/osrm-backend.yml

@@ -34,12 +36,38 @@ jobs:
        node-version: 16


Using Node v16, not sure if we need matrix build for Windows building for older versions.

We may need Node v18 though too(which is currently latest)

SiarheiFedartsou · 2022-09-09T14:16:05Z

CMakeLists.txt

+  list(APPEND sources ${filename})	
+endforeach()	
+add_library(check-headers STATIC EXCLUDE_FROM_ALL ${sources})	
+set_target_properties(check-headers PROPERTIES ARCHIVE_OUTPUT_DIRECTORY ${check_headers_dir})	


It is something I had to spend a lot of time on.

First of all as I understand the only purpose of this is to check "sanity" of headers, i.e. that every header includes everything what is needed to compile it. That's why it is not clear why we had condition to only have it if ENABLE_NODE_BINDINGS=ON. The second as it turned out we never built it due to EXCLUDE_FROM_ALL, but MSVC builds it for some reason and that's why I faced it and had to modify some headers in this PR.

Looks like it was designed to run manually or in CI as make check-headers (#3409) and the node conditional was added later for unknown reasons.

In any case, perhaps there is a C++ tooling replacement for this nowadays.

SiarheiFedartsou · 2022-09-09T14:16:45Z

CMakeLists.txt

+  ${PROJECT_SOURCE_DIR}/include/*.hpp)	
+foreach(header ${headers_to_check})	
+  if ("${header}" MATCHES ".*/include/nodejs/.*")
+    # we do not check NodeJS bindings headers


NodeJS bindings contain nan.h, i.e. we would have to add NAN as a dependency to check-headers target.

SiarheiFedartsou · 2022-09-09T14:17:08Z

scripts/ci/windows-build.bat

@@ -25,10 +25,6 @@ msbuild OSRM.sln ^
 /nologo
 IF %ERRORLEVEL% NEQ 0 GOTO ERROR

-dir C:\Users\runneradmin\.conan\


Forgot to remove this in previous PRs.

SiarheiFedartsou · 2022-09-09T14:17:26Z

scripts/ci/windows-build.bat

@@ -11,7 +11,7 @@ mkdir build
 IF %ERRORLEVEL% NEQ 0 GOTO ERROR
 cd build
 IF %ERRORLEVEL% NEQ 0 GOTO ERROR
-cmake -DENABLE_CONAN=ON -DCMAKE_BUILD_TYPE=%CONFIGURATION% -DBUILD_TOOLS=ON -G "Visual Studio 17 2022" ..


BUILD_TOOLS=ON is actually not needed.

SiarheiFedartsou · 2022-09-19T20:58:29Z

@mjjbell I updated PR, please take a look again when you have a chance

mjjbell

🚀

v5.27.0 - Changes from 5.26.0 - API: - ADDED: Add Flatbuffers support to NodeJS bindings. [Project-OSRM#6338](Project-OSRM#6338) - CHANGED: Add `data_version` field to responses of all services. [Project-OSRM#5387](Project-OSRM#5387) - FIXED: Use Boost.Beast to parse HTTP request. [Project-OSRM#6294](Project-OSRM#6294) - FIXED: Fix inefficient osrm-routed connection handling [Project-OSRM#6113](https://gihub.com/Project-OSRM/osrm-backend/pull/6113) - FIXED: Fix HTTP compression precedence [Project-OSRM#6113](Project-OSRM#6113) - NodeJS: - FIXED: Support `skip_waypoints` in Node bindings [Project-OSRM#6060](Project-OSRM#6060) - Misc: - ADDED: conanbuildinfo.json for easy reading of dependencies [Project-OSRM#6388](Project-OSRM#6388) - CHANGED: Improve performance of JSON rendering. Fix undefined behaviour in JSON numbers formatting. [Project-OSRM#6380](Project-OSRM#6380) - ADDED: Add timestamps for logs. [Project-OSRM#6375](Project-OSRM#6375) - CHANGED: Improve performance of map matching via getPathDistance optimization. [Project-OSRM#6378](Project-OSRM#6378) - CHANGED: Optimize RestrictionParser performance. [Project-OSRM#6344](Project-OSRM#6344) - ADDED: Support floats for speed value in traffic updates CSV. [Project-OSRM#6327](Project-OSRM#6327) - CHANGED: Use Lua 5.4 in Docker image. [Project-OSRM#6346](Project-OSRM#6346) - CHANGED: Remove redundant nullptr check. [Project-OSRM#6326](Project-OSRM#6326) - CHANGED: missing files list is included in exception message. [Project-OSRM#5360](Project-OSRM#5360) - CHANGED: Do not use deprecated Callback::Call overload in Node bindings. [Project-OSRM#6318](Project-OSRM#6318) - FIXED: Fix distance calculation consistency. [Project-OSRM#6315](Project-OSRM#6315) - FIXED: Fix performance issue after migration to sol2 3.3.0. [Project-OSRM#6304](Project-OSRM#6304) - CHANGED: Pass osm_node_ids by reference in osrm::updater::Updater class. [Project-OSRM#6298](Project-OSRM#6298) - FIXED: Fix bug with reading Set values from Lua scripts. [Project-OSRM#6285](Project-OSRM#6285) - FIXED: Bug in bicycle profile that caused exceptions if there is a highway=bicycle in the data. [Project-OSRM#6296](Project-OSRM#6296) - FIXED: Internal refactoring of identifier types used in data facade [Project-OSRM#6044](Project-OSRM#6044) - CHANGED: Update docs to reflect recent build and dependency changes [Project-OSRM#6383](Project-OSRM#6383) - Build: - REMOVED: Get rid of Mason. [Project-OSRM#6387](Project-OSRM#6387) - CHANGED: Use clang-format from CI base image. [Project-OSRM#6391](Project-OSRM#6391) - ADDED: Build Node bindings on Windows. [Project-OSRM#6334](Project-OSRM#6334) - ADDED: Configure cross-compilation for Apple Silicon. [Project-OSRM#6360](Project-OSRM#6360) - CHANGED: Use apt-get to install Clang on CI. [Project-OSRM#6345](Project-OSRM#6345) - CHANGED: Fix TBB in case of Conan + NodeJS build. [Project-OSRM#6333](Project-OSRM#6333) - CHANGED: Migrate to modern TBB version. [Project-OSRM#6300](Project-OSRM#6300) - CHANGED: Enable performance-move-const-arg clang-tidy check. [Project-OSRM#6319](Project-OSRM#6319) - CHANGED: Use the latest node on CI. [Project-OSRM#6317](Project-OSRM#6317) - CHANGED: Migrate Windows CI to GitHub Actions. [Project-OSRM#6312](Project-OSRM#6312) - ADDED: Add smoke test for Docker image. [Project-OSRM#6313](Project-OSRM#6313) - CHANGED: Update libosmium to version 2.18.0. [Project-OSRM#6303](Project-OSRM#6303) - CHANGED: Remove EXACT from find_package if using Conan. [Project-OSRM#6299](Project-OSRM#6299) - CHANGED: Configure Undefined Behaviour Sanitizer. [Project-OSRM#6290](Project-OSRM#6290) - CHANGED: Use Conan instead of Mason to install code dependencies. [Project-OSRM#6284](Project-OSRM#6284) - CHANGED: Migrate to C++17. Update sol2 to 3.3.0. [Project-OSRM#6279](Project-OSRM#6279) - CHANGED: Update macOS CI image to macos-11. [Project-OSRM#6286](Project-OSRM#6286) - CHANGED: Enable even more clang-tidy checks. [Project-OSRM#6273](Project-OSRM#6273) - CHANGED: Configure CMake to not build flatbuffers tests and samples. [Project-OSRM#6274](Project-OSRM#6274) - CHANGED: Enable more clang-tidy checks. [Project-OSRM#6270](Project-OSRM#6270) - CHANGED: Configure clang-tidy job on CI. [Project-OSRM#6261](Project-OSRM#6261) - CHANGED: Use Github Actions for building container images [Project-OSRM#6138](Project-OSRM#6138) - CHANGED: Upgrade Boost dependency to 1.70 [Project-OSRM#6113](Project-OSRM#6113) - CHANGED: Upgrade Ubuntu CI builds to 20.04 [Project-OSRM#6119](Project-OSRM#6119) - CHANGED: Make building osrm-routed optional [Project-OSRM#6144](Project-OSRM#6144) - FIXED: Run all unit tests in CI [Project-OSRM#5248](Project-OSRM#5248) - FIXED: Fix installation of Mason CMake and 32 bit CI build [Project-OSRM#6170](Project-OSRM#6170) - FIXED: Fixed Node docs generation check in CI. [Project-OSRM#6058](Project-OSRM#6058) - CHANGED: Docker build, enabled arm64 build layer [Project-OSRM#6172](Project-OSRM#6172) - CHANGED: Docker build, enabled apt-get update/install caching in separate layer for build phase [Project-OSRM#6175](Project-OSRM#6175) - FIXED: Bump CI complete meta job to ubuntu-20.04 [Project-OSRM#6323](Project-OSRM#6323) - CHANGED: Node packages are now scoped by @Project-OSRM [Project-OSRM#6386](Project-OSRM#6386) - Routing: - CHANGED: Lazily generate optional route path data [Project-OSRM#6045](Project-OSRM#6045) - FIXED: Completed support for no_entry and no_exit turn restrictions. [Project-OSRM#5988](Project-OSRM#5988) - ADDED: Add support for non-round-trips with a single fixed endpoint. [Project-OSRM#6050](Project-OSRM#6050) - FIXED: Improvements to maneuver override processing [Project-OSRM#6125](Project-OSRM#6125) - ADDED: Support snapping to multiple ways at an input location. [Project-OSRM#5953](Project-OSRM#5953) - FIXED: Fix snapping target locations to ways used in turn restrictions. [Project-OSRM#6339](Project-OSRM#6339) - ADDED: Support OSM traffic signal directions. [Project-OSRM#6153](Project-OSRM#6153) - FIXED: Ensure u-turn exists in intersection view. [Project-OSRM#6376](Project-OSRM#6376) - FIXED: Gracefully handle no-turn intersections in guidance processing. [Project-OSRM#6382](Project-OSRM#6382) - Profile: - CHANGED: Bicycle surface speeds [Project-OSRM#6212](Project-OSRM#6212) - Tools: - CHANGED: Do not generate intermediate .osrm file in osrm-extract. [Project-OSRM#6354](Project-OSRM#6354)

Extends CI to build NodeJS bindings for Windows.

SiarheiFedartsou added 17 commits August 25, 2022 18:12

Build Node bindings on Windows

e9c631d

Build Node bindings on Windows

2707eba

Build Node bindings on Windows

a36034b

Build Node bindings on Windows

33c9e05

Build Node bindings on Windows

fd7cf93

Build Node bindings on Windows

e32f03e

Build Node bindings on Windows

e177ef1

Build Node bindings on Windows

90e6c4b

Build Node bindings on Windows

28f9c46

Build Node bindings on Windows

3062375

Build Node bindings on Windows

f404d75

Build Node bindings on Windows

7a1a0ff

Build Node bindings on Windows

b6c22ac

Build Node bindings on Windows

232b814

Build Node bindings on Windows

76b671d

Build Node bindings on Windows

789de84

Build Node bindings on Windows

c5e6e02

mjjbell mentioned this pull request Aug 28, 2022

5.27 Release? #6325

Closed

7 tasks

SiarheiFedartsou added 12 commits September 7, 2022 20:57

wip

3551ca1

Merge branch 'master' into sf-windows-node

4c492c0

wip

fb5cf31

wip

050095c

wip

7f0e5a3

wip

b3f8b06

wip

f5ffe1f

wip

f1ce6c7

wip

c53b825

wip

9b7ae7e

wip

15f8f67

wip

45ae6ea

SiarheiFedartsou added 7 commits September 9, 2022 14:32

wip

72bc8fd

wip

614e4b2

wip

3400be3

wip

f1ad211

wip

29c0c09

wip

467fafb

wip

ad1d4e6

SiarheiFedartsou commented Sep 9, 2022

View reviewed changes

M1

fd574c2

SiarheiFedartsou requested a review from mjjbell September 9, 2022 14:32

SiarheiFedartsou marked this pull request as ready for review September 9, 2022 14:32

Build Node bindings on Windows

75780d0

SiarheiFedartsou force-pushed the sf-windows-node branch from 587c985 to 75780d0 Compare September 19, 2022 13:52

SiarheiFedartsou added 3 commits September 19, 2022 17:36

Build Node bindings on Windows

25f3f0f

Update osrm-backend.yml

b2a5fc7

Build Node bindings on Windows

62858cf

mjjbell approved these changes Sep 20, 2022

View reviewed changes

mjjbell merged commit 9b83481 into master Sep 20, 2022

mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023

Build Node bindings on Windows (Project-OSRM#6334)

2de8898

Extends CI to build NodeJS bindings for Windows.

DennisOSRM deleted the sf-windows-node branch May 3, 2024 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build Node bindings on Windows #6334

Build Node bindings on Windows #6334

SiarheiFedartsou commented Aug 25, 2022 •

edited

Loading

SiarheiFedartsou Sep 9, 2022

SiarheiFedartsou Sep 9, 2022

mjjbell Sep 17, 2022

SiarheiFedartsou Sep 17, 2022

SiarheiFedartsou Sep 9, 2022

mjjbell Sep 15, 2022

mjjbell Sep 16, 2022

SiarheiFedartsou Sep 16, 2022

mjjbell Sep 17, 2022

SiarheiFedartsou Sep 9, 2022

SiarheiFedartsou Sep 16, 2022

SiarheiFedartsou Sep 9, 2022

mjjbell Sep 15, 2022

SiarheiFedartsou Sep 9, 2022

SiarheiFedartsou Sep 9, 2022

SiarheiFedartsou Sep 9, 2022

SiarheiFedartsou commented Sep 19, 2022

mjjbell left a comment

Build Node bindings on Windows #6334

Build Node bindings on Windows #6334

Conversation

SiarheiFedartsou commented Aug 25, 2022 • edited Loading

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SiarheiFedartsou commented Sep 19, 2022

mjjbell left a comment

Choose a reason for hiding this comment

SiarheiFedartsou commented Aug 25, 2022 •

edited

Loading