Merges node-osrm into repo. #3768

daniel-j-h · 2017-03-02T12:08:14Z

Merging node-osrm NodeJS bindings into the repository. Can be built with

cmake .. -DENABLE_NODE_BINDINGS=On

Structured as follows

src/bindings/node - recursive CMakeLists.txt and binding impl.
include/bindings/node - binding headers

What's still missing

copy tbb libs to lib/bindings or use static tbb libs only for the node bindings (?)
binary publishing / caching, node-pre-gyp can we do this without the need for a Makefile?
tests
docs
cmake depends on npm install for node-cmake - should cmake do this on the fly?
hook up to travis
merge tests, only keep Berlin dataset, discard Monaco and adapt all tests
let travis build and publish binaries

TheMarex · 2017-03-02T12:18:49Z

test/bindings/node/data/Makefile

@@ -0,0 +1,32 @@
+OSRM_RELEASE:=$(shell node -e "console.log(require('../../../../package.json').osrm_release)")


We should replace the test data for test/data in the root directory with berlin from these tests and then reuse it here instead of having an own test dataset.

Yes, requires changing all coordinates from locations in Monaco to Berlin - I agree having multiple datasets is ugly.

TheMarex · 2017-03-02T12:21:33Z

src/bindings/node/node_osrm.cpp

@@ -0,0 +1,507 @@
+#include "osrm/engine_config.hpp"


Nitpick: I would prefer a less deep filesystem structure like: src/node instead of src/bindings/node purely because I don't like typing so much :-)

This was me being sneaky and opening up the door for more bindings :)

Also give https://github.com/junegunn/fzf a try (or ctrl+p)

Yeah but you can also have src/python if you want that. I don't think we will have namespace conflicts 😉

TheMarex · 2017-03-02T12:24:07Z

package.json

+
+    "nan": "^2.1.0",
+    "node-cmake": "^1.2.1",
+    "node-pre-gyp": "~0.6.30"


This package.json needs an install and preinstall target like here:

https://github.com/Project-OSRM/node-osrm/blob/master/package.json#L45-L47

It does, a few lines below https://github.com/Project-OSRM/osrm-backend/blob/merge-node-osrm/package.json#L37-L38

TheMarex · 2017-03-06T16:20:12Z

cmake depends on npm install for node-cmake - should cmake do this on the fly?

@daniel-j-h Hm, would it be possible to bundle the cmake script directly? That is what the v2 docs recommend as well: https://github.com/cjntaylor/node-cmake

daniel-j-h · 2017-03-07T09:58:33Z

Yes, can do. Vendoring in the cmake dir.

daniel-j-h · 2017-03-07T14:32:46Z

Merged node-osrm tests into backend; the Monaco dataset is gone now. Important: I kept the s3 files for backwards compat. and uploaded the Berlin ones and made them public.

Had to adapt all tests, but now it works. The Tile plugin is especially fragile in that regard, and caused some pain. In the end I got it working (e.g. with range checks for sizes).

The tile in Berlin does not seem to have strings in it for street names - any insights here @danpat? Should all tiles have the street names in them for streets they display?

Refs.

daniel-j-h · 2017-03-08T09:39:42Z

Having LTO builds enabled for this repo.'s release / production builds is a blocker for merging this PR: #3770

Also this is mostly done with the exception of

the Tile plugin behavior outlined above
publishing binaries via node-pre-gyp, I'm missing some context how to set this up and test the full pipeline without publishing artifacts to npm while doing so. cc @TheMarex

TheMarex · 2017-03-08T10:22:41Z

publishing binaries via node-pre-gyp, I'm missing some context how to set this up and test the full pipeline without publishing artifacts to npm while doing so. cc @TheMarex

node-pre-gyp does not publish packages to npm, that is a manual step you would need to do. To just test binary builds try the following:

Make sure https://github.com/Project-OSRM/node-osrm/blob/master/scripts/publish.sh is run at the end of the travis build
Change version in the package.json to something new like 5.7.0-daniel
Commit this with [publish binary] Test version of 5.7.0 for Daniel
????
Profit.

danpat · 2017-03-08T18:43:49Z

@daniel-j-h yes - there should be street name attributes for any visible street on the tile:

#2488

We're not rendering those labels on the debug frontend though - are you sure they're missing from the tiles?

TheMarex · 2017-03-15T22:41:17Z

What is blocking here? I would like to get this merged in time to integrate the algorithm selection in node-osrm and add new MLD toolchain.

daniel-j-h · 2017-03-17T14:09:43Z

Here's an update:

gcc debug builds are timing out on travis, looks unrelated but is a pita
clang 4 linker error https://travis-ci.org/Project-OSRM/osrm-backend/jobs/212117117#L4487-L4496, also looks unrelated
Check headers failing for nan https://travis-ci.org/Project-OSRM/osrm-backend/jobs/212117115#L2521

Otherwise building the binaries is hooked up now via node-pre-gyp.

TheMarex · 2017-03-17T14:48:36Z

Check headers failing for nan https://travis-ci.org/Project-OSRM/osrm-backend/jobs/212117115#L2521

You can either exclude the bindings from the header check or make the compile unit for it a fake node module.

gcc debug builds are timing out on travis, looks unrelated but is a pita
clang 4 linker error https://travis-ci.org/Project-OSRM/osrm-backend/jobs/212117117#L4487-L4496, also looks unrelated

Did you try rebasing on master?

daniel-j-h · 2017-03-20T10:48:32Z

Good catch - seems like the build and publish scripts were triggering a second build. I guarded them now behind the ENABLE_NODE_BINDINGS env var. Let's see how this works.

TheMarex · 2017-03-20T13:55:15Z

.travis.yml

@@ -173,7 +173,7 @@ install:
  - export OSRM_BUILD_DIR="$(pwd)/build-osrm"
  - mkdir ${OSRM_BUILD_DIR} && pushd ${OSRM_BUILD_DIR}
  - export CC=${CCOMPILER} CXX=${CXXCOMPILER}
-  - cmake .. -DCMAKE_BUILD_TYPE=${BUILD_TYPE} -DENABLE_MASON=${ENABLE_MASON:-OFF} -DENABLE_ASSERTIONS=${ENABLE_ASSERTIONS:-OFF} -DBUILD_SHARED_LIBS=${BUILD_SHARED_LIBS:-OFF} -DENABLE_COVERAGE=${ENABLE_COVERAGE:-OFF} -DENABLE_SANITIZER=${ENABLE_SANITIZER:-OFF} -DBUILD_TOOLS=ON -DENABLE_CCACHE=ON
+  - cmake .. -DCMAKE_BUILD_TYPE=${BUILD_TYPE} -DENABLE_MASON=${ENABLE_MASON:-OFF} -DENABLE_ASSERTIONS=${ENABLE_ASSERTIONS:-OFF} -DBUILD_SHARED_LIBS=${BUILD_SHARED_LIBS:-OFF} -DENABLE_COVERAGE=${ENABLE_COVERAGE:-OFF} -DENABLE_SANITIZER=${ENABLE_SANITIZER:-OFF} -DBUILD_TOOLS=ON -DENABLE_CCACHE=ON -DENABLE_NODE_BINDINGS=${ENABLE_NODE_BINDINGS}


Needs to be -DENABLE_NODE_BINDINGS=${ENABLE_NODE_BINDINGS:-OFF}.

TheMarex · 2017-03-21T00:59:59Z

Fixed the windows install and data preparation and disabled the ASAN option for the GCC build for now.

We need to switch the dataset back to Monaco, Berlin is too big for CI (contracting it takes ages in debug mode and makes our builds time out).

Hopefully this will work and we can merge this to unblock #3846.

Build with cmake .. -DCMAKE_BUILD_TYPE=Release -DENABLE_NODE_BINDINGS=On -DENABLE_MASON=On

daniel-j-h · 2017-03-21T15:14:43Z

Travis green! Beware, we are not yet running the NodeJs tests (tests need to be adapted to Monaco now that I changed it back from the Berlin dataset), and we have to enable coverage again.

oxidase · 2017-03-21T15:56:40Z

[00:12:32] > [email protected] install C:\projects\osrm
[00:12:32] > node-pre-gyp install --fallback-to-build=false
[00:12:32] 
[00:12:32] node-pre-gyp ERR! install error 
[00:12:32] node-pre-gyp ERR! stack Error: 403 status code downloading tarball https://mapbox-node-binary.s3.amazonaws.com/osrm/v5.7.0/Release/node-v48-win32-ia32.tar.gz

is there any chance to get for win32? Otherwise npm testing should be commented in appveyor.yml

TheMarex · 2017-03-21T16:09:47Z

Hrm I'm not sure why npm even tries to run node-pre-gyp on Windows. I thought this was disabled with decf222#diff-180360612c6b8c4ed830919bbb4dd459R26

TheMarex · 2017-03-21T20:10:33Z

Remaining issue with the tests ticketed here #3851 Merging to unblock #3834

daniel-j-h added the Work In Progress label Mar 2, 2017

daniel-j-h requested a review from TheMarex March 2, 2017 12:08

daniel-j-h force-pushed the merge-node-osrm branch 3 times, most recently from ba29a24 to 6e130b6 Compare March 2, 2017 15:59

TheMarex requested changes Mar 2, 2017

View reviewed changes

daniel-j-h force-pushed the merge-node-osrm branch 2 times, most recently from b2ac9dc to 09a7273 Compare March 6, 2017 14:41

daniel-j-h force-pushed the merge-node-osrm branch from 09a7273 to 69d6a10 Compare March 7, 2017 09:44

daniel-j-h force-pushed the merge-node-osrm branch 2 times, most recently from e71cc1b to 15d1474 Compare March 7, 2017 14:28

danpat mentioned this pull request Mar 10, 2017

Remove circleci support Project-OSRM/node-osrm#303

Closed

2 tasks

daniel-j-h force-pushed the merge-node-osrm branch from 15d1474 to 720f8f3 Compare March 15, 2017 15:08

daniel-j-h force-pushed the merge-node-osrm branch from bfb7b00 to 5af45d4 Compare March 16, 2017 11:15

TheMarex mentioned this pull request Mar 16, 2017

Clean libosrm from concepts and dependencies external to library's design #2029

Closed

daniel-j-h force-pushed the merge-node-osrm branch 3 times, most recently from fcf7520 to 9ae8ca3 Compare March 17, 2017 11:34

TheMarex mentioned this pull request Mar 17, 2017

Reimplement osrm-routed in NodeJS #3832

Open

daniel-j-h force-pushed the merge-node-osrm branch 2 times, most recently from cf61099 to 4b31624 Compare March 17, 2017 13:25

daniel-j-h force-pushed the merge-node-osrm branch from 05d9274 to 483abf6 Compare March 20, 2017 12:08

TheMarex reviewed Mar 20, 2017

View reviewed changes

daniel-j-h force-pushed the merge-node-osrm branch from 483abf6 to 1050953 Compare March 20, 2017 14:18

TheMarex added Review and removed Work In Progress labels Mar 20, 2017

TheMarex force-pushed the merge-node-osrm branch from 1284f5d to 886dca3 Compare March 21, 2017 00:17

daniel-j-h force-pushed the merge-node-osrm branch 3 times, most recently from c2a959f to cfa4e5f Compare March 21, 2017 13:19

daniel-j-h and others added 9 commits March 21, 2017 14:52

Merges node-osrm into repository

7eab979

Build with cmake .. -DCMAKE_BUILD_TYPE=Release -DENABLE_NODE_BINDINGS=On -DENABLE_MASON=On

Use core size 0.2 on berlin for preparing test data

1779230

