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 windows subsystem #8

Closed
wants to merge 2 commits into from
Closed

Conversation

henvic
Copy link

@henvic henvic commented Aug 23, 2017

I think this is an acceptable solution for this (though awkward). I can't think of anything better.

It might be related to the error described on #6.

Please see:
https://blogs.msdn.microsoft.com/commandline/2016/10/19/interop-between-windows-and-bash/
microsoft/WSL#423

@henvic
Copy link
Author

henvic commented Aug 23, 2017

By the way, usually you expect nothing to be printed to your stdout when calling commands on an API like this one[, but error].

The reason I am removing it is because Windows does print something.

Copy link
Member

@davecheney davecheney left a comment

Choose a reason for hiding this comment

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

not lgtm. Thanks for looking at this but the changes to make this work on windows (I'm honestly not sure if it ever did, i'm not a windows user) should be constrained to files ending in _windows.go. There shouldn't be any changes to other os's.

@henvic
Copy link
Author

henvic commented Oct 12, 2018

I agree it should be constrained to files ending in _windows.go and makes little to no sense to have them on a file ending in linux.go, but we should blame Windows subsystem on it as there seems to be no standard way to detect it (and the program identifies it as Linux) ¯_(ツ)

Probably there's not much to do regarding this.

@paultyng
Copy link

A little necromancy here, but as WSL usage is growing, would be nice to solve it somehow. Can confirm the fix does need to be in the *_linux.go files, even though its targetting a Windows.

alisdair added a commit to hashicorp/terraform that referenced this pull request Feb 13, 2020
Windows Subsystem for Linux failed to open a browser due to
lack of support for xdg-open. This commit reuses a solution from
pkg/browser#8 which detects a WSL
environment and uses cmd.exe to open the URL instead.
alisdair added a commit to hashicorp/terraform that referenced this pull request Feb 13, 2020
With the current implementation of terraform login, Windows Subsystem
for Linux fails to open a browser due to lack of support for xdg-open.
This commit reuses a fix from pkg/browser#8 which detects a WSL
environment and uses cmd.exe to open the URL instead.
alisdair added a commit to hashicorp/terraform that referenced this pull request Feb 14, 2020
With the current implementation of terraform login, Windows Subsystem
for Linux fails to open a browser due to lack of support for xdg-open.
This commit reuses a fix from pkg/browser#8 which detects a WSL
environment and uses cmd.exe to open the URL instead.
@jetersen
Copy link

jetersen commented Jun 12, 2020

This should use wslview instead of cmd.exe

@henvic
Copy link
Author

henvic commented Jun 24, 2020

@jetersen,

Perhaps this?

  1. Try xdg-open.
  2. Fallback to wslview.
  3. Fallback to cmd.exe (to still support WSL 1?)

I don't maintain anything supposed to run on Windows anymore, so maybe you or someone else could continue from here?

Copy link
Member

@davecheney davecheney left a comment

Choose a reason for hiding this comment

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

Thank you for this change. I don't think this change should touch browser_linux.go. Can you please move the code to a more appropriate location. Thank you.

@luna-duclos
Copy link
Collaborator

Hey @davecheney , I'm looking at creating a new PR to do this properly, how do you envision doing this without touching browser_linux ? Since WSL uses GOOS linux

@luna-duclos
Copy link
Collaborator

Tidying up, Closing this as very old, please reopen if still relevant.

@luna-duclos luna-duclos closed this Dec 9, 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.

5 participants