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

x/mobile/cmd/gobind: TestGobind/.*/Go-Cgopkg broken at CL 437298 #56292

Closed
bcmills opened this issue Oct 18, 2022 · 10 comments
Closed

x/mobile/cmd/gobind: TestGobind/.*/Go-Cgopkg broken at CL 437298 #56292

bcmills opened this issue Oct 18, 2022 · 10 comments
Labels
FrozenDueToAge mobile Android, iOS, and x/mobile
Milestone
Unreleased

Comments

@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2022

https://build.golang.org/log/c656c3d100391699f5e7a0389263d3a2be0a6497:

--- FAIL: TestGobind (10.23s)
    --- FAIL: TestGobind/GOPATH (5.18s)
        --- FAIL: TestGobind/GOPATH/Go-Cgopkg (1.59s)
            gobind_test.go:132: GO111MODULE=off gobind -lang go,java,objc golang.org/x/mobile/bind/testdata/cgopkg failed: exit status 1: // Code generated by gobind. DO NOT EDIT.
…
    --- FAIL: TestGobind/Modules (5.05s)
        --- FAIL: TestGobind/Modules/Go-Cgopkg (1.58s)
            gobind_test.go:132: GO111MODULE=on gobind -lang go,java,objc golang.org/x/mobile/bind/testdata/cgopkg failed: exit status 1: // Code generated by gobind. DO NOT EDIT.
…
FAIL
FAIL	golang.org/x/mobile/cmd/gobind	11.353s

The first failure on the dashboard was at https://go.dev/cl/437298, although the x repos don't always test every Go commit.

Unfortunately, since these tests don't produce useful failure messages, it's hard for me to tell why they're even failing.

(attn @hyangah per https://dev.golang.org/owners; CC @hajimehoshi @golang/android @golang/ios)
@gopherbot gopherbot added the mobile Android, iOS, and x/mobile label Oct 18, 2022
@gopherbot gopherbot added this to the Unreleased milestone Oct 18, 2022
@bcmills
Copy link
Contributor Author

bcmills commented Oct 18, 2022

(I do see an exit status 1 in the failure message, but I don't see anything in the output of the command that would explain why it exited with nonzero status.)

@hajimehoshi
Copy link
Member

My previous change might cause this issue. I'll take a look

@gopherbot
Copy link

Change https://go.dev/cl/443655 mentions this issue: cmd/gobind: exec the test binary as gobind and log only stderr by default

@bcmills
Copy link
Contributor Author

bcmills commented Oct 18, 2022

I cleaned up the test output in https://go.dev/cl/443655, and got the error output down to something more comprehensible:

~/x/mobile$ go1.19.2 test ./cmd/gobind -run=TestGobind/Modules/Go-Cgopkg
ok      golang.org/x/mobile/cmd/gobind  1.621s

~/x/mobile$ gotip test ./cmd/gobind -run=TestGobind/Modules/Go-Cgopkg
--- FAIL: TestGobind (1.61s)
    --- FAIL: TestGobind/Modules (1.61s)
        --- FAIL: TestGobind/Modules/Go-Cgopkg (1.54s)
            gobind_test.go:115: stderr (/tmp/go-build4070383437/b001/gobind.test -lang go,java,objc golang.org/x/mobile/bind/testdata/cgopkg):
                unable to import bind/java: [-: go build golang.org/x/mobile/bind/java: # golang.org/x/mobile/bind/java
                bind/java/context_android.go:9:10: fatal error: jni.h: No such file or directory
                    9 | //#include <jni.h>
                      |          ^~~~~~~
                compilation terminated.]
            gobind_test.go:143: /tmp/go-build4070383437/b001/gobind.test -lang go,java,objc golang.org/x/mobile/bind/testdata/cgopkg: exit status 1
FAIL
FAIL    golang.org/x/mobile/cmd/gobind  1.642s

@hajimehoshi
Copy link
Member

Hm, I couldn't reproduce the issue on my local machine...

$ go test ./cmd/gobind -run=TestGobind/Modules/Go-Cgopkg
ok      golang.org/x/mobile/cmd/gobind  3.799s

@bcmills
Copy link
Contributor Author

bcmills commented Oct 18, 2022 •
edited

git bisect on my workstation confirms that this test started failing at CL 437298 exactly:

01604129aee8bfc9dd3e2fffd2ad8f772a3089ec is the first bad commit
commit 01604129aee8bfc9dd3e2fffd2ad8f772a3089ec
Author: Michael Matloob <matloob@golang.org>
Date:   Fri Sep 30 16:49:42 2022 -0400

    cmd/go: do not exit with non-zero code from go 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.

    For #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>

 src/cmd/go/internal/list/list.go             |  3 +
 src/cmd/go/internal/work/action.go           |  1 +
 src/cmd/go/internal/work/exec.go             | 95 +++++++++++++++++-----------
 src/cmd/go/internal/work/gc.go               |  4 +-
 src/cmd/go/testdata/script/list_export_e.txt | 19 ++++++
 5 files changed, 84 insertions(+), 38 deletions(-)
 create mode 100644 src/cmd/go/testdata/script/list_export_e.txt

(CC @matloob)

@bcmills bcmills changed the title x/mobile/cmd/gobind: TestGobind/.*/Go-Cgopkg broken at or before CL 437298 x/mobile/cmd/gobind: TestGobind/.*/Go-Cgopkg broken at CL 437298 Oct 18, 2022
@gopherbot
Copy link

Change https://go.dev/cl/443656 mentions this issue: go.mod: update dependency on x/tools/go/packages

gopherbot pushed a commit to golang/mobile that referenced this issue Oct 18, 2022
@gopherbot
cmd/gobind: exec the test binary as gobind and log only stderr by def…
3eb8272 
…ault

Using the test binary as the "gobind" command not only avoids the
overhead of recompiling the command, but also allows commands like "go
test -race" to actually test the requested configuration of the
command.

Logging stderr and stdout separately — and logging only stderr by
default — makes the failure messages much easier to spot. (Logging the
combined output as before produced a massive wall of text that tends
to bury the actual errors.)

For golang/go#56292.

Change-Id: Ia11fad19418d9b9004608c76fe512ceab4f247bc
Reviewed-on: https://go-review.googlesource.com/c/mobile/+/443655
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Hajime Hoshi <hajimehoshi@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: David Chase <drchase@google.com>
gopherbot pushed a commit to golang/mobile that referenced this issue Oct 18, 2022
@gopherbot
go.mod: update dependency on x/tools/go/packages
51f526d 
Also update to "go 1.17" to enable module graph pruning.

For golang/go#56292.

Change-Id: I1d1f4a2625d36d4c8587080e391d3e2c2e7f7808
Reviewed-on: https://go-review.googlesource.com/c/mobile/+/443656
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hajime Hoshi <hajimehoshi@gmail.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: David Chase <drchase@google.com>
@hyangah
Copy link
Contributor

hyangah commented Oct 19, 2022

@hajimehoshi This is reproducible only with the go built on tip in the path (after cl/437298)

$ go install golang.org/dl/gotip@latest
$ gotip download
$ gotip install golang.org/x/mobile/cmd/gobind@latest
$ export PATH=$(gotip env GOROOT)/bin:$PATH
$ GOOS=android CGO_ENABLED=1 gobind -lang=go,java golang.org/x/mobile/bind/testdata/cgopkg

With cl/437298 (that is a bug fix IMO), the error is reported in the Error field and eventually in go/packages.Package's Errors.

$ CGO_ENABLED=1 GOOS=android gotip list -e -json=Name,ImportPath,Error -compiled=true -test=false -export=false -deps=false -find=true golang.org/x/mobile/bind/java
{
        "ImportPath": "golang.org/x/mobile/bind/java",
        "Name": "java",
        "Error": {
                "ImportStack": null,
                "Pos": "",
                "Err": "go build golang.org/x/mobile/bind/java: # golang.org/x/mobile/bind/java\n../../bind/java/context_android.go:9:10: fatal error: 'jni.h' file not found\n#include \u003cjni.h\u003e\n         ^~~~~~~\n1 error generated.\n"
        }
}

As a result, the gobind command checks the Errors field and fails.
https://github.com/golang/mobile/blob/51f526d719ab0aa30e2c27a3c1701819189ae1de/cmd/gobind/gen.go#L384

Before cl/437298, go list -e failed with an error (*exec.ExitError)

$ CGO_ENABLED=1 GOOS=android go1.19.1 list -e -json=Name,ImportPath,Error -compiled=true -test=false -export=false -deps=false -find=true golang.org/x/mobile/bind/java
# golang.org/x/mobile/bind/java
../../bind/java/context_android.go:9:10: fatal error: 'jni.h' file not found
#include <jni.h>
         ^~~~~~~
1 error generated.
{
        "ImportPath": "golang.org/x/mobile/bind/java",
        "Name": "java"
}

This failure error is reported as a friendlyErr of gocmdRunner.RunRaw call in https://github.com/golang/tools/blob/bc2e3aeaba30b86fc421aefdfc78094b43700314/go/packages/golist.go#L950.

But it looks like go/packages dropped this error on the floor... Interestingly, the comment here says:

  // Is there an error running the C compiler in cgo? This will be reported in the "Error" field
  // and should be suppressed by go list -e.

There are some heuristic scanning of stderr but this failure mode is not captured. As a result, package load succeeds and the returned Package carries no error.

The previous bug depended on the bug in go list. It's unclear to me whether gobind has to handle this error.
It looks to me that gobind attempts to load the package just to find out the package directory. Then, we don't NeedCompiledGoFiles bit.

@gopherbot
Copy link

Change https://go.dev/cl/443935 mentions this issue: cmd/gobind: do not compile package just to find package dir

gopherbot pushed a commit to golang/mobile that referenced this issue Oct 19, 2022
@hyangah @hajimehoshi
cmd/gobind: do not compile package just to find package dir
406ed3a 
Gobind utilizes golang.org/x/tools/go/packages.Load to find
the directory of a package. Configure the load configuration
to just find the list of files. Zero load mode is equivalent
to combining NeedName+NeedFiles+NeedCompiledGoFiles bits.
That is unnecessary, and can increase the chance of load
failures. For example, load with the zero load mode may fail
if all the necessary cgo dependencies aren't available in the
system, but that shouldn't be critical for gobind's use case.

Updates golang/go#56292

Change-Id: Ifaf4f43e9053cf4a43fd657a9a394fc13f611576
Reviewed-on: https://go-review.googlesource.com/c/mobile/+/443935
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Hajime Hoshi <hajimehoshi@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
@hyangah
Copy link
Contributor

hyangah commented Oct 19, 2022

Verified long builder passed with this commit.

@hyangah hyangah closed this as completed Oct 19, 2022
@chenxiaolong chenxiaolong mentioned this issue May 22, 2023
Open
@golang golang locked and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge mobile Android, iOS, and x/mobile
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@hajimehoshi @hyangah @bcmills @gopherbot