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

Use powershell instead of cmd to launch browser on Windows/WSL #520

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

Pocket-titan
Copy link
Contributor

cmd cannot deal with UNC paths when executed from WSL:

wsl_error

But powershell can! Since it ships with every Windows version >= Windows 7 SP1 || Windows server 2008, I'd say it's a safe replacement.

@fonsp
Copy link
Owner

fonsp commented Oct 2, 2020

Nice!

@fonsp
Copy link
Owner

fonsp commented Oct 2, 2020

So on WSL2 it shows an error, but it works anyway?

@Pocket-titan
Copy link
Contributor Author

It'll work but it will give this warning and execute cmd on the windows path, which will incur a slight delay (probably not relevant on a single start, but still)

@fonsp
Copy link
Owner

fonsp commented Oct 5, 2020

Maybe it's a good idea to use a pipe or pipeline or something to redirect stderr and stdout to devnull, because it really does not matter if the start command failed. I have seen some comments from users reporting an error message from the start command and we shuold just suppress those messages.

Could you experiment? Your computer with cmd is a good environment to test it with :)

@Pocket-titan
Copy link
Contributor Author

I'd be happy to look at those cases but that's kind of a different issue, right? This PR at least fixes this particular warning for UNC, paths instead of suppressing it (which is better, no?). You can redirect stdout/stderr with powershell aswell if we need to do that in the future

@fonsp
Copy link
Owner

fonsp commented Oct 6, 2020

Suppressing stderr would fix the same issue, and also fix some linux problems. Not switching to powershell means less new things that can break

@Pocket-titan
Copy link
Contributor Author

  1. Suppressing an stderr isn't fixing the error, right? The fact remains that cmd doesn't support UNC paths (so it gives an error) and powershell does (so it gives no error). Is it weird to think fixing an error is better than suppressing it?

  2. "Not switching to powershell means less new things that can break"

I'd agree with you in general, were it not that we already have a thing "breaking" with cmd, which is fixed by switching to powershell for this particular command. I worry about things breaking too, that's why I made this PR in the first place!
But you mentioned that

"it really does not matter if the start command failed"

And that we should "suppress stderr/stdout", which to me is like the opposite of caring about things breaking (or am i interpreting this the wrong way?). Sorry but I don't really get why it matters if things break/fail when using powershell, but not when using cmd. In my eyes we could have the "best of both worlds" by both switching to powershell (fixing the UNC path error) and piping stderr to devnull (not caring about any failures which may prevent the command from executing, e.g. on docker systems or something), do you agree?

@fonsp
Copy link
Owner

fonsp commented Oct 6, 2020

Yes, let's do both!

About my comment that switching to cmd might break: I just meant that we tested the cmd line on a few windows versions but not the powershell one, but you are probably right that it does not make sense for powershell to break.

The original code was based on a nodejs package, which I assume is well tested. And they also switched to powershell!
https://github.com/sindresorhus/open/blob/v7.3.0/index.js#L73

As for ignoring stderr: the reason why it would be okay (and the reason why run(...) is in a try catch block that ignores the exception) is that we always print the address to the terminal. If the browser didn't launch then that's fine, but we don't want the user to see a confusing error message.

@fonsp fonsp merged commit 5c4fcb4 into fonsp:master Oct 6, 2020
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.

None yet

2 participants