Prepend instead of changing if shadowed by system dirs (fixes #1652)

[zeorin]

This change modifies nvm_change_path: it makes sure that nvm's directory is in front of any system directories.

I.e. it'll replace the path in place, like it currently does, unless it's being shadowed by system directories, in which case it'll prepend again.

Includes a unit test to make sure there'll be no regression on this.

Fixes #1652.

[zeorin]

I removed the g flag, as I'd only like to change the first occurrence of an nvm path.

[ljharb]

is there a reason we have to effectively hardcode /usr/local here, or is there a way we could infer it?

[zeorin]

On Linux systems, we can count on compatibility with these paths, as they are part of the FHS.

On macOS, the paths that path_helper injects/moves to the front are read from the contents of files at /etc/(man)?paths(.d/*)?. We could read the paths from those files (first checking we're on a Darwin system) to match against, but I don't know that the juice is worth the squeeze:

• harder to maintain
• harder to test
• performance impact

These paths are very unlikely to all be removed from $PATH. Many other tools depend on those paths (I'd guess most of the BSD-based suite of Darwin). Some of them are explicitly listed in Apple's docs, so it's a de facto standard for macOS, too.

Inferring them just to end up at the same result seems like overkill. On the whole, these paths have been an official and unofficial standard for decades, and are so entrenched that changing them will have huge repercussions throughout the *NIX software ecosystem: it's unlikely to happen, and if it does, you'll be able to see it coming.

On another note, I just recalled that nvm_change_path is also used to change $MANPATH, so perhaps I should change the regex from

"(^|:)(/usr(/local)?)?/s?bin:.*${NVM_DIR}/[^/]*${2-}"

to

"(^|:)(/usr(/local)?)?(${2-}|/sbin):.*${NVM_DIR}/[^/]*${2-}"

? Or just leave out /sbin in the regex altogether?

[ljharb]

That's a fair point. Will the inclusion of /usr/local be useful at all on non-darwin systems? If not, could the logic be confined to darwin?

I'm content with whichever of the regexes you think is best.

[zeorin]

It is possible to do that, but not checking the OS would also protect us from any other OS introducing behaviour similar to macOS (who knows what Windows is going to end up doing?).

I think that I'll leave out /sbin, as system node doesn't install binaries there anyway.

[ljharb]

nvm already has darwin detection via nvm_get_os; the question was more, can we branch the implementation on that.

[zeorin]

We can, but I don't see any advantage in that.

Currently, the test for being shadowed by system directories is OS-agnostic. I think that's a good thing. If another OS introduces similar behaviour (whether as a bug, as in macOS's case, or intentionally), the way it currently is, we'll be able to handle that situation just fine.

If we do isolate this test to macOS, we lose that. If another OS introduces this behaviour, we'd have to fix it again. In addition to that, isolating this to macOS makes it harder to test. We'd have to mock the OS in order for the unit test I modified to be able to catch any regressions.

I don't have any strong feeling against isolating this fix to macOS, but I'm not sure it's better than just leaving it as is. What is your thinking in isolating this?

[ljharb]

Mocking the OS is easy because it's all behind nvm_get_os.

I'm concerned about unintentional consequences on non-Mac OS's, and I'm not concerned about the likelihood of a new, unknown OS popping up that replicates the problematic behavior.

That said, I'm not opposed to keeping it as-is - I just want to make sure we've explored it fully.

[zeorin]

The change is:

If we're shadowed by standard system dirs, shadow them instead.

Before my initial #1316 PR, that was already happening implicitly (because then the behaviour was: "shadow all the paths, all the time").

The only consequence our current change will lead to is to ensure that we always shadow standard system dirs, which is intentional. If we don't do that, we can't ensure that the user is using the intended version of node.

I think we've explored it fully 😊. Are you happy for me to make the regex change and push in prep for a merge?

[ljharb]

Sounds great to me, thanks :-) We'll need to get tests passing still.

[zeorin]

I rebased on master and changed the regex to omit checking for /sbin. The tests are now passing (although previously I don't think they weren't passing because of any code changes I made: something timed out during the tests).