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/go: go list -export -e outputs errors to stderr and has non-zero exit code #25842

Open
myitcv opened this Issue Jun 12, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@myitcv
Member

myitcv commented Jun 12, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version devel +6b3e343408 Tue Jun 12 03:42:37 2018 +0000 linux/amd64

Does this issue reproduce with the latest release?

No; this is only available in tip.

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/tmp/tmp.4mkm9vJCVB"
GORACE=""
GOROOT="/home/myitcv/gos"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build091981204=/tmp/go-build -gno-record-gcc-switches"

What did you do?

cd `mktemp -d`
export GOPATH=$PWD
mkdir -p src/example.com
cd src/example.com
mkdir p1
cat <<EOD > p1/p1.go
package p1

const Name = "p1"
EOD
mkdir p2
cat <<EOD > p2/main.go
package main

import "fmt"
import "example.com/p1"

func main() {
        fmt.Println(p1.Name == 5)
}
EOD

go list behaves as expected (there are no syntax errors but there is a compile error):

$ go list ./...
example.com/p1
example.com/p2

But if we add the -export and -e flags we see:

$ go list -e -export ./...
p2/main.go:7:29: cannot convert p1.Name (type untyped string) to type int
p2/main.go:7:29: invalid operation: p1.Name == 5 (mismatched types string and int)
example.com/p1
example.com/p2
$ echo $?
2

I wonder whether my expectations about -e are correct in the context of -export?

The -e flag changes the handling of erroneous packages, those that cannot be found or are malformed.

What did you expect to see?

$ go list -e -export ./...
example.com/p1
example.com/p2
$ echo $?
0

What did you see instead?

Per the repro steps:

$ go list -e -export ./...
p2/main.go:7:29: cannot convert p1.Name (type untyped string) to type int
p2/main.go:7:29: invalid operation: p1.Name == 5 (mismatched types string and int)
example.com/p1
example.com/p2
$ echo $?
2

/cc @rsc

@bcmills

This comment has been minimized.

Member

bcmills commented Jun 12, 2018

It seems to me that go list -e -export should return zero only if it successfully wrote up-to-date export data. It would be reasonable for go list -e -export to return zero if there is an error in the package that does not affect its export data, if such a thing is possible.

That does suggest that we should refine the documentation, though. It's not quite accurate to say that “[w]ith the -e flag, the list command never prints errors to standard error”.

@myitcv

This comment has been minimized.

Member

myitcv commented Jun 12, 2018

It seems to me that go list -e -export should return zero only if it successfully wrote up-to-date export data.

To make it concrete, the issue I see with this is that as the caller of go list -e -export (and I'm guessing go/packages would also be such a caller?) I can't distinguish between:

  1. go list failing for some truly fatal reason
  2. go list failing because one or more of the packages I asked it to list -export (and I would typically do this with -json) failed to compile

We could use some sort of semantic that says "if there is any output on stdout then use it as an indicator of success, compile errors will be reported to stderr", but this seems somewhat brittle.

@bcmills do you see another way of me distinguishing between cases 1 and 2 above?

Which is why I felt the semantics of -e worked quite well for -export, with output to stderr being reserved for situations where there are indeed errors that will result in a non-zero exit code (or I guess where a -verbose-like flag is applied):

$ go list -json asdf
can't load package: package asdf: cannot find package "asdf" in any of:
        /home/myitcv/gos/src/asdf (from $GOROOT)
        /home/myitcv/.mountpoints/gopherize.me/src/myitcv.io/gopherize.me/_vendor/src/asdf (from $GOPATH)
        /home/myitcv/.mountpoints/gopherize.me/src/asdf
$ echo $?
1

vs:

$ go list -e -json asdf
{
        "ImportPath": "asdf",
        "Stale": true,
        "StaleReason": "build ID mismatch",
        "Incomplete": true,
        "Error": {
                "ImportStack": [
                        "asdf"
                ],
                "Pos": "",
                "Err": "cannot find package \"asdf\" in any of:\n\t/home/myitcv/gos/src/asdf (from $GOROOT)\n\t/home/myitcv/.mountpoints/gopherize.me/src/myitcv.io/gopherize.me/_vendor/src/asdf (from $GOPATH)\n\t/home/myitcv/.mountpoints/gopherize.me/src/asdf"
        }
}
$ echo $?
0

With go list -e -export, packages that fail to compile would simply omit the .Export field, and instead contain an .Error field similar to the above (with the compile error, although this is probably not essential).

@bcmills

This comment has been minimized.

Member

bcmills commented Jun 12, 2018

Hmm, that's a good point. -export is fairly useless without either -json or -f, and with one of those flags it seems reasonable to set Incomplete instead of Export for any package that we couldn't build.

@rsc

This comment has been minimized.

Contributor

rsc commented Aug 9, 2018

May not fix for the release but I'd at least like to understand it better and talk to the go/packages team.

@rsc

This comment has been minimized.

Contributor

rsc commented Aug 17, 2018

In general go/packages must expect that the subcommand it runs ("go list" or any other helper like bazel) can exit unsuccessfully and print errors to standard output/error. That should make Load fail, returning an err including the output. The usual way to generate such an error is:

out, err := cmd.CombinedOutput()
if err != nil {
	return nil, fmt.Errorf("%s: %v\n%s", strings.Join(cmdline, " "), err, out)
}

If errors happen during the build for -export, I agree that go list -e should ideally report them in the Package structs. But go/packages must be able to handle the current failure mode regardless, even if it's not quite right for the question that was asked.

Leaving for Go 1.12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment