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

cmd/bug: does nothing if there is no windowing system #19131

Closed
broady opened this issue Feb 16, 2017 · 20 comments
Closed

cmd/bug: does nothing if there is no windowing system #19131

broady opened this issue Feb 16, 2017 · 20 comments
Milestone

Comments

@broady
Copy link
Contributor

broady commented Feb 16, 2017

What version of Go are you using (go version)?

go1.8

What operating system and processor architecture are you using (go env)?

linux/amd64

What did you do?

Run go bug on a headless system.

What did you expect to see?

Anything.

What did you see instead?

Nothing.

$ go bug
$
@bradfitz
Copy link
Contributor

Let's bug @josharian.

@rakyll
Copy link
Contributor

rakyll commented Feb 16, 2017

/cc @shurcooL

@dmitshur
Copy link
Contributor

$ go bug
$

What was the exit code from go bug (ala echo $?)?

@broady
Copy link
Contributor Author

broady commented Feb 16, 2017 via email

@josharian
Copy link
Contributor

go bug should have handled the fallback case where there was no browser: https://github.com/golang/go/blob/master/src/cmd/go/internal/bug/bug.go#L63.

Would you mind instrumenting the browser opening code and finding out what's happening there? See https://github.com/golang/go/blob/master/src/cmd/internal/browser/browser.go#L38. I'd be curious about which browser command is succeeding, and what happens if you run it manually.

@rakyll
Copy link
Contributor

rakyll commented Feb 17, 2017

@broady,

Can you share any more information about the headless system you are on? I cannot experience this neither when I am ssh'ing into Debian or Ubuntu.

@ronin13
Copy link

ronin13 commented Feb 19, 2017

Able to reproduce on a headless system. web.OpenBrowser doesn't check if the command succeeds or not (only that it is runnable) and doesn't print any stderr, so if any of the browsers in https://github.com/golang/go/blob/master/src/cmd/internal/browser/browser.go#L17 are installed (xdg-utils (providing xdg-open) is the one most probably installed on a headless system), it will silently fail on headless system with exit code of 0 without printing the body.

BROWSER=/usr/bin/false go bug
echo $?
0

It is better to log the browser being used to open (in browser.go) and/or to provide a -headless option to go bug to just print. (otherwise, checking DISPLAY and printing is not portable).

@josharian
Copy link
Contributor

This will impact any command that opens a browser, including go trace, go tool pprof, etc.

We don't check the exit code or wait for the command because some browser commands block.

I don't want to log every time; that'd be ugly, and almost all invocations succeed.

Let's start by fixing the specific problem on the specific system, even if it is not very portable. Sounds like maybe checking DISPLAY might work?

@ronin13
Copy link

ronin13 commented Feb 19, 2017

