jasongin / nvs

Node Version Switcher - A cross-platform tool for switching between versions and forks of Node.js
Other
2.69k stars 210 forks source link

Command `nvs use` changes characters in Path string #294

Open PfRune opened 7 months ago

PfRune commented 7 months ago

Nvs version: 1.7.1 OS: Windows 10

When using nvs use some characters in the $env:Path will be changed, even though they shouldn't be. My name (and windows user) contains the letter "ø", which in Path this shows up in 2 different ways: either "ø" or "�". The second is more common However when nvs updates the path it will change the letter "ø" -> "ø" and "�" -> "�". The text added at the start of path uses "ø". It does make some sense that the character would be wrong in the added text, but it's pretty weird that it's changed everywhere in the string.

This means that I unfortunately can't use nvs at all.

JinEnMok commented 6 months ago

Can confirm this issue with a Cyrillic username. It seems NVS tries to write the absolute AppData path to the variable, and the non-ascii characters sometimes get mangled in the process.

Wouldn't it be better to instead write %LOCALAPPDATA%, and let Windows expand it into whatever it needs? This has the benefit of being username and language-agnostic.

ZaLiTHkA commented 5 months ago

@PfRune, I just created a dummy folder and added it to my PATH environment variable to test on my Win10 system, and I only this behaviour in the age-old Command Prompt (cmd.exe). I don't see any problems in PowerShell 7 (pwsh.exe) though..

you don't explicitly state which terminal environment you're using here, but your mention of $env:Path does make me think PowerShell... could this perhaps be the built-in "Windows PowerShell" then? what happens if you try with PowerShell 7?


@JinEnMok, unfortunately I don't think that would actually fix it.. this affects all entries in the PATH variable, even if it's not related to the Node instance that NVS is currently adding to the list.

JinEnMok commented 5 months ago

@ZaLiTHkA I'm afraid I don't follow. Are you saying nvs' install would mangle other entries in my Path?

ZaLiTHkA commented 5 months ago

@JinEnMok, depending on which terminal environment you use, that does appear to be the case. but with that said, I highly doubt there are too many people that actually use cmd.exe directly any more.

just for clarity: I have nvs installed through scoop, and I only see this particular issue with Cyrillic characters when I run nvs use <version> from a cmd.exe based terminal. I did not experience this issue through the open source "PowerShell for every system" (currently version 7), not sure about Windows PowerShell (version 5, as bundled with Windows).

JinEnMok commented 5 months ago

I have it installed through Chocolatey, and I used the default Powershell shipped with Win10, running in the standard Windows "Terminal" app (the one downloadable through their app store or whatnot). Only the parts pertaining to nvs were mangled in my case.

I know nothing about JS, but wouldn't this issue be fixed if nvs stopped referencing the username altogether in its path-handling functions? Especially since Windows offers shorthands like %LOCALAPPDATA% anyway

ZaLiTHkA commented 5 months ago

I have it installed through Chocolatey, and I used the default Powershell shipped with Win10, running in the standard Windows "Terminal" app (the one downloadable through their app store or whatnot). Only the parts pertaining to nvs were mangled in my case.

I don't think that would cause any problems here, but there are many things at play here between OS and utility versions... with that said, I'm not entirely sure how the Chocolately installation handles persistent data, Scoop does some fancy footwork with separating "volatile" and "persistent" files into different folders, and then uses symbolic links (or maybe directory junctions?) to link everything back up into one folder so that the application in question can still access it's files at the expected relative paths.

regardless, once NVS is installed, none of that should matter to how it functions..

I know nothing about JS, but wouldn't this issue be fixed if nvs stopped referencing the username altogether in its path-handling functions? Especially since Windows offers shorthands like %LOCALAPPDATA% anyway

as for this part, the logic that NVS uses internally for this starts out in the use module, where the specific use function references paths that are calculated in the settings module, which in turn dips into the windowsEnv (at least, it does on Windows).

there is a chance that something in that flow could be affecting this, abstracting logic away into different modules can potentially hide edge case issues in very mysterious ways, but off the top of my head I can't say for sure..

the fact that you're experiencing this issue with PowerShell and only for paths that relate to NVS directly does help narrow it down at least, but that only tells me where to start looking for potential issues.

