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

Make it work on Windows with WSL, MSYS, Cygwin #2391

Merged
merged 2 commits into from
Apr 16, 2021
Merged

Conversation

nmarghetti
Copy link
Contributor

No description provided.

@nmarghetti nmarghetti force-pushed the master branch 2 times, most recently from 632b42a to 302cc4c Compare December 26, 2020 23:36
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This is an unexpected and potentially very large feature to add.

i could use more explanation, but also, this will definitely need tests so that windows support isn’t accidentally broken later.

install.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
@ljharb ljharb added feature requests I want a new feature in nvm! installing node Issues with installing node/io.js versions. installing nvm Problems installing nvm itself OS: windows labels Dec 26, 2020
@nmarghetti nmarghetti force-pushed the master branch 3 times, most recently from d669c3a to 68cca1b Compare December 27, 2020 01:30
@nmarghetti nmarghetti changed the title Make it work on Windows for GitBash and Cygwin Make it work on Windows for GitBash (MSYS) and Cygwin Dec 27, 2020
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This PR sjould also add CI that runs in windows as well (using Github Actions, ideally).

nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
@nmarghetti
Copy link
Contributor Author

I will try to add a CI with GitHub action to test MSYS, Cygwin and WSL

@nmarghetti nmarghetti changed the title Make it work on Windows for GitBash (MSYS) and Cygwin Make it work on Windows with WSL, MSYS, Cygwin Dec 28, 2020
@nmarghetti nmarghetti force-pushed the master branch 4 times, most recently from bd10ec4 to 7f15005 Compare December 28, 2020 16:21
@nmarghetti nmarghetti requested a review from ljharb December 28, 2020 16:32
@nmarghetti
Copy link
Contributor Author

I managed to add a CI for WSL and MSYS.

@nmarghetti nmarghetti force-pushed the master branch 4 times, most recently from 79fe611 to 2ab05e8 Compare December 28, 2020 20:31
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

nvm_ensure_posix_path and nvm_install_binary_extract both need unit tests.

.github/workflows/windows-npm.yml Show resolved Hide resolved
.github/workflows/windows-npm.yml Show resolved Hide resolved
.github/workflows/windows-npm.yml Outdated Show resolved Hide resolved
.github/workflows/windows-npm.yml Outdated Show resolved Hide resolved
.github/workflows/windows-npm.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
nvm.sh Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
@nmarghetti nmarghetti force-pushed the master branch 2 times, most recently from f43618f to 6c5456c Compare January 1, 2021 18:23
@ljharb
Copy link
Member

ljharb commented Feb 15, 2021

I extracted out d396181 and 7f6c0c0 and landed them on master, and then rebased this.

nvm.sh Outdated Show resolved Hide resolved
nvm.sh Show resolved Hide resolved
nvm.sh Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
@nmarghetti nmarghetti force-pushed the master branch 2 times, most recently from f54b3cd to 8d21c86 Compare February 21, 2021 22:18
@ljharb
Copy link
Member

ljharb commented Feb 21, 2021

@nmarghetti did you mean to rebase out everything except the last test?

@nmarghetti
Copy link
Contributor Author

nmarghetti commented Feb 21, 2021

@nmarghetti did you mean to rebase out everything except the last test?

I just added this commit to ensure the PR passes (as I am on Windows, I wanted to be sure).
Now that I know this commit is ok, I am adding the other one too ;)

@nmarghetti
Copy link
Contributor Author

I will update it again to add Windows test for nvm_is_version_installed.
Also, should I remove nvm_windows_drive_posix and nvm_ensure_posix_path ?
If we use

NVM_CONFIG_VALUE="$(cd "$NVM_CONFIG_VALUE" 2>/dev/null && pwd)"

instead of

NVM_CONFIG_VALUE="$(nvm_ensure_posix_path "$NVM_CONFIG_VALUE")"

then it is not needed anymore, but it might still be useful later (not really sure if there are not other places where it needs to convert some Windows path)

@nmarghetti nmarghetti force-pushed the master branch 2 times, most recently from 581b82b to a25b66a Compare February 21, 2021 22:57
@nmarghetti
Copy link
Contributor Author

I have added a "Cleanup" commit to remove nvm_windows_drive_posix and nvm_ensure_posix_path.
Should I squash that commit or should I remove that commit ?

@nmarghetti
Copy link
Contributor Author

In the end I squashed it, I still have the code on another branch if needed...

@ljharb
Copy link
Member

ljharb commented Feb 21, 2021

Sounds great. I’ll likely end up rearranging the commits a bit later anyways :-)

@nmarghetti
Copy link
Contributor Author

@ljharb what would prevent it to be integrated ?

@ljharb
Copy link
Member

ljharb commented Apr 16, 2021

master's been broken for a few weeks; it's fixed now. i'll rebase and take a look.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This is a big one, thanks.

If it ends up not producing a flood of bug reports from windows users who've tried it, I owe you a beer or five :-)

@ljharb ljharb merged commit f258227 into nvm-sh:master Apr 16, 2021
@PabloSzx
Copy link

@ljharb when are you planning to release it?

@ljharb
Copy link
Member

ljharb commented May 21, 2021

When I have time to handle the incoming bug reports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature requests I want a new feature in nvm! installing node Issues with installing node/io.js versions. installing nvm Problems installing nvm itself OS: windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants