Expose algorithms option in node bindings and integrate new toolchain by TheMarex · Pull Request #3834 · Project-OSRM/osrm-backend

TheMarex · 2017-03-17T16:47:11Z

Issue

Integrates algorithm flags into node bindings and bundles the new toolchain for MLD. This addresses #3846.

Tasklist

Merge Merges node-osrm into repo. #3768
review
adjust for comments

Requirements / Relations

Based on #3768.

daniel-j-h · 2017-03-30T08:00:23Z

.travis.yml

 env:
  global:
+   - secure: "hk+32aXXF5t1ApaM2Wjqooz3dx1si907L87WRMkO47WlpJmUUU/Ye+MJk9sViH8MdhOcceocVAmdYl5/WFWOIbDWNlBya9QvXDZyIu2KIre/0QyOCTZbrsif8paBXKIO5O/R4OTvIZ8rvWZsadBdmAT9GSbDhih6FzqXAEgeIYQ="
+   - secure: "VE+cFkseFwW4jK6XwkP0yW3h4DixPJ8+Eb3yKcchGZ5iIJxlZ/8i1vKHYxadgPRwSYwPSB14tF70xj2OmiT2keGzZUfphmPXinBaLEhYk+Bde+GZZkoSl5ND109I/LcyNr0nG9dDgtV6pkvFchgchpyP9JnVOOS0+crEZlAz0RE="


Where did you get those from and how are they getting used? for node pre gyp publishing / aws credentials?

Yup they set node_pre_gyp_{secretKey,KeyId}.

daniel-j-h · 2017-03-30T08:02:22Z

.travis.yml

+    - /^v\d+\.\d+(\.\d+)?(-\S*)?$/

 cache:
+  yarn: true


Ah I guess this is https://blog.travis-ci.com/2016-11-21-travis-ci-now-supports-yarn - is yarn worth it locally?

Yeah factor 3-5 faster then npm.

daniel-j-h · 2017-03-30T08:07:49Z

.travis.yml

  - popd
-  - npm run build-api-docs
+  # building docs only works with npm3+ not with yarn or npm2
+  #- yarn run docs


Why? And can't we just use npm here, then?

This needs fixes in the docs tool. I don't see the point of building the docs for every build job. It almost takes a minute and would block us from using a sensible package manager like yarn.

daniel-j-h · 2017-03-30T08:10:29Z

docs/releasing.md

+7. Use `npm run build-api-docs` to generate the API documentation.  Copy `build/docs/*` to `https://github.com/Project-OSRM/project-osrm.github.com` in the `docs/vN.N.N/api` directory
+8. Push tags and commits: `git push; git push --tags`
 9. If not a release-candidate: Write a mailing-list post to [email protected] to announce the release
+10. Wait until the travis build has been complated and check if the node binaries were published by doing:


daniel-j-h · 2017-03-30T08:12:42Z

include/nodejs/node_osrm_support.hpp

+    {
+        Nan::ThrowError("algorithm option must be a string and one of 'CH', 'CoreCH', or 'MLD'.");
+        return engine_config_ptr();
+    }


Hm, using the maybe returned from Get is fine this way? as in, this is the same as

maybe = Get(); if (!maybe) err;

?

Hrm the docs say that the result from Object::Get is a MaybeLocal<> but that type does not have a operator->. For a Local<> calling operator-> is not safe since it would dereference a nullptr. So yes this would need a check.

daniel-j-h · 2017-03-30T08:17:00Z

test/nodejs/route.js

+        assert.ok(route.routes.length);
+        assert.ok(route.routes[0].geometry);
+    });
+});


Urgh please merge into master asap, I'm already working on changing the tests to monaco

daniel-j-h · 2017-03-30T08:17:25Z

.travis.yml

    if [[ "${TRAVIS_OS_NAME}" == "osx" ]]; then
      export JOBS=$((`sysctl -n hw.ncpu` + 1))
      sudo mdutil -i off /
+      npm install -g yarn


We need to update the yarn.lock file every time we update package.config now?

No it is updated automatically by yarn when you add a package using yarn add package. You will have to commit it though.

oxidase · 2017-03-30T14:30:28Z

package.json

-  "author": "",
-  "license": "ISC",
+  "author": "Project OSRM Team",
+  "license": "BSD 2-Clause",


guess ISC was wrong originally?

Yes, this is just the default license and nobody changed it.

oxidase · 2017-03-30T14:36:15Z

yarn.lock

@@ -0,0 +1,6179 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


Is it really necessary to commit the auto-generated file? Guess it is better to add it to .gitignore otherwise will be always local changes and conflicts.

Yes this basically ensure that yarn resolves package versions the same for everyone installing osrm. One advantage of the yarn approach with the lock file is to enable caching, e.g. on Travis.

TheMarex · 2017-03-30T17:38:01Z

@daniel-j-h @oxidase hopefully I addressed all concerns. Let me know if this is good to merge.

daniel-j-h · 2017-03-30T18:16:44Z

👍

oxidase

👍 just disappointed due to adding 6000 lines that is basically a job of a package manager.

TheMarex added MLD Work In Progress labels Mar 17, 2017

TheMarex force-pushed the feature/algorithmsjs branch from 76b6626 to 598219b Compare March 17, 2017 16:54

TheMarex mentioned this pull request Mar 21, 2017

Merges node-osrm into repo. #3768

Merged

8 tasks

TheMarex force-pushed the feature/algorithmsjs branch 3 times, most recently from c850d90 to 24781e7 Compare March 28, 2017 14:25

TheMarex added 3 commits March 29, 2017 14:45

Integrate MLD in node bindings

31b6e4b

Switch to yarn

82dc092

Fix publishing node binaries

ce5afb7

TheMarex force-pushed the feature/algorithmsjs branch from 24781e7 to ce5afb7 Compare March 29, 2017 14:47

Switch from commit message publishing to tag-based publishing

abaa474

TheMarex force-pushed the feature/algorithmsjs branch from fdfe32b to abaa474 Compare March 29, 2017 15:30

Fix travis.yml

ae01a1a

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

TheMarex requested a review from daniel-j-h March 29, 2017 16:06

Update releasing docs

8c790be

daniel-j-h added the NodeJs label Mar 29, 2017

daniel-j-h mentioned this pull request Mar 29, 2017

NodeJs Binding Tests #3877

Merged

4 tasks

daniel-j-h reviewed Mar 30, 2017

View reviewed changes

oxidase reviewed Mar 30, 2017

View reviewed changes

Always check local variables

ef9969e

TheMarex added Review - In feedback and removed Review labels Mar 30, 2017

oxidase approved these changes Mar 30, 2017

View reviewed changes

TheMarex merged commit 273fd68 into master Mar 30, 2017

TheMarex deleted the feature/algorithmsjs branch March 30, 2017 20:12

TheMarex mentioned this pull request Apr 3, 2017

Integrate MLD into node bindings #3846

Closed

		@@ -0,0 +1,6179 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

Uh oh!

Conversation

TheMarex commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Tasklist

Requirements / Relations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheMarex commented Mar 30, 2017

Uh oh!

daniel-j-h commented Mar 30, 2017

Uh oh!

oxidase left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TheMarex commented Mar 17, 2017 •

edited

Loading