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

Remove usage of wslu #217

Merged
merged 1 commit into from Jan 30, 2021
Merged

Conversation

anaisbetts
Copy link
Contributor

@anaisbetts anaisbetts commented Jan 29, 2021

This PR is simplified version of #198 - Windows 10 does not allow installing the OS to locations other than C:\Windows so fetching %systemroot% is unnecessary here (and since WSL is exclusive to Windows 10, we do not need to worry about Win7 here). WSL also handles marshaling paths between WSL and Windows, so we don't need to convert this path manually.

Fixes #198
Fixes #200
Fixes #204

This PR is simplified version of sindresorhus#198 - Windows 10 does not allow
installing the OS to locations other than C:\Windows so fetching
%systemroot% is unnecessary here (and since WSL is exclusive to
Windows 10, we do not need to worry about Win7 here). WSL also handles
marshaling paths between WSL and Windows, so we don't need to convert
this path manually.
command = String.raw`${windowsRoot}\System32\WindowsPowerShell\v1.0\powershell${isWsl ? '.exe' : ''}`;
command = isWsl ?
'/mnt/c/Windows/System32/WindowsPowerShell/v1.0/powershell.exe' :
`${process.env.SYSTEMROOT}\\System32\\WindowsPowerShell\\v1.0\\powershell`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is for standard Win32, where we may be running under Win7 where installing to a different location may be possible - luckily we can just fetch %SYSTEMROOT% here

@sindresorhus sindresorhus changed the title Remove usage of wslu Remove usage of wslu Jan 30, 2021
@sindresorhus sindresorhus merged commit 315a480 into sindresorhus:main Jan 30, 2021
@sindresorhus
Copy link
Owner

Thanks :)

@jonaskuske
Copy link
Contributor

Seeing this too late, but:

Windows 10 does not allow installing the OS to locations other than C:\Windows so fetching %systemroot% is unnecessary here

The Windows location is configurable (in WSL) – so while this PR hard-coded /mnt/c/, it could also be just /c/ or /windows/c/ or anything else! The wslpath utility from wslu (that was removed by this PR) handled this case, so did the previous PR (that was closed in favor of this PR). We'll need an alternative – I guess parsing /etc/wslconf in JS as suggested in the review here is still an option?

@anaisbetts
Copy link
Contributor Author

anaisbetts commented Feb 1, 2021

Why would anyone do this other than a desire to have their tools be less reliable?

@jonaskuske
Copy link
Contributor

jonaskuske commented Feb 1, 2021

@anaisbetts
When WSL was first introduced, mounting your drives directly into / was necessary if you wanted to run Docker, that's why it's still set up this way on some of my devices. Don't know if any (non-legacy) reasons to do so remain... But it's definitely still supported, even now with WSL2: https://docs.microsoft.com/en-US/windows/wsl/wsl-config#automount

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error launching chrome from WSL
3 participants