No osrm-contract run for the MLD dataset

e843749

Use float literals to silcen Windows warnings

89dee3e

Only extract berlin.osm.pbf once and fix windows builds

decf222

Fix example

0a9a37a

Try disabling sanitizer fo GCC6 for now

4549743

Back to Monaco for unit tests

70c2e58

Back to Monaco for examples and benchmarks

1603ebe

daniel-j-h force-pushed the merge-node-osrm branch from cfa4e5f to 1603ebe Compare March 21, 2017 13:53

fix appveyor fail

d393853

TheMarex approved these changes Mar 21, 2017

View reviewed changes

TheMarex mentioned this pull request Mar 21, 2017

Adjust tests for node bindings for using coordinates in Monaco not Berlin #3851

Closed

TheMarex merged commit bfc272f into master Mar 21, 2017

TheMarex deleted the merge-node-osrm branch March 21, 2017 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merges node-osrm into repo. #3768

Merges node-osrm into repo. #3768

daniel-j-h commented Mar 2, 2017 •

edited by TheMarex

Loading

TheMarex Mar 2, 2017

daniel-j-h Mar 3, 2017 •

edited

Loading

TheMarex Mar 2, 2017

daniel-j-h Mar 3, 2017

TheMarex Mar 3, 2017

TheMarex Mar 2, 2017

daniel-j-h Mar 3, 2017

TheMarex commented Mar 6, 2017

daniel-j-h commented Mar 7, 2017

daniel-j-h commented Mar 7, 2017

daniel-j-h commented Mar 8, 2017

TheMarex commented Mar 8, 2017

danpat commented Mar 8, 2017

TheMarex commented Mar 15, 2017

daniel-j-h commented Mar 17, 2017 •

edited

Loading

TheMarex commented Mar 17, 2017

daniel-j-h commented Mar 20, 2017

TheMarex Mar 20, 2017

TheMarex commented Mar 21, 2017

daniel-j-h commented Mar 21, 2017

oxidase commented Mar 21, 2017 •

edited

Loading

TheMarex commented Mar 21, 2017

TheMarex commented Mar 21, 2017

		@@ -0,0 +1,32 @@
		OSRM_RELEASE:=$(shell node -e "console.log(require('../../../../package.json').osrm_release)")

Merges node-osrm into repo. #3768

Merges node-osrm into repo. #3768

Conversation

daniel-j-h commented Mar 2, 2017 • edited by TheMarex Loading

TheMarex Mar 2, 2017

Choose a reason for hiding this comment

daniel-j-h Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

TheMarex Mar 2, 2017

Choose a reason for hiding this comment

daniel-j-h Mar 3, 2017

Choose a reason for hiding this comment

TheMarex Mar 3, 2017

Choose a reason for hiding this comment

TheMarex Mar 2, 2017

Choose a reason for hiding this comment

daniel-j-h Mar 3, 2017

Choose a reason for hiding this comment

TheMarex commented Mar 6, 2017

daniel-j-h commented Mar 7, 2017

daniel-j-h commented Mar 7, 2017

daniel-j-h commented Mar 8, 2017

TheMarex commented Mar 8, 2017

danpat commented Mar 8, 2017

TheMarex commented Mar 15, 2017

daniel-j-h commented Mar 17, 2017 • edited Loading

TheMarex commented Mar 17, 2017

daniel-j-h commented Mar 20, 2017

TheMarex Mar 20, 2017

Choose a reason for hiding this comment

TheMarex commented Mar 21, 2017

daniel-j-h commented Mar 21, 2017

oxidase commented Mar 21, 2017 • edited Loading

TheMarex commented Mar 21, 2017

TheMarex commented Mar 21, 2017

daniel-j-h commented Mar 2, 2017 •

edited by TheMarex

Loading

daniel-j-h Mar 3, 2017 •

edited

Loading

daniel-j-h commented Mar 17, 2017 •

edited

Loading

oxidase commented Mar 21, 2017 •

edited

Loading