I'll dig into this a bit more later this evening (GMT+2) and see what I can find. can't make any promises though, as I've only skimmed through the source code for this project, never tried making any changes myself.

ZaLiTHkA commented 5 months ago

I'll dig into this a bit more later this evening (GMT+2) and see what I can find. can't make any promises though, as I've only skimmed through the source code for this project, never tried making any changes myself.

so, just for a little context: I ended up creating a new local user account to dig into this. my first name is actually André and I know from the OP that ø was also causing problems, so this used ended up being named usérø (so original, I know).

anyway, I then cloned this repo into my user folder and started adding an obscene amount of console.log calls to various points in the lib/*.js files. midnight came and went and I could not find any issues, the path strings calculated in the JS files never got mangled even once.

I decided to sleep on it and have another look this morning.. ended up adding even more logging output, and still found no issues. I was about to give up when I decided to look at the temporary file created at the end of the generate function in lib/postScript.js, which led me to an interesting discovery.

the contents of the temporary ps1 file written to disk at the end of the generate function is correct, but when that file is sourced into the current environment with . $env:NVS_POSTSCRIPT the resulting $env:PATH variable values get messed up. I suspected a file encoding issue, so I bounced a few ideas off Google Gemini and we came up with a workaround.

the file in question is definitely written to disk with UTF-8 encoding, but something with the process of "sourcing" that file gets the encoding mixed up. explicitly setting the encoding to use with $OutputEncoding = New-Object System.Text.UTF8Encoding immediately before updating the environment variable seems to fix the issue.


@PfRune and @JinEnMok, could I ask you to please check this on your side and see if it helps at all? I pushed the fix to a fork of this project.

instructions for "manual install" are in the project's doc/SETUP.md file. keep in mind you may need to remove any other installations (Chocolately, Scoop, etc) before the manual installation process.

JinEnMok commented 5 months ago

@ZaLiTHkA just tried your fix on my normal user, and while the new nvs entry was written correctly, other entries with cyrrilics in them got mangled again.

Tried on a freshly created user, with the same result :(

I've changed all entries with references to my username with either %userprofile% or %appdata%/%localappdata%, this seems to preserve them. Imho, this is the healthier option anyway

ZaLiTHkA commented 5 months ago

@ZaLiTHkA just tried your fix on my normal user, and while the new nvs entry was written correctly, other entries with cyrrilics in them got mangled again.

thanks for checking that.. the only reason I can think for the other paths getting mangled would be nvs.ps1@130, this is the only other place I can find usage of this temporary NVS_POSTSCRIPT. perhaps $OutputEncoding should also be explicitly set to UTF-8 there as well? I'm no PowerShell expert, so that's just wild speculation on my part.

I've changed all entries with references to my username with either %userprofile% or %appdata%/%localappdata%, this seems to preserve them. Imho, this is the healthier option anyway

just to check, where did you make these changes?

I do agree that using built-in environment variables can make things easier, but you can also run into other weird issues. can't remember any by name right now, but in the past I have come across many different third-party tools that also modified my environment's PATH variable, with weird results ranging from changing it from "expandable" to "static", or more commonly just duplicating entries as both %USERPROFILE\custom-path and C:\Users\Andre\custom-path, etc..

tl;dr: Windows environment management is a utter PITA.

JinEnMok commented 5 months ago

@ZaLiTHkA I did it through Windows' standard "Environment variables" menu. I seem to have some stuff there like scoop's own directory, so I just went ahead and changed those

ZaLiTHkA commented 5 months ago

ah, ok.. just keep in mind that NVS searches the array of paths stored in the PATH variable to see what needs to be added and/or removed. so by manually changing these, you could end up with a path that works to access the Node version you want to use, but in a way that NVS cannot detect and therefore will not modify in future calls to nvs use <version>.

with that said, based on what I see here, I suspect that as long as the NVS_HOME variable uses %USERPROFILE% instead of the absolute path explicitly, then you should be ok.

might be an idea to get some feedback from @jasongin himself about this. I had never dug into the source for this project before yesterday, he would be much more familiar with it's inner workings.

ZaLiTHkA commented 5 months ago

just had a random thought. the Scoop installation script allows you to set a custom path for "global scoop apps".

so if you used that, setting this to somewhere other than your "user home" directory, then Scoop would install NVS there instead, bypassing any potential path character issues entirely.