RFE: Use #!/usr/bin/env node for npm executable

[isaacs]

In the node installer, the shebang in npm's executable is rewritten from #!/usr/bin/env node to #!${PREFIX}/bin/node (where ${PREFIX} is node's install prefix).

I have come to believe that this is a mistake.

1. It violates user expectations.

   The PATH environment variable is used to determine where the system looks for an executable. If I've placed a node binary at a place earlier in the PATH, then I should expect that this will be the node that is used to execute node scripts. If a different node is used, then that's unusual, at least. Most unix script programs use /usr/bin/env $PROGRAM so that the user's preferred version of the executable will be used.

2. It is unnecessary.

   IIRC, this was originally added because SmartOS systems installed a "system" node (of version 0.2) at /usr/bin/node and a "user" node (of a higher version) at /opt/local/bin/node, and wanted to ensure that the npm script in one location didn't inadvertently call the node at the other location.

   However, (a) system installers like this are more than capable of floating patches for such situations, and (b) system-level scripts that call /usr/bin/npm can just as easily call /usr/bin/node /usr/bin/npm to force the version of node that will be invoked. That is the correct place to be solving this issue, not in node's installer.

3. It is less portable.

   Because npm is edited in the install process, hard-coding the shebang to a specific path to the node executable, you can't move the install location to another place without re-invoking the installer. If it simply left the npm binary alone, then the installation would be more portable, and could be moved anywhere, as long as node is in the PATH. And if node is not in the PATH, then of course the user will have to invoke node via absolute path anyway. (In that case, they're also invoking npm via absolute path, so they're well off the beaten track.)

   In order to maintain this absolute-shebang contract in the "portable" pre-built tarballs of node, it's necessary to replace the npm script with a sh script that calculates the location of the current script, and invokes the node binary explicitly. This layer of indirection is unnecessary complexity, added to deal with the fact that we're solving a problem at the wrong layer. It is a really clever hack to preserve the contract that npm is always executed by the node it ships with, but also, it's a way too clever hack, and the smell indicates that this contract itself is problematic.

4. It makes test coverage wrappers (and other analytics) more difficult (or impossible).

   In order to provide test coverage for node programs, in such a way that does not break when a node program is invoked in a child process, nyc uses spawn-wrap, which monkey-patches the built in spawn machinery in node to make it load a shim script whenever executing node. It also modifies the PATH to ensure that shim is the first node found by the child process.

   Since the shebang is explicitly set to /usr/local/bin/node, this doesn't work when doing spawn('npm'), because the PATH is not used to look up the node binary. In order to work around this situation, spawn-wrap now has to read the script being invoked, and determine if its shebang is an absolute path to node, and then act accordingly.

   However, if the user does exec('npm') instead, we're pretty much lost, because then the actual spawn will be /bin/sh -c "npm", and the npm found in the PATH won't be pulling in the node found in the PATH.

   In the "portable" node builds, it's of course completely ridiculous. So the spawn-wrap module now just says, "Oh, npm? That's probably a node program. Let's not even bother checking."

   Say what you may about monkey-patching builtins, I think we can all agree that, in general, test coverage is a Good Thing.

Suggestion: Just ship the npm executable as it is delivered by the npm cli team. Remove the shebang hacking in scripts/install.py.

This is a breaking change, albeit one that is long overdue and corrects a subtle and significant defect. I'd suggest getting it on the roadmap for 6.0 as soon as possible, so that platform maintainers have ample time to adjust accordingly.

• Version: 5.x and before
• Platform: all
• Subsystem: installer

[jbergstroem]

I agree with setting it to /usr/bin/env. Although it might be considered breaking as in semver-major, I don't think this will have a big impact seeing how most packagers set prefix to /usr (which raises another problem with npm's global config creating /usr/etc, but that's another issue).

[bnoordhuis]

I guess it's fine to leave the shebang untouched. At the risk of sounding obvious, this is a breaking change for people that have an old node binary on their path. That doesn't particularly bother me except that we'll end up being first-line support for such people.

[jbergstroem]

@bnoordhuis: If packagers does that I'd suggest they patch instead of us.

[isaacs]

this is a breaking change for people that have an old node binary on their path.

Indeed. The good news is that the npm cli team tests regularly going back even further than Node LTS, I believe, and we often recommend old node users upgrade npm anyway. So from node's point of view, you should be fine. (That is, Node v0.8 is still a first class citizen, and probably will be for some time, as long as we keep seeing registry traffic from it.)

[jbergstroem]

I guess it might also be worth noting that installing mutliple node binaries isn't really supported. We need to have something fancier in place to handle that properly.

[TooTallNate]

Since the original intent seems to be forgotten: we added this so that the
npm included in the precompiled tarballs would use the version of node that
it's bundled with.

On Friday, April 8, 2016, Johan Bergström notifications@github.com wrote:

I guess it might also be worth noting that installing mutliple node
binaries isn't really supported. We need to have something fancier in place
to handle that properly.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#6095 (comment)

[TooTallNate]

Ignore me; I was referring to the PORTABLE thing in the install script, but
now I don't think that's what this issue is about (I'm tired and on a
plane).

On Friday, April 8, 2016, Nathan Rajlich nathan@tootallnate.net wrote:

Since the original intent seems to be forgotten: we added this so that the
npm included in the precompiled tarballs would use the version of node that
it's bundled with.

On Friday, April 8, 2016, Johan Bergström <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

I guess it might also be worth noting that installing mutliple node
binaries isn't really supported. We need to have something fancier in place
to handle that properly.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#6095 (comment)