In the last case, one can do like:

 diff --git a/src/cmd/go/internal/bug/bug.go b/src/cmd/go/internal/bug/bug.go
 index 963da94c49..214108d945 100644
 --- a/src/cmd/go/internal/bug/bug.go
 +++ b/src/cmd/go/internal/bug/bug.go
 @@ -60,7 +60,7 @@ func runBug(cmd *base.Command, args []string) {

         body := buf.String()
         url := "https://github.com/golang/go/issues/new?body=" + web.QueryEscape(body)
 -       if !web.OpenBrowser(url) {
 +       if len(os.Getenv("DISPLAY")) == 0 || !web.OpenBrowser(url) {
                 fmt.Print("Please file a new issue at golang.org/issue/new using this template:\n\n")
                 fmt.Print(body)
         }

This should work on a headless systems with Xorg (which covers everything except windows).

Tested with:

DISPLAY= $HOME/bin/go bug     # Prints to terminal

and

echo $DISPLAY
:0
$HOME/bin/go bug       # Opens browser.

@bradfitz
Copy link
Contributor

DISPLAY won't work on OS X, though. (at least, I assume our target audience is not XQuartz users) Will it even work on Wayland? Or do you also have to check WAYLAND_DISPLAY?

@ronin13
Copy link

ronin13 commented Feb 19, 2017

For Wayland, yes, $WAYLAND_DISPLAY needs to be checked. Does the X server in OSX export any similar variable?

@bradfitz
Copy link
Contributor

macOS doesn't use an X server.

@josharian
Copy link
Contributor

Could we only use display if there is a DISPLAY envvar? Or hard code some OSes?

And this code belongs in the browser-handling code, not go bug.

@ronin13
Copy link

ronin13 commented Feb 19, 2017

Regarding browser.go, strictly speaking, there are browsers which work without X server, will be harder to impose with that constraint there.

But, again, if we restrict to graphical browsers only, then one can fail early in https://github.com/golang/go/blob/master/src/cmd/internal/browser/browser.go#L20 depending on the platform (so fail only for known platforms in the beginning).

@broady
Copy link
Contributor Author

broady commented Feb 21, 2017

More details on my setup:

$ echo $BROWSER,$DISPLAY
,
$ which xdg-open
/usr/bin/xdg-open

Added some logs and can confirm xdg-open is the command being used. If I run xdg-open in a terminal window, it opens up lynx.

Suggested fix: wait 5 seconds for the process to exit. If the exit code is nonzero or takes >5 seconds to return, print out the URL to stdout.

That is, keep the current behavior if the command returns a nonzero exit code within 5 seconds.

@dmitshur
Copy link
Contributor

I've seen at least one reported case where where "xdg-open" seemed to block and not return until browser was closed. To help the issue, I had to run it in a background goroutine. See shurcooL/Go-Package-Store@a15fc56 and shurcooL/Go-Package-Store#26 (comment).

However, this was 2 years ago, and it looked like an outdated, possibly misconfigured linux system.

@josharian
Copy link
Contributor

Suggested fix: wait 5 seconds for the process to exit. If the exit code is nonzero or takes >5 seconds to return, print out the URL to stdout.

I think waiting 5s is reasonable. On most systems, the command exits almost immediately, so I doubt anyone will notice.

But we shouldn't print the URL to stdout. As I keep saying, the fix should go in the browser-opening code, not in 'go bug', and each individual command can decide how to handle the browser not opening; 'go bug' already has handling for that case.

@broady
Copy link
Contributor Author

broady commented Feb 22, 2017

Well, xdg-open does also complete within 5 seconds with a non-zero exit code.

I hooked up stdin/stdout and get the following:

stderr:

/usr/bin/xdg-open: line 461: links2: command not found

stdout:

<snip a bunch of ANSI codes>
Looking up host
<snip a bunch of ANSI codes>

Presumably it's opening up links (I don't have links2 installed) but exiting right away.

I guess xdg-open is at fault here? It's opening up links without a tty.

edit: man xdg-open says:
"xdg-open is for use inside a desktop session only."

So, I suppose we need to handle this. Do we need to inspect anything other than DISPLAY?

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/37390 mentions this issue.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/37391 mentions this issue.

gopherbot pushed a commit that referenced this issue Feb 23, 2017
xdg-open's man page says:
> xdg-open is for use inside a desktop session only.

Use the DISPLAY environment variable to detect this.

Updates #19131.

Change-Id: I3926b3e1042393939b2ec6aacd9b63ac8192df3b
Reviewed-on: https://go-review.googlesource.com/37390
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
gopherbot pushed a commit that referenced this issue Feb 24, 2017
Wait a short period between trying commands. Many commands
will return a non-zero exit code if the browser couldn't be launched.

For example, google-chrome returns quickly with a non-zero
exit code in a headless environment.

Updates #19131.

Change-Id: I0ae5356dd4447969d9e216615449cead7a8fd5c9
Reviewed-on: https://go-review.googlesource.com/37391
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
@broady broady closed this as completed Feb 24, 2017
@broady broady modified the milestones: Go1.9Early, Go1.9 Feb 24, 2017
@golang golang locked and limited conversation to collaborators Feb 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants