run-script: Add option to ignore the current node executable for PATH

[addaleax]

I know a lot of time has already been spent arguing about this topic, but I still think this is the right thing to do and I at least want to propose it. No hard feelings if you decide to turn this PR down on principle!

Also, ping @isaacs on whether he’d accept a patch like addaleax/spawn-wrap@e4294c0 that could, in the long run, replace all other npm-specific hacks that needed to be built into spawn-wrap in the past few months.

---

Add a --scripts-ignore-node-path option that makes npm assume its behaviour from prior to v3.7.0, namely not adding the directory of the current node executable to the PATH, even if that means that npm will invoke a different node executable than the one which it is running.

This is a non-invasive solution to some of the problems described in #12318. It should not come with any drawbacks for npm’s side, as it shifts the responsibility of making sure that the PATH variable contains everything it should to whoever invoked npm with --scripts-ignore-node-path.

While #12968 may have addressed some of the problems described there, it does not help in the case that the PATH has explicitly been overridden with the intention of having a different node executed (This is the case for spawn-wrap).

[addaleax]

I assume the needs-discussion label @iarna added here refers to some kind of internal discussion on your side, so… is there anything you’d like to know/anything I can help with?

[zkat]

That's part of what the needs-discussion label means, although it often also means "we need to talk with the rest of the community about it".

I'll flag this again and see if we can discuss whatever made us want to discuss it. Thanks for the patch! :)

[addaleax]

Aye, thanks for the explanation!

[othiym23]

The CLI team finally had an opportunity to discuss this. Previous discussions:

• Prefer current node in run-script #11175
• Use node version from current process when run node-gyp #9253
• npm run-script pollutes $PATH #5876
• npm should invoke node-gyp using process.execPath #2680

As you can see, this has been a long and thorny discussion, and we've reversed ourselves a number of times. Our first task was to look at the discussion thus far, and determine whether it's possible to resolve this issue without adding additional configuration (i.e. find some kind of default behavior that will satisfy all of the use cases enumerated in this discussion).

The answer to that appears to be "no", because there are two conflicting use cases:

1. Node ecosystem users, who want to ensure that the same version of Node is being used for npm and all the Node-based tooling called in run-scripts
2. People using tools that rely on other runtime environments with their own tools for managing the versions of those runtimes. The two most obvious examples are Ruby tools like rbenv and Python's virtualenv.

If we were able to determine at runtime unerringly that a tool is meant to be run with Node, then we could just invoke those scripts with the execPath for the current Node. Unfortunately, given the way that Node scripts use shebangs and / or cmd-shim, and given that users might want to hardcode a given Node path for their own reasons, npm can’t determine this without using heuristics that might cause an even weirder and harder-to-deal with set of edge cases for those users. So that’s no good.

Therefore, yeah, this needs to be something configurable, and put in the hands of the end user, who has the most information available to determine what behavior is going to break the fewest things. Additionally, @zkat made a persuasive case that the most reasonable default behavior is to not prepend execPath to the runtime path, as the number of people who care about this is relatively small next to the set of people who don’t want their runtime path munged (see @sciyoshi’s correct point on #11175 (comment) that the current behavior is a regression). Therefore, the default should be to not change the path, flipping the logic in your patch. This is a breaking change, but the time is nigh for npm@4, so I don’t think this PR would have to wait too long to get merged.

One last thing that would be extremely helpful for users would be to add to the code that determines which version of node to use a check to see whether execPath is included in the current PATH, and to print a warning telling users that they may want to be using this option (--prepend-exec-path?) when they’re explicitly invoking a particular Node version to run npm. It would be fantastic to include that in this path.

These are a bunch of changes, and I understand if that’s too much for you to pick up and change here, but this is what we’d like to see land as part of npm@4. Is this something you’d be interested in banging out? Either way, thanks very much for what you’ve done thus far.

[addaleax]

Is this something you’d be interested in banging out?

Yes! It’s late here and I’m busy tomorrow, so give me a few days, but I still care about this and feel like I’m already getting more than I asked for.

One last thing that would be extremely helpful for users would be to add to the code that determines which version of node to use a check to see whether execPath is included in the current PATH, and to print a warning telling users that they may want to be using this option (--prepend-exec-path?) when they’re explicitly invoking a particular Node version to run npm. It would be fantastic to include that in this path.

Ack! I’ll definitely think about this in more detail, though, because doing this in the naïve way would probably mean showing that warning to everyone who runs something like nyc npm test in a perfectly valid way.

To clarify, you’d like to see:

• No changes in npm v3.x and v2.x? My original motivation for this PR was that it would allow nyc/spawn-wrap to support npm >= 3.7.0 without a series of really weird hacks…
• Basically the the 1:1 inverse logic of this PR? Sounds doable.

[othiym23]

No changes in npm v3.x and v2.x? My original motivation for this PR was that it would allow nyc/spawn-wrap to support npm >= 3.7.0 without a series of really weird hacks…

If you want access to this in npm@3 and npm@2 for spawn-wrap, which seems reasonable, this patch, as is, is only a semver-minor change. That said, immediately reversing the behavior by changing the defaults in npm@4 seems liable to cause some community whiplash, so it comes down to which is more important: ironing out the kinks and regressions introduced by the most recent change, or fixing things for nyc / tap / spawn-wrap users. Right now, I tend towards the former, mostly because I think that fixes more things for more people.

Basically the the 1:1 inverse logic of this PR? Sounds doable.

Correct.

Thanks for being flexible!

[addaleax]

so it comes down to which is more important: ironing out the kinks and regressions introduced by the most recent change, or fixing things for nyc / tap / spawn-wrap users

Yeah no, of course the former is most important; UX > code aesthetics, no doubt about that. I think most of the time I can tell my personal motivation from what’s important in the big picture pretty well, even if they are not the same. ;)

I think I was just misunderstanding you – what I was trying to say is that I’d like to see ways to explicitly tell npm which behaviour to assume (always prepend to PATH/never prepend) that work across versions, and I think that’s in the interest of npm’s regular user base, too.

I’ve been busy today, but I should be able to update this PR tomorrow. (And just to be sure we’re on the same page: the 1:1 reversed logic of this would be an option that tells npm to auto-determine whether it should prepend node’s directory to the PATH, and default to not doing so when that option is not set. At least that’s what my brain, which has definitely endured too much math-y reasoning, thinks? 😄)

Sorry if you feel like I’m holding anything up here!

[othiym23]

Sorry if you feel like I’m holding anything up here!

There's no rush! The CLI team decided, in our usual, free-wheeling manner, that our target release date for npm@4 is early October, and since this is a breaking change, that's when it would get rolled out.

I think I was just misunderstanding you – what I was trying to say is that I’d like to see ways to explicitly tell npm which behaviour to assume (always prepend to PATH/never prepend) that work across versions, and I think that’s in the interest of npm’s regular user base, too.

As an nyc and tap user, I agree with you here; it should be possible to implement things in such a way that all that gets flipped in npm@4 is the default, rather than anything else. I'm a little reluctant to land this in npm@2, because we'd really like to get everybody onto npm@3, especially as we prepare to release npm@4, but Node LTS is a thing sighs forever.

[addaleax]

So, I’ve updated this! I’ve called the option --scripts-prepend-node-path for now based on your suggestion, but still including the scripts prefix because imho it makes it more obvious what the option is supposed to do. I don’t feel strongly about it, though.

I’ve expanded it a tiny bit to address all the use cases that are at the table (I think), to have auto (for the current behaviour) and warn-only (based on your suggestion above) values besides the always/never boolean. The only thing that’s really breaking here is contained in the last commit, which just flips the default to warn-only from auto. Again, I don’t feel strongly about the names here! :)

So… yeah, please review and let me hear your thoughts. I know it seems like the complexity has increased a bit here, but the diff for the actual logic is no bigger than +23/-3, I’d consider that okay.

I'm a little reluctant to land this in npm@2, because we'd really like to get everybody onto npm@3, especially as we prepare to release npm@4, but Node LTS is a thing sighs forever.

That’s your call, for the nyc/spawn-wrap side it’s going to be a long-term thing anyway, and it s really is understandable that you’d like users to migrate to npm@3.

And on the chance of going overly off-topic here… if your plan is releasing npm@4 in October, does that mean you’d like to see that shipped with Node v6 LTS, if that’s possible/reasonable? I know you have been unhappy with the npm@2 situation a while, and if you think continued support for npm@3 might put you in the same position, maybe it’s worth talking about that?

[zkat]

This was merged into release-next and will be included in npm@4. #14334 is nudging things a little, but this patch works as-is. Thanks again for the excellent work, @addaleax!