Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for latest Nodejs Version and upgrading NVM #5318

Closed
wants to merge 1 commit into from

Conversation

salimkayabasi
Copy link
Contributor

@salimkayabasi salimkayabasi commented Dec 17, 2018

Issue

OSRM automated builds are targeting the builds for hardcoded versions of nodejs. Instead of compiling files for node8 or node10, it could be compiled for lts version and latest version

Regarding to #5312
Introducing support for latest nodejs version and upgrading NVM version

  • Upgrading osx images to default one (osx9.4),osx9.2 has outdated nvm (0.31) which was issue while using lts version
  • removed dist: trusty param which is not required anymore (travis supports sudo for every images)
  • printing nvm --version
  • renamed s/$NODE/$NODE_VERSION, in fact, it was version of node, not node itself and it will be consistent with node-pre-gyp

Official support will follow nodejs release schedule, active development on OSRM will be for latest nodejs version and keep supporting for node10 which is lts for now

Dropping support for node8 which will be dropped down at the end of this year
https://github.com/nodejs/Release#release-schedule

Tasklist

Requirements / Relations

Optimized solution after this PR #5314

@salimkayabasi salimkayabasi force-pushed the travis branch 2 times, most recently from f79c86f to b00ce7a Compare December 17, 2018 14:55
@danpat
Copy link
Member

danpat commented Dec 17, 2018

@salimkayabasi Thanks for the PR - unfortunately, there are a few problems with this change.

  1. This breaks binary package publishing. The way node-pre-gyp works, it requires an explicit node module version in the binary package name (see "node module version" column at https://nodejs.org/en/download/releases/). We need to do a separate build for each Node release we support.
  2. This change would mean we only support LTS, whichever version that happens to be at the time the build occurs. This means people who don't upgrade to the latest LTS won't be able to update OSRM.
  3. Success/failure of the build depends on what the LTS build happens to be at the time the build occurs. This means our builds may start failing due to no fault of our own. We don't want this.

What might be worthwhile is adding an additional LTS build that does allow_failures

https://docs.travis-ci.com/user/build-matrix/#rows-that-are-allowed-to-fail

which would give us some non-fatal warning that changes are required to support a new LTS.

@salimkayabasi
Copy link
Contributor Author

thanks @danpat , I got your point. Let me clear and update my PR

@salimkayabasi salimkayabasi changed the title Better travis config and supporting LTS and latest version instead if hardcoded versions [WIP]: Better travis config and supporting LTS and latest version instead if hardcoded versions Dec 17, 2018
@salimkayabasi salimkayabasi force-pushed the travis branch 16 times, most recently from ce62bc8 to 60041f9 Compare December 19, 2018 14:11
@salimkayabasi salimkayabasi changed the title [WIP]: Better travis config and supporting LTS and latest version instead if hardcoded versions Better travis config and supporting LTS and latest version instead if hardcoded versions Dec 19, 2018
@salimkayabasi salimkayabasi force-pushed the travis branch 3 times, most recently from aab1117 to 875a32b Compare December 19, 2018 14:31
@salimkayabasi
Copy link
Contributor Author

salimkayabasi commented Dec 19, 2018

hey @danpat,

I've checked and updated a few lines regarding to latest travis documentation and node-pre-gyp documentation

https://www.npmjs.com/package/node-pre-gyp#travis-automation
https://docs.travis-ci.com/user/reference/overview/#virtualisation-environment-vs-operating-system

These are my feedback regarding to your questions

  1. Looks like node-pre-gyp is using process.versions.modules in order to get number of module
  2. We will keep supporting LTS and latest versions which are 10 and 11 now and it will be 10 and 12 in future. Node8 support will stop at the end of this year.
  3. Providing only number (nvm use 10) is also installing latest version of 10.X, that mean we are testing OSRM against to minor changes on nodejs. If we start supporting latest and keep it green, it won't be an issue once it get marked as LTS.

OSRM will start supporting latest before it will turn into LTS.

For the people whoever are still using node4,node6 or node8 probably they are not upgrading version of OSRM too. It should be out of scope.

@salimkayabasi salimkayabasi force-pushed the travis branch 4 times, most recently from feb3a4b to 0f87519 Compare December 19, 2018 15:08
@salimkayabasi salimkayabasi force-pushed the travis branch 2 times, most recently from 82ccf38 to 81e4c54 Compare December 19, 2018 15:27
@danpat
Copy link
Member

danpat commented Dec 19, 2018

@salimkayabasi I can't merge this - I'm paid by Mapbox to work on OSRM, and we're going to be using Node 8 for at least another 6 months, so I can't drop OSRM support for it yet. Over in #5312, I pulled download logs for the various binary versions we're publishing - about 80% of them are Node 8, so we'd break a lot of folks if we removed support for that Node version right now. Not everyone is able to be upgraded to "latest" all the time - production systems take time to upgrade.

According to https://github.com/nodejs/Release#release-schedule, Node 8 doesn't end-of-life until Dec 2019 - the "Maintenance LTS" period begins at the end of the year, and stable production systems likely won't get upgraded until closer to the EOL date.

I do not like depending on transient version aliases for this process - we should be explicit about which versions we support. I'd be just fine having a "lts" and "latest" builds that are not required, so we have canaries when new releases land, but those cannot be the only versions we support - the label "lts" will change over time, which might mean that someone who needs to re-build an old version of OSRM for some reason cannot because they don't know what "lts" was at the time.

@salimkayabasi
Copy link
Contributor Author

salimkayabasi commented Dec 19, 2018

about 80% of them are Node 8

I didn't know that

Agree with transient versions, what about supporting like this node8 node10 and latest
We will be sure that everything is working with upcoming/latest version

@salimkayabasi salimkayabasi changed the title Better travis config and supporting LTS and latest version instead if hardcoded versions Adding support for latest Nodejs Version and upgrading NVM Dec 19, 2018
@danpat
Copy link
Member

danpat commented Dec 19, 2018

what about supporting like this node8 node10 and latest

Yeah, that's what I meant by "I'd be just fine having a "lts" and "latest" builds that are not required, so we have canaries when new releases land"

By "canary" I was referring to https://en.wikipedia.org/wiki/Sentinel_species#Historical_examples

@salimkayabasi
Copy link
Contributor Author

You've just introduced binaries for node10 around 2w ago by #5246

Those metrics about download numbers could not be perfect for real use case. It is also quite possible that people are compiling OSRM libraries by themselves.

@salimkayabasi salimkayabasi force-pushed the travis branch 4 times, most recently from 4ef4481 to 89fd178 Compare December 19, 2018 21:04
@salimkayabasi
Copy link
Contributor Author

Have u had any chance to review this PR?

@danpat
Copy link
Member

danpat commented Jan 30, 2019

@salimkayabasi I've created a new PR which works the way I think we need: #5347

You can see the additional optional node and lts builds at the bottom of this Travis page:

https://travis-ci.org/Project-OSRM/osrm-backend/builds/486531607

Those optional builds will publish binaries when tags are made, if the lts and node tags don't conflict with the 8 and 10 builds we have hard-coded above. If lts and node do conflict, then the optional builds will simply fail and not publish anything, and won't block the build succeeding.

Thank you for prompting the idea though - this is something we should've set up a while ago.

@danpat danpat closed this Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants