Publicly expose binaries to Node.js bindings, resolves #4377

[daniel-j-h]

For #4377:

npm install && npm link

now puts osrm binaries in the right place, so users don't have to fiddle with internal paths. After npm link you can just call osrm-extract; it will be in your PATH.

cc @maning @srividyacb

/home/daniel/.nvm/versions/node/v4.8.3/bin/osrm-extract -> /home/daniel/.nvm/versions/node/v4.8.3/lib/node_modules/osrm/lib/binding/osrm-extract
/home/daniel/.nvm/versions/node/v4.8.3/bin/osrm-contract -> /home/daniel/.nvm/versions/node/v4.8.3/lib/node_modules/osrm/lib/binding/osrm-contract
/home/daniel/.nvm/versions/node/v4.8.3/bin/osrm-partition -> /home/daniel/.nvm/versions/node/v4.8.3/lib/node_modules/osrm/lib/binding/osrm-partition
/home/daniel/.nvm/versions/node/v4.8.3/bin/osrm-customize -> /home/daniel/.nvm/versions/node/v4.8.3/lib/node_modules/osrm/lib/binding/osrm-customize
/home/daniel/.nvm/versions/node/v4.8.3/bin/osrm-routed -> /home/daniel/.nvm/versions/node/v4.8.3/lib/node_modules/osrm/lib/binding/osrm-routed
/home/daniel/.nvm/versions/node/v4.8.3/bin/osrm-components -> /home/daniel/.nvm/versions/node/v4.8.3/lib/node_modules/osrm/lib/binding/osrm-components
/home/daniel/.nvm/versions/node/v4.8.3/bin/osrm-datastore -> /home/daniel/.nvm/versions/node/v4.8.3/lib/node_modules/osrm/lib/binding/osrm-datastore
/home/daniel/.nvm/versions/node/v4.8.3/lib/node_modules/osrm -> /tmp/osrm-backend

[danpat]

Did the package-lock.json file need to be updated to support something in particular, or was that just a general update?

After speaking with some folks, the usual strategy seems to be:

1. Update package-lock.json if you change a dependency in package.json
2. Update package-lock.json if you need to update a dependency somewhere in the tree
3. Otherwise, it can generally be left alone.

If there's no explicit reason to update the lockfile here, would you mind reverting that part of this change?

Side note: we're building using yarn (not npm) on Travis, but we're not maintaining yarn's duplicate yarn.lock file. Until we have a good consistent strategy for maintaining these files, I suggest we don't clutter our PRs with the noise that the auto-generated file diff creates.

[danpat]

Does this PR need to bump the version number?

[daniel-j-h]

No I don't think so I just noticed the version is 5.11.0 and we want a version string on master not matching a release we want to do in the future (if I understand it correctly?). This probably needs a section in the releasing.md, to bump the version string after a release immediately on master (?)

[TheMarex]

Yeah bumping after release to {MAJOR}.{MINOR+1}.0-latest.1 should be the best-practice.

[daniel-j-h]

I will add this to the releasing docs.

What is unclear to me is file compatibility between releases. We bump the package json version but not the CMakeLists version. This results in:

• either we bump to 5.11.0 in CMakeLists immediately after we released 5.10, which would result in the actual 5.11.0 release being compatible with all datasets generated right after we cut 5.10, or
• we only bump to 5.11.0 in CMakeLists right before we publish the proper 5.11.0 release, which would result in all datasets generated before being compatible with 5.10.

[danpat]

How about setting versions on master to 0.0.0 (both package.json and CMakeLists.txt) always, and only modifying the version field on release branches?

[daniel-j-h]

Then all master datasets will be compatible with each other

[danpat]

But we have that problem already - we often merge breaking data changes to master and we don't bump the version number.

IMO the biggest concern with "master datasets are all compatible" would be bizzaro tickets being opened by folks not realizing that they're trying to use old data, creating weird error situations and undefined behaviour that's hard to debug. You're right, I don't think we want to create that kind of trouble for ourselves.

The old approach of checksumming the critical IO files was semi-workable, as long as we can capture all code that affects the file format in the checksum value. I haven't looked at our re-factoring for a while, are we closer, or further away from being able to automatically detect file compatibility using that approach?

[danpat]

Unfortunately, I think we need to roll this back.

The problem is that the install rule that downloads and puts node_modules/osrm/lib/binding in place, gets run after the bin symlinks are created, so npm fails with:

$ npm install osrm@5.11.0-rc.1
npm ERR! path /home/danpat/test/node_modules/osrm/lib/binding/osrm-extract
npm ERR! code ENOENT
npm ERR! errno -2
npm ERR! syscall chmod
npm ERR! enoent ENOENT: no such file or directory, chmod '/home/danpat/test/node_modules/osrm/lib/binding/osrm-extract'
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent

i.e. osrm-extract does not yet exist when npm is trying to create the symlinks in node_modules/.bin.

I thought that maybe we could fix this by moving the call to node-pre-gyp into the preinstall rule. This seems to work on my machine, but I've learned that it's not reliable (behaviour depends on npm version): Project-OSRM/node-osrm#305 TryGhost/node-sqlite3#720 (comment)

The Mapnik project gets around this issue by bundling wrapper scripts as the binaries destined for node_modules/.bin instead of the actual executables - example: https://github.com/mapnik/node-mapnik/blob/master/bin/mapnik-index.js

We should do something similar (ugly as it is). I'll open a ticket to describe what needs to be done, but for now, I'm going to revert this change to un-block the 5.11 release.