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

bcmills · 2022-10-18T12:57:24Z

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)

The text was updated successfully, but these errors were encountered:

bcmills · 2022-10-18T13:05:11Z

(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 · 2022-10-18T13:09:03Z

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

gopherbot · 2022-10-18T13:28:27Z

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

bcmills · 2022-10-18T13:30:25Z

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 · 2022-10-18T14:08:34Z

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 · 2022-10-18T14:17:17Z

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)

gopherbot · 2022-10-18T14:24:57Z

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

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

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 · 2022-10-19T05:30:32Z

@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 · 2022-10-19T05:31:56Z

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

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 · 2022-10-19T15:40:27Z

Verified long builder passed with this commit.

gopherbot added the mobile Android, iOS, and x/mobile label Oct 18, 2022

gopherbot added this to the Unreleased milestone Oct 18, 2022

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

hyangah closed this as completed Oct 19, 2022

chenxiaolong mentioned this issue May 22, 2023

x/mobile/bind/java: fatal error: jni.h: No such file or directory #58426

Open

golang locked and limited conversation to collaborators Oct 19, 2023

gopherbot added the FrozenDueToAge label Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bcmills commented Oct 18, 2022

bcmills commented Oct 18, 2022

hajimehoshi commented Oct 18, 2022

gopherbot commented Oct 18, 2022

bcmills commented Oct 18, 2022

hajimehoshi commented Oct 18, 2022

bcmills commented Oct 18, 2022 •

edited

Loading

gopherbot commented Oct 18, 2022

hyangah commented Oct 19, 2022

gopherbot commented Oct 19, 2022

hyangah commented Oct 19, 2022

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

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

Comments

bcmills commented Oct 18, 2022

bcmills commented Oct 18, 2022

hajimehoshi commented Oct 18, 2022

gopherbot commented Oct 18, 2022

bcmills commented Oct 18, 2022

hajimehoshi commented Oct 18, 2022

bcmills commented Oct 18, 2022 • edited Loading

gopherbot commented Oct 18, 2022

hyangah commented Oct 19, 2022

gopherbot commented Oct 19, 2022

hyangah commented Oct 19, 2022

bcmills commented Oct 18, 2022 •

edited

Loading