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

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

Projects

None yet

7 participants

@broady
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
Member

Let's bug @josharian.

@josharian josharian was assigned by bradfitz Feb 16, 2017
@bradfitz bradfitz added the HelpWanted label Feb 16, 2017
@rakyll
Member
rakyll commented Feb 16, 2017

/cc @shurcooL

@shurcooL
Member
$ go bug
$

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

@broady
Contributor
broady commented Feb 16, 2017
@josharian
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
Member
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
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
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
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
Member

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
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
Member

macOS doesn't use an X server.

@josharian
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
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
Contributor
broady commented Feb 21, 2017 edited

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.

@shurcooL
Member

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
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
Contributor
broady commented Feb 22, 2017 edited

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

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

@gopherbot

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

@gopherbot gopherbot pushed a commit that referenced this issue Feb 23, 2017
@broady broady cmd/internal/browser: use xdg-open only from a desktop session
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>
d580972
@gopherbot gopherbot pushed a commit that referenced this issue Feb 24, 2017
@broady broady cmd/internal/browser: wait 3 seconds for non-zero exit codes
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>
2818cb5
@broady broady closed this Feb 24, 2017
@broady broady removed the HelpWanted label Feb 24, 2017
@broady broady modified the milestone: Go1.9Early, Go1.9 Feb 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment