fix(windows): revert writing all possible cases of PATH variables

[JimiC]

Fixes #20

[JimiC]

@zkat I'm really wondering what's holding the merge. We've been waiting for this bug to get fixed for some time now.

[zkat]

There's been a bit of discussion as to what this was originally meant to fix, and whether there's a better way to address it, as well as why this patch broke things in the first place: https://npm.community/t/3-path-variables-are-assigned-to-child-process-launched-by-npm/1042

I think the way forward right now is to make sure all the questions are resolved, so we don't go back to breaking things but in a different way (which I believe is what this patch would do as-is, right now).

[JimiC]

The PR that introduced the bug was #17 supposedly to fix the issue #16. As I explained in this comment there is no reason to declare 3 different path variables on Windows. Considering that others have issues with that change, the logical way is to revert the change and ask @laggingreflex about the deep nature of his issue. Note here that I cloned his repro repo (https://github.com/laggingreflex/npm-lifecycle-windows-path-issue) and couldn't reproduce his issue.

[laggingreflex]

Hi there, I submitted the original PR. Well this is embarrassing! Surprisingly I can't seem to reproduce my own issue either now (I've tested node v8-10, npm v5.8-current).

I'm still not sure what was going wrong that day, but I saw two path variables - Path and PATH (on process.env) and even though npm was writing to one of them, Windows seem to be using the other one (I think I set the command to echo %Path% to confirm this). Which is what prompted me to fix it by just writing to all possible "Paths".

Another thing I can remember is that in "System properties > Advanced > Environment Variables" dialog, the "User variables > PATH" was different from "System variables > Path". I can't reproduce this either now (both are Path for me currently) but a quick search on google seems to confirm that they can indeed be different: 1, 2. I'm guessing this is probably the root cause of all this, or has something to do with it.

[JimiC]

@laggingreflex @zkat Now this clarifies things. From my investigation, I can assure you that there is no need to do anything as node combines user and system variables, in order to construct process.env. On my machine, I have both user and system path variables. It doesn't matter what casing they have, I tested with both Path and PATH on both variables. The end result is node writing always a process.env.Path (in that exact casing) and combining the path variable values into one.

Conclusion: The problematic code can be safely removed to restore functionality on Windows. The existing code does the necessary handling.

Note: In order to avoid any confusion with other env variables, note that node doesn't treat user and system TMP and TEMP variables, as it does with path. In this case, user variables are preferred.

[jordanbtucker]

Found this bug in the wild.

felixrieseberg/windows-build-tools#135

[zkat]

👍 looks like this is the best way forward. Thanks all for taking the time to look into it so we could be more careful/intentional about this change this time around. Hopefully this makes like generally easier for win32 users ^_^