node-check-version.js breaks on new NPM installs

[Pomax]

wp-calypso/bin/check-node-version

Line 3 in 66126f3

semver = require( process.env.NPM_GLOBAL_ROOT + '/npm/node_modules/semver' );

relies on checking a hardcoded location that no longer applies. A better way to do this would be to use

semver = require('semver');

with the semver package in the devDependencies list.

Although the job this is doing can be done entirely without semver, by simply putting an engines section in the package.json that says which node version is required for this software to run. No need for check-node-version with that in place.

[klimeryk]

Thanks for bringing this to our attention, @Pomax! I honestly did not know about the engines section. But we are using it. However, reading into it, it does not quite work as you'd expect. It's only checked by npm when installing a package - but not its dependencies. And since running make run only installs the dependencies, this check is omitted. To trigger it you'd have to, for example, cd .. and run npm install wp-calypso there (source).

About the changed hardcoded path - which version of npm are you referring to?

BTW, we are doing it via script like this because we want to check for a "sane" version of npm, before starting to npm install anything. /cc @rralian @blowery for confirmation 😁

[Pomax]

the process.env.NPM_GLOBAL_ROOT + '/npm/node_modules/semver' location will not exist for locally installed modules. Simply relying on the local version by using require('semver') will still be beneficial to many. Checking for preinstalled packages before npm install doesn't make all that much sense: npm will already consult its global cache, that's not a thing you need to try to make it do =)

Better to assume people have an up to date node, and have them deal with the problem of using outdated versions, then having the install process break for people due to the hardcoded location not being a valid fs location, right?

[nylen]

More discussion about this at https://github.com/Automattic/wp-calypso/pull/2124/files#r54121505. Summarizing a bit, we want to run this quick check before npm install in case installing all those dependencies is a waste of time.

so far four people I work with ran into the semver require call failing due to it not having been installed

Can you provide more details about this? Is there a version of Node.js / npm that no longer contains semver in the expected location?

[Pomax]

Node does not come with semver; it's an external package managed through the npm registry. It might be that some linux distros come with a bundle of some handy packages when you install it through their package managers, or run a post-install for node that makes npm install a set of useful packages, but if you install the official Node from https://nodejs.org, or you install it through brew on OSX or using the official Node.js ppa on debian/ubuntu/etc. you just get Node and NPM, nothing else. So pretty much everyone who I work with who's tried installing Calypso so far on their machine has run into bin/check-node-version erroring out due to an unmet requirement.

This should be relatively easy to take care of with a preinstall hook in package.json though:

"scripts": {
 ...
 "preinstall": "npm install semver && node bin/check-node-version",
  ...
}

This will run as part of an npm install call, before the "real" install happens, and any termination with exit code != 0 will cause the install run to abort, so bin/check-node-version can call something like:

if (bad version) {
  console.error("Calypso requires a Node.js v4.2 or higher (you are currently using %s)", nodeVersion);
  process.exit(1);
}

and users will only need to type npm install. If they have the wrong version, it'll abort before all the dependencies get installed automatically.

[nylen]

In every Node.js install I have ever seen, npm is included, and npm depends on semver, so semver is installed. This includes brew install node on OS X.

I'm surprised to hear that there are situations where this isn't true, and I'd like more details about how to reproduce that condition.

[Pomax]

Then all I can tell you is that these are all regular node.js installs on several OSX and Windows machines. We clone the calypso project, we run the make script, things die when we hit require("semver") because it can't be found, and an "npm install semver" solves that problem. So it's probably a great idea still to explicitly add it as preinstall operation with the check-node-version script call.

If I run Node 5.6 on, for instance, windows 7 x64, in a dir without any npm modules installed, semver is not available:

c:\>node
> require('semver')
Error: Cannot find module 'semver'
    at Function.Module._resolveFilename (module.js:339:15)
    at Function.Module._load (module.js:290:25)
    at Module.require (module.js:367:17)
    at require (internal/module.js:16:19)
    at repl:1:1
    at REPLServer.defaultEval (repl.js:252:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:417:12)
    at emitOne (events.js:95:20)

It's entirely possible that npm relies on a local copy of semver, nested in "wherever it lives", in accordance with the NPM3 way of module management. I don't know. All I know is that if you install Node.js through official channels, semver is not available as a globally installed packaged.

[klimeryk]

As seen in the Makefile, we search for the semver package in different places depending on the OS:

