Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(env): use unix paths in windows bash, zsh, fish #724

Closed
wants to merge 5 commits into from
Closed

fix(env): use unix paths in windows bash, zsh, fish #724

wants to merge 5 commits into from

Conversation

ariscript
Copy link

Related to #390.

Fix environment variable generation for fnm env in Windows.

Cygwin, MSYS, and Git Bash use UNIX-style paths (like /c/Users/username/AppData/Local/fnm_multishells), but fnm generates Windows-style paths (like C:\Users\username\AppData\Local\fnm_multishells) in Windows, regardless of the shell.

With these changes, fnm can convert paths to UNIX using cygpath in bash, zsh, and fish, but leave Windows paths alone in pwsh and cmd.

src/commands/env.rs Outdated Show resolved Hide resolved
src/commands/env.rs Outdated Show resolved Hide resolved
src/pathutils.rs Outdated Show resolved Hide resolved
src/pathutils.rs Outdated Show resolved Hide resolved
src/shell/bash.rs Outdated Show resolved Hide resolved
src/commands/env.rs Outdated Show resolved Hide resolved
src/shell/zsh.rs Outdated Show resolved Hide resolved
@Schniz Schniz added PR: Bugfix A bug was fixed os:windows related to Windows labels Jul 3, 2022
@Schniz
Copy link
Owner

Schniz commented Jul 4, 2022

I forgot to say thanks in advance for this contribution. Really good stuff!

@changeset-bot
Copy link

changeset-bot bot commented Jul 4, 2022

⚠️ No Changeset found

Latest commit: 690b13f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ariscript
Copy link
Author

Adding new flags --wsl and --cygwin for fnm env might help solve #758 as well, because I can't find a great way to detect if the shell is run in something cygwin-based or WSL reliably.

Running ./target/debug/fnm.exe env --shell bash in WSL works when launched from Windows Terminal, but if I open WSL in Git Bash (presumably works the same in other cygwin-based shells) cygpath is being found so it generates paths that won't work in WSL.

Copy link
Owner

@Schniz Schniz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it's not a shell concern but environment one. I'm sorry that I'm requesting changes like a pendulum but I'm thinking about the problem as I'm reviewing the code. Really appreciate the contribution!

@@ -48,4 +49,8 @@ impl Shell for PowerShell {
fn to_clap_shell(&self) -> clap_complete::Shell {
clap_complete::Shell::PowerShell
}

fn format_path(&self, path: &str) -> Result<String> {
Ok(path.to_string())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

powershell can be run on Mac and Linux (not sure about usage though). Maybe we need to call unixpath here too? We probably can't know if we're in WSL or Cygwin, or do we? based on the infer_shell infra and the process-walking method?
What I'm thinking is--maybe it's not shell-related, but environment related, and therefore if we have this information?

We can start without any inference, so it will require the following, if one wants to use the Windows version of fnm within Cygwin or WSL:

fnm env --path-formatter=cygwin
fnm env --path-formatter=wsl

what do you think about it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Powershell actually uses the correct path format for each platform so we don't have to convert the path to the right format. The windows shell inference code that we have breaks when run in WSL, so inferring isn't possible without reworking that somehow and I can't think of any way to do that currently, but for now I can implement it with flags.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I figured out a way to detect if the executable is being run from WSL, but inferring the shell is not possible and the user needs to provide it as a flag (maybe there can be a custom error message for it).
Detecting WSL works the same way as shell inferring, at some point up the parent process chain there is a process named "wsl.exe", but there's no information on the shell being used.

src/shell/zsh.rs Outdated Show resolved Hide resolved
@ariscript
Copy link
Author

I just realized running fnm.exe in WSL breaks the main point of WSL existing. Running node.exe would run the Windows version of node. so it would be no different from running it in Windows but with bash/zsh/fish (at which point using something cygwin-based like I do would be more useful). Without a lot of work, getting the Linux version of node to install in WSL even when the Windows executable is being used isn't really possible, and it requires basically writing another version of fnm.

@Schniz
Copy link
Owner

Schniz commented Jun 29, 2023

Superseded by #960 , sorry :pray_parrot:

@Schniz Schniz closed this Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os:windows related to Windows PR: Bugfix A bug was fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't use fnm for Windows in WSL bash
2 participants