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

Use npm 5 during Travis builds #1600

Merged
merged 4 commits into from
Jul 28, 2017
Merged

Use npm 5 during Travis builds #1600

merged 4 commits into from
Jul 28, 2017

Conversation

nylen
Copy link
Member

@nylen nylen commented Jun 30, 2017

This should pretty dramatically speed up builds. Let's see how much.

Edit: code is from travis-ci/travis-ci#4653 (comment).

@ntwb
Copy link
Member

ntwb commented Jun 30, 2017

I'm -1 on this change right now

npm 5 is having some pretty big issues if npm install has already been run and then a subsequent npm install, npm update, or npm prune is then run.

If the tests for this PR are restarted I expect them to fail as this will trigger the above scenario as Travis CI has now already cached the npm install for this PR. 😢

@nylen Can you restart the tests and see if the above occurs please?

@nylen
Copy link
Member Author

nylen commented Jun 30, 2017

npm 5 is having some pretty big issues if npm install has already been run and then a subsequent npm install, npm update, or npm prune is then run.

What issues have you been seeing? How does this fail?

I had restarted a job once already to see what the time savings was like from caching, and it passed. I've done so again and all seems fine.

Currently, average build runtime is around 7 minutes:

Hard to tell from just a couple of builds, but it looks like this PR may decrease that time to around 5 minutes (#, #).

Then, when restarting a job, I saw the first npm install take 85 seconds and the second one take 45 seconds. So we may gain up to 40 seconds more due to Travis caching.

@mtias mtias added the [Type] Build Tooling Issues or PRs related to build tooling label Jun 30, 2017
@ntwb
Copy link
Member

ntwb commented Jun 30, 2017

Hmmm.... So this is the npm issue I created npm/npm#17419

This was in relation to https://github.com/WordPress-Coding-Standards/eslint-plugin-wordpress and I've had to disable Travis CI caching completely for this repo as a workaround for now :/

Then https://github.com/WordPress-Coding-Standards/stylelint-config-wordpress has no issues whatsoever, so parts of this are still a little mysterious to say the least.

There's a significant number of similar npm issues https://github.com/npm/npm/search?q=cb%28%29+never+called&type=Issues&utf8=%E2%9C%93 that have been reported recently and no definitive answer on the root cause as yet.

But hey, if this repo isn't affected which appears to be the case, then woot 😄 (though I'd still like to see all the build jobs restarted to ensure this is in fact the case and that they all do pass the 2nd time round to be sure)

Extra issues here for if the above proves not to be an issue:

• Update https://github.com/WordPress/gutenberg/blob/master/CONTRIBUTING.md with instructions on installing npm 5

• npm 5 by default now creates a package-lock.json file (think yarn.lock) , this should be committed to the repo and will further improve the install to I guess ~20 seconds based on my previous WordPress npm 5 testing.

• Consider further adoption of npm 5 for WordPress, Calypso, Jetpack etc as the "standard supported npm version"

@nylen
Copy link
Member Author

nylen commented Jun 30, 2017

I've restarted both builds associated with this PR. Let's see how they do [edit: still all green].

Those issues do look pretty nasty. Curious to hear from others whether we should try this out - personally I think this repo is a good place to do it, and the build time savings are really nice. If so, I'm happy to update this PR to do items (1) and (2).

@ntwb
Copy link
Member

ntwb commented Jun 30, 2017

Yup, /shrug no idea, and npm staff haven't chimed in much (if at all) on those issues.

Removing node_modules and running an npm install with the latest Node.js 7.x and npm 5 installed should result in a package-lock.json file to be added and committed 👍

Aside 1: The above issues I mention could only an issue with Node.js 8.x with npm 5, and the use of Node.js 7.x here is the saving grace, I'd not tried debugging with Node.js 7.x and npm 5

Aside 2: Switching to Node.js 8.x should probably also be considered now, as Node.js 8.x is now the "current" branch and supersedes the 7.x branch

@ntwb
Copy link
Member

ntwb commented Jul 14, 2017

A couple of notes on the above two commits:

• We should use the same Node.js version for all the things, as we are supporting Node.js 6.x LTS we should test and use the latest LTS release of Node.js, currently this is 6.11.0

• We should use the latest npm release for all the things, npm does not have LTS releases and npm have clarified and explained this in the this blog post

@ntwb
Copy link
Member

ntwb commented Jul 14, 2017

For the package-lock.json side of things, as there's a chance of code changes not related to Travis CI in adding a package-lock.json as there's a few dependancy updates that may as well go in at the same time I'll do that in a separate PR.

@nylen
Copy link
Member Author

nylen commented Jul 27, 2017

@ntwb any objections to merging this? should we also go ahead and switch to npm v8 like we'll do soon in packages?

@ntwb
Copy link
Member

ntwb commented Jul 28, 2017

@ntwb any objections to merging this?

Nope, I'll merge it now

should we also go ahead and switch to npm v8 like we'll do soon in packages?

Yes, let's do it in a follow up PR, I'll do it later today if no one beats me to it

@ntwb ntwb merged commit 5dbac5a into master Jul 28, 2017
@ntwb ntwb deleted the update/travis-npm-5 branch July 28, 2017 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants