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.

Sign up for GitHub

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, x/tools: many tests are broken with go1.19.10 #60650

Closed
findleyr opened this issue Jun 7, 2023 · 13 comments
Closed

cmd/go, x/tools: many tests are broken with go1.19.10 #60650

findleyr opened this issue Jun 7, 2023 · 13 comments
Assignees
@prattmic @bcmills
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.19.11

Comments

@findleyr
Copy link
Contributor

findleyr commented Jun 7, 2023

Many tests, for example x/tools/go/packages, are broken with Go 1.19.10.

Here's an example trybot failure:
https://storage.googleapis.com/go-build-log/3ba9c890/linux-amd64_b61d03c6.log

And here's the failure for go/packages, which may be illustrative:

> go1.19.10 test ./go/packages
--- FAIL: TestMultiplePackageVersionsIssue36188 (0.00s)
    --- FAIL: TestMultiplePackageVersionsIssue36188/GOPATH (0.13s)
        packages_test.go:2199: err: exit status 1: stderr: go build golang.org/fake/b: golang.org/fake/a/a.go:1:19: import "golang.org/fake/b" is a program, not an importable package
            
    --- FAIL: TestMultiplePackageVersionsIssue36188/Modules (0.12s)
        packages_test.go:2199: err: exit status 1: stderr: go build golang.org/fake/b: a/a.go:1:19: import "golang.org/fake/b" is a program, not an importable package
            
--- FAIL: TestReturnErrorForUnexpectedDirectoryLayout (0.00s)
    --- FAIL: TestReturnErrorForUnexpectedDirectoryLayout/Modules (0.10s)
        packages_test.go:1716: want error message: unexpected directory layout, got: err: exit status 1: stderr: go build b: a.go:1:19: package b is not in GOROOT (/usr/local/google/home/rfindley/sdk/go1.19.10/src/b)
--- FAIL: TestInvalidPackageName (0.00s)
    --- FAIL: TestInvalidPackageName/GOPATH (0.06s)
        packages_test.go:2735: err: exit status 1: stderr: go build golang.org/fake: golang.org/fake/main.go:1:9: expected 'IDENT', found 'default'
            
    --- FAIL: TestInvalidPackageName/Modules (0.14s)
        packages_test.go:2735: err: exit status 1: stderr: go build golang.org/fake: main.go:1:9: expected 'IDENT', found 'default'
            
--- FAIL: TestMissingDependency (0.00s)
    --- FAIL: TestMissingDependency/GOPATH (0.10s)
        packages_test.go:1732: err: exit status 1: stderr: go build this/package/doesnt/exist: golang.org/fake/a/a.go:1:19: cannot find package "this/package/doesnt/exist" in any of:
            	/usr/local/google/home/rfindley/sdk/go1.19.10/src/this/package/doesnt/exist (from $GOROOT)
            	/tmp/TestMissingDependency_GOPATH2093183701/fake/src/this/package/doesnt/exist (from $GOPATH)
            
    --- FAIL: TestMissingDependency/Modules (0.19s)
        packages_test.go:1732: err: exit status 1: stderr: go build this/package/doesnt/exist: a/a.go:1:19: package this/package/doesnt/exist is not in GOROOT (/usr/local/google/home/rfindley/sdk/go1.19.10/src/this/package/doesnt/exist)
            
--- FAIL: TestInvalidFilesBeforeOverlay (0.00s)
    --- FAIL: TestInvalidFilesBeforeOverlay/GOPATH (0.06s)
        --- FAIL: TestInvalidFilesBeforeOverlay/GOPATH/no_overlay (0.06s)
            overlay_test.go:739: err: exit status 1: stderr: go build golang.org/fake/d: golang.org/fake/d/d.go:1:1: expected 'package', found 'EOF'
                go build golang.org/fake: golang.org/fake/main.go:1:1: expected 'package', found 'EOF'
                
    --- FAIL: TestInvalidFilesBeforeOverlay/Modules (0.08s)
        --- FAIL: TestInvalidFilesBeforeOverlay/Modules/no_overlay (0.04s)
            overlay_test.go:739: err: exit status 1: stderr: go build golang.org/fake: main.go:1:1: expected 'package', found 'EOF'
                go build golang.org/fake/d: d/d.go:1:1: expected 'package', found 'EOF'
                
--- FAIL: TestInvalidFilesBeforeOverlayContains (0.00s)
    --- FAIL: TestInvalidFilesBeforeOverlayContains/Modules (3.40s)
        --- FAIL: TestInvalidFilesBeforeOverlayContains/Modules/test_variant (1.11s)
            overlay_test.go:825: expected package ID "golang.org/fake/d [golang.org/fake/d.test]", got "command-line-arguments [command-line-arguments.test]"
        --- FAIL: TestInvalidFilesBeforeOverlayContains/Modules/second_file (0.20s)
            overlay_test.go:825: expected package ID "golang.org/fake/d", got "command-line-arguments"
        --- FAIL: TestInvalidFilesBeforeOverlayContains/Modules/main (1.16s)
            overlay_test.go:825: expected package ID "golang.org/fake", got "command-line-arguments"
        --- FAIL: TestInvalidFilesBeforeOverlayContains/Modules/xtest (0.91s)
            overlay_test.go:825: expected package ID "golang.org/fake/d_test [golang.org/fake/d.test]", got "command-line-arguments_test [command-line-arguments.test]"
    --- FAIL: TestInvalidFilesBeforeOverlayContains/GOPATH (3.79s)
        --- FAIL: TestInvalidFilesBeforeOverlayContains/GOPATH/test_variant (1.29s)
            overlay_test.go:825: expected package ID "golang.org/fake/d [golang.org/fake/d.test]", got "command-line-arguments [command-line-arguments.test]"
        --- FAIL: TestInvalidFilesBeforeOverlayContains/GOPATH/second_file (0.24s)
            overlay_test.go:825: expected package ID "golang.org/fake/d", got "command-line-arguments"
        --- FAIL: TestInvalidFilesBeforeOverlayContains/GOPATH/main (1.26s)
            overlay_test.go:825: expected package ID "golang.org/fake", got "command-line-arguments"
        --- FAIL: TestInvalidFilesBeforeOverlayContains/GOPATH/xtest (1.00s)
            overlay_test.go:825: expected package ID "golang.org/fake/d_test [golang.org/fake/d.test]", got "command-line-arguments_test [command-line-arguments.test]"

Given the amount of tests that fail, it seems likely that this failure mode is user-facing.

CC @bcmills @matloob @rsc
@findleyr findleyr added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Soon This needs to be done soon. (regressions, serious bugs, outages) labels Jun 7, 2023
@bcmills
Copy link
Contributor

bcmills commented Jun 7, 2023

At least this should be pretty east to bisect. 😅

@bcmills
Copy link
Contributor

bcmills commented Jun 7, 2023

(Bisecting now.)

@bcmills
Copy link
Contributor

bcmills commented Jun 7, 2023 

c160b49b6d328c86bd76ca2fff9009a71347333f is the first bad commit
commit c160b49b6d328c86bd76ca2fff9009a71347333f
Author: Bryan C. Mills <bcmills@google.com>
Date:   Fri May 12 14:15:16 2023 -0400

    [release-branch.go1.19] cmd/go: disallow package directories containing newlines

    Directory or file paths containing newlines may cause tools (such as
    cmd/cgo) that emit "//line" or "#line" -directives to write part of
    the path into non-comment lines in generated source code. If those
    lines contain valid Go code, it may be injected into the resulting
    binary.

    (Note that Go import paths and file paths within module zip files
    already could not contain newlines.)

    Thanks to Juho Nurminen of Mattermost for reporting this issue.

    Updates #60167.
    Fixes #60515.
    Fixes CVE-2023-29402.

    Change-Id: If55d0400c02beb7a5da5eceac60f1abeac99f064
    Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1882606
    Reviewed-by: Roland Shoemaker <bracewell@google.com>
    Run-TryBot: Roland Shoemaker <bracewell@google.com>
    Reviewed-by: Russ Cox <rsc@google.com>
    Reviewed-by: Damien Neil <dneil@google.com>
    (cherry picked from commit 41f9046495564fc728d6f98384ab7276450ac7e2)
    Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1902229
    Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1904343
    Reviewed-by: Michael Knyszek <mknyszek@google.com>
    Reviewed-by: Bryan Mills <bcmills@google.com>
    Reviewed-on: https://go-review.googlesource.com/c/go/+/501218
    Run-TryBot: David Chase <drchase@google.com>
    Auto-Submit: Michael Knyszek <mknyszek@google.com>
    TryBot-Result: Gopher Robot <gobot@golang.org>

 src/cmd/go/internal/load/pkg.go                  |   4 +
 src/cmd/go/internal/work/exec.go                 |   6 ++
 src/cmd/go/script_test.go                        |   1 +
 src/cmd/go/testdata/script/build_cwd_newline.txt | 100 +++++++++++++++++++++++
 4 files changed, 111 insertions(+)
 create mode 100644 src/cmd/go/testdata/script/build_cwd_newline.txt

Well then. Time to figure out what old bug we're tickling on the release branch. 😩

@bcmills bcmills self-assigned this Jun 7, 2023
@bcmills bcmills added the GoCommand cmd/go label Jun 7, 2023
@bcmills
Copy link
Contributor

bcmills commented Jun 7, 2023

I suspect that the go/packages failures are due to stricter error-reporting in cmd/go/internal/work/exec.go:

	if p.Error != nil {
		// Don't try to build anything for packages with errors. There may be a
		// problem with the inputs that makes the package unsafe to build.
		return p.Error
	}

But I don't know why that would be specific to 1.19. (My intuition is that it may be related to #29666, but we started using more precise -json flags at 1.19!)

@bcmills
Copy link
Contributor

bcmills commented Jun 8, 2023

This may also be related to the bug fixed in https://go.dev/cl/437298. I'll see if that backports cleanly, and if so whether it fixes the problem.

@bcmills
Copy link
Contributor

bcmills commented Jun 8, 2023

I got it backported (with a couple of minor merge conflicts), but it doesn't seem to fix x/tools. 😞

@gopherbot
Copy link

Change https://go.dev/cl/502175 mentions this issue: go/packages/packagestest: set Config.Logf if the test is run verbosely

@bcmills bcmills mentioned this issue Jun 9, 2023
Open
@gopherbot gopherbot mentioned this issue Jun 9, 2023
Closed
@bcmills bcmills added this to the Go1.19.11 milestone Jun 9, 2023
@gopherbot
Copy link

Change https://go.dev/cl/502195 mentions this issue: [release-branch.go1.19] cmd/go: do not exit with non-zero code from go list -e -export

@gopherbot
Copy link

Change https://go.dev/cl/502196 mentions this issue: [release-branch.go1.19] cmd/go/internal/work: make formatOutput return an error that includes the import path

@bcmills
Copy link
Contributor

bcmills commented Jun 9, 2023

I have identified two backport CLs from #25842 that together appear to fix all of the broken x/tools tests.

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 9, 2023
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 9, 2023
@findleyr
Copy link
Contributor Author

findleyr commented Jun 9, 2023

Great, thank you, Bryan!

gopherbot pushed a commit to golang/tools that referenced this issue Jun 10, 2023
@gopherbot
go/packages/packagestest: set Config.Logf if the test is run verbosely
db6a81e 
The Config.Logf field enables debug logging in go/packages, which is
exactly what we want when running a test in verbose mode to debug it.

For golang/go#60650.

Change-Id: I36b47e214860b5aec7c66042fc0ceb50c7062f1a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/502175
Reviewed-by: Michael Matloob <matloob@golang.org>
TryBot-Bypass: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
@bcmills
Copy link
Contributor

bcmills commented Jun 13, 2023

At this point this is just waiting on the backport CLs to be merged by the release team. (I think @prattmic is on interrupts this week, but I'm not sure when in the process the release team usually does those merges.)

@bcmills bcmills removed the Soon This needs to be done soon. (regressions, serious bugs, outages) label Jun 13, 2023
@gopherbot
Copy link

Closed by merging c045822 to release-branch.go1.19.

gopherbot pushed a commit that referenced this issue Jun 13, 2023
@matloob @prattmic
[release-branch.go1.19] cmd/go: do not exit with non-zero code from g…
c045822 
…o list -e -export

go list -e -export puts errors running build actions on the load.Package
corresponding to the failed action rather than exiting with a non zero
exit code.

Fixes #60710.
Fixes #60650.
Updates #25842.

Change-Id: I1fea85cc5a0557f514fe9d4ed3b6a858376fdcde
Reviewed-on: https://go-review.googlesource.com/c/go/+/437298
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/502195
TryBot-Bypass: Bryan Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue Jun 13, 2023
@prattmic
[release-branch.go1.19] cmd/go/internal/work: make formatOutput retur…
7d6517c 
…n an error that includes the import path

This refines the error output that was previously adjusted in CL 437298.

Longer term, we should consider unraveling the call chains involving
formatOutput to avoid passing so many parameters through so many
different formatting functions.

Updates #60710.
Updates #60650.
Updates #25842.

Change-Id: I3b9d03bf5968902d8ccc4841ab4dbe114a2239e4
Reviewed-on: https://go-review.googlesource.com/c/go/+/451218
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/502196
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Bypass: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@prattmic prattmic

@bcmills bcmills

Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.19.11
Development

No branches or pull requests
4 participants
@prattmic @bcmills @gopherbot @findleyr