• on Windows we search in the base node_modules folder for the node installation
• on OSX/Linux, we use the path provided by npm root -g and go from there

Can you please add an echo $SEMVER_GLOBAL_PATH in the node-version recipe and report the path that gets constructed on the affected systems?

node-version:
    echo $SEMVER_GLOBAL_PATH
    @$(BIN)/check-node-version

That's the path where we expect semver to be installed as part of npm's dependencies. As @nylen pointed out, semver is a dependency of npm and it has to be installed for npm to work - it's not something installed additionally by any other package manager or custom installer. So it's very puzzling that it's missing (or possibly installed somewhere else?).

[Pomax]

I can try to, although I'm still going to recommend not "looking for where the module lives" -that information should be black box information- but just relying on npm being able to do the right thing here: it is really easy to bootstrap the semver package locally with an npm install semver call, and makes it so that you're not tied to doing any guesswork based on different operating systems and FS layouts (especially weird ones you might not have thought of, or ones that change from OS version to OS version).

It'll just work.

Hook it up as preinstall script paired with the node version check, and we're all the way there.

[Pomax]

As an aside, a quick check on Debian 8 with Node 5.6 shows the same as windows:

pomax@debian:~$ node -v
v5.6.0
pomax@debian:~$ node
> require('semver')
Error: Cannot find module 'semver'
    at Function.Module._resolveFilename (module.js:339:15)
    at Function.Module._load (module.js:290:25)
    at Module.require (module.js:367:17)
    at require (internal/module.js:16:19)
    at repl:1:1
    at REPLServer.defaultEval (repl.js:252:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:417:12)
    at emitOne (events.js:95:20)
> 

[klimeryk]

As an aside, a quick check on Debian 8 with Node 5.6 shows the same as windows

That's correct - that's why we use an absolute path (the SEMVER_GLOBAL_PATH exported from the Makefile) when requiring semver in the check-node-version - to pull in semver from npm's dependencies.

[Pomax]

As a cross-platform advocate, I highly recommend making npm do that for you automatically, as part of how it handles installs already. No filesystem mining necessary, so no potential to break on edge cases. Especially as part of this issue; if I tell you what my SEMVER_GLOBAL_PATH var says, is that going to lead to a PR that adds another location to check? Because if so, that would not be the right solution here. I'll be happy to let you know where semver apparently lives on a win7 x64 installation of node, but that knowledge should be filed as "interesting" and not be necessary to perform a script run.

[Pomax]

It looks like "spaces in file names" is biting us here; at least on Windows, Node and NPM install themselves in the Program Files folder (as windows applications should), and the SEMVER_GLOBAL_PATH ends up getting mangled to c/ Files/nodejs//node_modules/npm/node_modules/semver, which will never resolve.

Definitely worth asking npm to handle this.

(No access to an OSX machine atm, I've asked a colleague who ran into this on OSX whether they can check what their SEMVER_GLOBAL_PATH ends up as on their machine. Might not get that answer until Monday though)

[nylen]

It does seem cleaner to do npm install semver && node bin/check-node-version. If you'd like to submit a PR for that, feel free to ping any of us for review.

I bet this check isn't the only thing that fails when paths have spaces in them, though. From the Windows install instructions:

In particular, when installing any of the following tools, be sure to use paths that do not have spaces in them.

[Pomax]

yeah that's not really an option for people who actually work with windows setups, though, unless you have the money to buy separate licenses for different concurrent setups =(

Git, node, etc. all have normal installers that respect Windows' rules on where applications go, for instance, so as good Windows citizens they're going to live in a path with spaces, like every other application. You'd really need a separate install or WinVM for ntentionally installing everything in complete the wrong places, so instructions like these offer Windows support in the same way that a Windows OSS project could note that things are super easy to setup on Windows, and on unix you "just have to make sure to install everything in /sys/dev/char, with superuser permissions". Technically, yes, those are instructions that might work for a unix super, but any unix user will raise three eyebrows to that and then likely go "are you serious right now?" =)

Fixing the semver requirement (despite there being a few more places where spaces in paths might be a problem I understand from previous comments/comments in the PR?) seems to take running the project a long way, so I'll file a PR this weekend, when I have time to look at how much of the make process can actually be done by npm scripts.

[lancewillett]

Closing as no action in several months — feel free to re-open once a pull request is submitted to change things.

[nylen]

The particular problem here was fixed in #4002. Any further problems with the build process should get their own specific issue.