Build Node bindings on Windows #6334
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,9 @@ jobs: | |
| needs: format-taginfo-docs | ||
| runs-on: windows-2022 | ||
| continue-on-error: false | ||
| env: | ||
| BUILD_TYPE: Release | ||
| ENABLE_APPLE_SILICON: "OFF" | ||
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - run: pip install conan==1.51.3 | ||
|
|
@@ -34,12 +37,38 @@ jobs: | |
| node-version: 16 | ||
| - run: node --version | ||
| - run: npm --version | ||
| - name: Prepare environment | ||
| shell: bash | ||
| run: | | ||
| PACKAGE_JSON_VERSION=$(node -e "console.log(require('./package.json').version)") | ||
|
There was a problem hiding this comment. 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 |
||
| echo PUBLISH=$([[ "${GITHUB_REF:-}" == "refs/tags/v${PACKAGE_JSON_VERSION}" ]] && echo "On" || echo "Off") >> $GITHUB_ENV | ||
| - run: npm install --ignore-scripts | ||
| - run: npm link --ignore-scripts | ||
| - uses: microsoft/setup-msbuild@v1.1 | ||
| - name: Build | ||
| run: | | ||
| .\scripts\ci\windows-build.bat | ||
| - name: Run node tests | ||
| shell: bash | ||
| run: | | ||
| ./lib/binding/osrm-datastore.exe test/data/ch/monaco.osrm | ||
| node test/nodejs/index.js | ||
| - name: Build Node package | ||
| shell: bash | ||
| run: ./scripts/ci/node_package.sh | ||
| - name: Publish Node package | ||
| if: ${{ env.PUBLISH == 'On' }} | ||
| uses: ncipollo/release-action@v1 | ||
|
There was a problem hiding this comment. Couldn't it check as well, but I expect that it should work as on other Linux/macOS jobs. There was a problem hiding this comment. You can test it on your own fork by publishing some dummy versions. I'll give it a try on mine. There was a problem hiding this comment. Hmm, well the Windows build was published, which is great. But the Linux packaging failed. Not sure if it's related to these changes, looks like it might be a Conan issue. There was a problem hiding this comment. I think I fixed it in #6360 (via RPATH changes, tbh I am not sure that it is caused by Conan, but may be) There was a problem hiding this comment. Yep, looks like that fixes it: https://github.com/mjjbell/osrm-backend/actions/runs/3073294832 |
||
| with: | ||
| allowUpdates: true | ||
| artifactErrorsFailBuild: true | ||
| artifacts: build/stage/**/*.tar.gz | ||
| omitBody: true | ||
| omitBodyDuringUpdate: true | ||
| omitName: true | ||
| omitNameDuringUpdate: true | ||
| replacesArtifacts: true | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| format-taginfo-docs: | ||
| runs-on: ubuntu-20.04 | ||
|
|
@@ -526,15 +555,13 @@ jobs: | |
| key: v3-test-${{ matrix.name }}-${{ github.sha }} | ||
| restore-keys: | | ||
| v3-test-${{ matrix.name }}- | ||
| - name: Prepare environment | ||
| run: | | ||
| PACKAGE_JSON_VERSION=$(node -e "console.log(require('./package.json').version)") | ||
| echo PUBLISH=$([[ "${GITHUB_REF:-}" == "refs/tags/v${PACKAGE_JSON_VERSION}" ]] && echo "On" || echo "Off") >> $GITHUB_ENV | ||
| echo "OSRM_INSTALL_DIR=${GITHUB_WORKSPACE}/install-osrm" >> $GITHUB_ENV | ||
| echo "OSRM_BUILD_DIR=${GITHUB_WORKSPACE}/build-osrm" >> $GITHUB_ENV | ||
| if [[ "$ENABLE_SANITIZER" == 'ON' ]]; then | ||
| # We can only set this after checkout once we know the workspace directory | ||
| echo "LSAN_OPTIONS=print_suppressions=0:suppressions=${GITHUB_WORKSPACE}/scripts/ci/leaksanitizer.conf" >> $GITHUB_ENV | ||
|
|
@@ -549,12 +576,10 @@ jobs: | |
| sudo mdutil -i off / | ||
| export MASON_OS=osx | ||
| fi | ||
| echo "MASON=${GITHUB_WORKSPACE}/scripts/mason.sh" >> $GITHUB_ENV | ||
|
|
||
| echo "CMAKE_URL=https://mason-binaries.s3.amazonaws.com/${MASON_OS}-x86_64/cmake/${CMAKE_VERSION}.tar.gz" >> $GITHUB_ENV | ||
| echo "CMAKE_DIR=mason_packages/${MASON_OS}-x86_64/cmake/${CMAKE_VERSION}" >> $GITHUB_ENV | ||
| - name: Install dev dependencies | ||
| run: | | ||
| python3 -m pip install conan==1.51.3 | ||
|
|
@@ -634,6 +659,7 @@ jobs: | |
| else | ||
| APPLE_SILICON_FLAGS=() | ||
| fi | ||
|
|
||
| cmake .. -DCMAKE_BUILD_TYPE=${BUILD_TYPE} \ | ||
| -DENABLE_CONAN=${ENABLE_CONAN:-OFF} \ | ||
| -DENABLE_ASSERTIONS=${ENABLE_ASSERTIONS:-OFF} \ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ cmake_minimum_required(VERSION 3.2) | |
| set(CMAKE_CXX_STANDARD 17) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED TRUE) | ||
|
|
||
|
|
||
| if(CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_CURRENT_BINARY_DIR AND NOT MSVC_IDE) | ||
| message(FATAL_ERROR "In-source builds are not allowed. | ||
| Please create a directory and run cmake from there, passing the path to this source directory as the last argument. | ||
|
|
@@ -883,22 +884,23 @@ if (ENABLE_FUZZING) | |
| add_subdirectory(fuzz) | ||
| endif () | ||
|
|
||
| # add headers sanity check target that includes all headers independently | ||
| set(check_headers_dir "${PROJECT_BINARY_DIR}/check-headers") | ||
| file(GLOB_RECURSE headers_to_check | ||
| ${PROJECT_BINARY_DIR}/*.hpp | ||
| ${PROJECT_SOURCE_DIR}/include/*.hpp) | ||
| foreach(header ${headers_to_check}) | ||
| if ("${header}" MATCHES ".*/include/nodejs/.*") | ||
| # we do not check NodeJS bindings headers | ||
|
There was a problem hiding this comment. NodeJS bindings contain |
||
| continue() | ||
| endif() | ||
| get_filename_component(filename ${header} NAME_WE) | ||
| set(filename "${check_headers_dir}/${filename}.cpp") | ||
| if (NOT EXISTS ${filename}) | ||
| file(WRITE ${filename} "#include \"${header}\"\n") | ||
| endif() | ||
| 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}) | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. Looks like it was designed to run manually or in CI as In any case, perhaps there is a C++ tooling replacement for this nowadays. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| #include "util/json_util.hpp" | ||
|
|
||
| #include <iterator> | ||
| #include <map> | ||
| #include <vector> | ||
|
|
||
| namespace osrm | ||
|
|
||
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.
Using Node v16, not sure if we need matrix build for Windows building for older versions.
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.
We may need Node v18 though too(which is currently latest)