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

Jump to bottom

cmd/go: some errors not reported with go list #59186

Open
cherrymui opened this issue Mar 22, 2023 · 9 comments
Open

cmd/go: some errors not reported with go list #59186

cherrymui opened this issue Mar 22, 2023 · 9 comments
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@cherrymui
Copy link
Member

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

tip (go1.21-4d9beb2052)

Does this issue reproduce with the latest release?

Yes

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

darwin/amd64

What did you do?

Having a module with two packages a and b. b is a main package. a imports b (erroneously).

go.mod

module m

go 1.21

a/a.go

package a

import _ "m/b"

b/b.go

package main

func main() {}

Run go list ./a ./b.

What did you expect to see?

Report the error for importing a non-importable package, like

a/a.go:3:8: import "m/b" is a program, not an importable package
m/a
m/b

What did you see instead?

No error, with 0 exit code.

$ go list ./a ./b
m/a
m/b
$ echo $?
0

Interestingly, go list -test ./a ./b reports the error, although the error has nothing to do with test files (there is none). The documentation doesn't seem to mention that the -test flag can cause different behavior for error reporting.

There are also some other errors not being reported with go list, but reported with go list -test. https://cs.opensource.google/go/go/+/master:src/cmd/go/testdata/script/vendor_import.txt is one example. bad.go and invalid.go contains some errors, but the go list command succeeded.

It looks like the different behavior is related to that this code https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/list/list.go;l=736-788 runs when -test is specified.

Found while working on CL https://golang.org/cl/474236.

cc @bcmills
@cherrymui cherrymui added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels Mar 22, 2023
@gopherbot
Copy link

Change https://go.dev/cl/477838 mentions this issue: cmd/go: permit "go list" error in some tests

gopherbot pushed a commit that referenced this issue Mar 22, 2023
@cherrymui
cmd/go: permit "go list" error in some tests
3d28e0e 
The vendor_import test lists packages that are known bad (e.g.
bad.go, invalid.go). Pass -e to permit error.

The mod_vendor_auto test includes a package that imports a main
package, which should be an error. Pass -e to permit error.

Updates #59186.

Change-Id: I3b63025c3935f55feda1a95151d4c688d0394644
Reviewed-on: https://go-review.googlesource.com/c/go/+/477838
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
@bcmills bcmills added this to the Backlog milestone Mar 22, 2023
@cherrymui
Copy link
Member Author

cherrymui commented Mar 23, 2023 •
edited

After CL https://golang.org/cl/474236, when listing multiple packages, the default -pgo=auto mode will cause this code https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/list/list.go;l=736-788 to run, which will cause it to report the error. go list -pgo=off ./a ./b will get back to the previous (Go 1.20) unexpected behavior.

@seankhliao
Copy link
Member

I thought blank imports of main packages were allowed as a way to track dependencies, eg when using the tools.go pattern.

@bcmills
Copy link
Contributor

bcmills commented Apr 3, 2023

@seankhliao, go mod tidy allows them, but in general we expect go list to follow go build in terms of what errors it reports, and go build certainly will not let you import a package main explicitly.

@thockin
Copy link

thockin commented May 27, 2023

I hit this through another vector, but similar. It seems very strange to assert that I can't list, when I am allowed to set up this construct in the first place. This is a breaking change - is it really worth it?

@bcmills
Copy link
Contributor

bcmills commented Jun 9, 2023

@thockin, given that we don't understand why this ever worked differently in the first place, we can't easily restore that buggy behavior.

go list supports the -e flag to allow listing packages that may have errors. If the -e flag does not allow go list to proceed past these errors, please file a separate issue. (go list -e has had many bugs, but we have been slowly fixing those and hopefully we can keep moving it toward a better long-term state.)

This was referenced Jul 3, 2023
Merged
Merged
@MadhavJivrajani
Copy link

MadhavJivrajani commented Jul 3, 2023 •
edited

We hit this in kubernetes as well while trying to test with go1.21rc2: kubernetes/kubernetes#118996
The -e workaround seems to work (kubernetes/kubernetes#119027), but I'm not sure adopting that is prudent in the long run.

@bcmills bcmills mentioned this issue Aug 8, 2023
Closed
This was referenced Aug 18, 2023
Merged
Merged
Merged
Merged
@jpadilla jpadilla mentioned this issue Aug 21, 2023
Merged
4 tasks
@alexbozhenko
Copy link
Contributor

@bcmills:
Hi.
Other issue that I reported #62228 was closed as duplicated.
But there I provide the reproducer, and I think it is caused by this commit: a5c7928

Or maybe that actually a separate issue, not a duplicate of this one?

@bcmills
Copy link
Contributor

bcmills commented Aug 23, 2023

@alexbozhenko, thanks for flagging that commit. That's an interesting clue, although it still doesn't really explain why the PGO changes seemed to trigger this behavior. 😅

But I think that will help us investigate.

@UTsweetyfish UTsweetyfish mentioned this issue Jan 16, 2024
Merged
@leandro-alt leandro-alt mentioned this issue Mar 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@bcmills @alexbozhenko @thockin @gopherbot @seankhliao @MadhavJivrajani @cherrymui