nwolverson / purescript-language-server

MIT License
184 stars 41 forks source link

On windows 10 should use `Path` variable not `PATH` #133

Closed wclr closed 3 years ago

wclr commented 3 years ago

Problem:

If addNpmPath is set true if it is globally available purs of another version then locally project's installed, build command is using global not local one, if to add npx to buildCommand it works ok.

Cause:

LS provides updated PATH env var when running buildCommand, but on Windows 10 it should insert not PATH but Path, here it is: https://stackoverflow.com/questions/48686267/windows-10-system-environment-variables-path-vs-path

UPD: Though not sure if it is the standard thing about Path or PATH, but my system happens to have Path not PATH in envs. Don't remember I changed it.

@nwolverson Need to decide, how to handle this problem correctly.

1) Set both PATH and Path 2) Update only existing PATH or Path

nwolverson commented 3 years ago

I seem to remember it's not actually that simple, and it might be either Path or PATH, depending on your system setup.

Seems like updating the existing variable (2) is the more correct thing to do. I'm concerned that lookupEnv is the wrong thing in general

wclr commented 3 years ago

I seem to remember it's not actually that simple, and it might be either Path or PATH, depending on your system setup.

Yes, I've already got it, I'm often too quick regarding details.

I'm concerned that lookupEnv is the wrong thing in general

Why is that?

nwolverson commented 3 years ago

If lookupEnv is not performing a case-insensitive lookup, it is not behaving as expected on Windows, where I think the native single-variable lookup facility is case-insensitive, but node returns the whole env. Maybe it's arguable, certainly could be worth raising on node-process but in the meantime can easily have a case-insensitive version here.

wclr commented 3 years ago

lookupEnv just works with Process.env FO object. And seems env variables are case sensitive on windows as well, I actually can add additional PATH so they are independent, and Path seems preferred by the process. Seems it is better to update both PATH and Path (that exist in the env).

wclr commented 3 years ago

I will make a PR for this. Interesting that you are right that there is something strange with this process.env object on windows.

FO.lookup checks if prop in obj and for process.env both is true for "PATH" and "Path" props, and they are both pointing to the property, though Object.keys returns only "Path".

But getEnv uses copyObject that uses Object.assign, and "PATH" in Object.assign({}, process.env) happens to be false, but "Path" is there. So just need to check if copied env contains PATH use it, otherwise use Path.