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

x/tools/go/packages: Load returns different results depending on build cache state #33687

Open
hajimehoshi opened this issue Aug 16, 2019 · 10 comments

Comments

@hajimehoshi
Copy link
Contributor

commented Aug 16, 2019

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

$ go version
go version go1.12.5 darwin/amd64

Does this issue reproduce with the latest release?

Maybe yes

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/hajimehoshi/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/hajimehoshi/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ht/ky_bwgzs4bd5z1hh02k34x_h0000gn/T/go-build727446393=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Clear the cache by

go clean -cache

and run the below code:

package main

import (
        "fmt"

        "golang.org/x/tools/go/packages"
)

func main() {
        cfg := &packages.Config{
                Mode: packages.NeedName | packages.NeedFiles | packages.NeedCompiledGoFiles |
                        packages.NeedImports | packages.NeedDeps |
                        packages.NeedTypes | packages.NeedSyntax | packages.NeedTypesInfo,
        }
        allPkg, err := packages.Load(cfg, "golang.org/x/mobile/bind/testdata/testpkg/javapkg")
        if err != nil {
                panic(err)
        }
        fmt.Println(allPkg)
}

What did you expect to see?

[golang.org/x/mobile/bind/testdata/testpkg/javapkg]

What did you see instead?

[Java/java/beans]

In the second time with cache, the result became expected one.

CC @hyangah

@gopherbot gopherbot added this to the Unreleased milestone Aug 16, 2019

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@hyangah

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

The "Java/..." packages are special package paths introduced to support for reverse-binding (See #16876, and also x/mobile/bind/genclasses.go files).

@eliasnaur Is it possible to generate those packages early? I hope we are not facing a chicken-and-egg problem.
@matloob Is it possible to teach package.Load to ignore some of the not-yet-existing packages?

@hyangah hyangah changed the title x/tools/go/package: Load returns weird results with a package including gomobile reverse bindings x/mobile: x/tools/go/packages.Load is unhappy about not-yet-existing packages for reverse binding Aug 16, 2019

@gopherbot gopherbot added the mobile label Aug 16, 2019

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

I think this is not a mobile-specific issue, but an issue with unknown package.

For example,

// $GOPATH/go/src/github.com/hajimehoshi/testpkg/foo.go
package foo

import (
        "Fooooooo"
)

can cause the same issue:

// main.go
package main

import (
        "fmt"

        "golang.org/x/tools/go/packages"
)

func main() {
        cfg := &packages.Config{
                Mode: packages.NeedName | packages.NeedFiles | packages.NeedCompiledGoFiles |
                        packages.NeedImports | packages.NeedDeps |
                        packages.NeedTypes | packages.NeedSyntax | packages.NeedTypesInfo,
        }
        allPkg, err := packages.Load(cfg, "github.com/hajimehoshi/testpkg")
        if err != nil {
                panic(err)
        }
        fmt.Println(allPkg)
}

and run:

go clean -cache && env GO111MODULE=off go run . && env GO111MODULE=off go run .

The result is:

[Fooooooo]
[github.com/hajimehoshi/testpkg]
@eliasnaur

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

The "Java/..." packages are special package paths introduced to support for reverse-binding (See #16876, and also x/mobile/bind/genclasses.go files).

@eliasnaur Is it possible to generate those packages early?

I'm not sure what you mean by "early". The Java/... and ObjC/... packages are generated as a first step before the actual generating of bindings.

I hope we are not facing a chicken-and-egg problem.

The reverse bindings work similarly to Cgo, where the AST of Go source files are analyzed for their imports matching Java/... or ObjC/... and for references using those imports. In a sense this is a chicken-and-egg problem because Go source that imports the magic packages are not buildable yet. However, since the analysis only needs the AST the chicken-and-egg problem didn't apply when using go/build. I don't know about go/packages.

@hyangah

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@hajimehoshi Indeed! Thanks for the small example for reproducing the issue.

 go clean -cache | env GO111MODULE=off GOPACKAGESDEBUG=true  go run . && env GO111MODULE=off GOPACKAGESDEBUG=true  go run .
2019/08/16 17:33:53 26.264922ms for GOROOT= GOPATH= GO111MODULE=off PWD=/Users/hakim/go/src/bar go "env" "GOMOD", stderr: <<>>
2019/08/16 17:33:53 26.415485ms for GOROOT= GOPATH= GO111MODULE=off PWD=/Users/hakim/go/src/bar go "list" "-m" "-json" "all", stderr: <>
2019/08/16 17:33:54 25.019105ms for GOROOT= GOPATH= GO111MODULE=off PWD=/Users/hakim/go/src/bar go "env" "GOPATH", stderr: <<>>
2019/08/16 17:33:54 52.135849ms for GOROOT= GOPATH= GO111MODULE=off PWD=/Users/hakim/go/src/bar go "list" "-e" "-json" "-compiled=true" "-test=false" "-export=false" "-deps=true" "-find=false" "--" "foo.com/cool", stderr: <>
[Foo]
2019/08/16 17:33:54 26.189039ms for GOROOT= GOPATH= GO111MODULE=off PWD=/Users/hakim/go/src/bar go "env" "GOMOD", stderr: <<>>
2019/08/16 17:33:54 26.442476ms for GOROOT= GOPATH= GO111MODULE=off PWD=/Users/hakim/go/src/bar go "list" "-m" "-json" "all", stderr: <>
2019/08/16 17:33:54 37.745538ms for GOROOT= GOPATH= GO111MODULE=off PWD=/Users/hakim/go/src/bar go "list" "-e" "-json" "-compiled=true" "-test=false" "-export=false" "-deps=true" "-find=false" "--" "foo.com/cool", stderr: <<>>
[foo.com/cool]

I will move this to x/tools/go/packages category.

@hyangah hyangah changed the title x/mobile: x/tools/go/packages.Load is unhappy about not-yet-existing packages for reverse binding x/tools/go/packages Load returns different results depending on build cache state Aug 16, 2019

@hyangah

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@hajimehoshi is it possible to work around the issue temporarily while we figure out what's going on with go/packages, by running Load twice to warm the cache?

@gopherbot

This comment has been minimized.

Copy link

commented Aug 17, 2019

Change https://golang.org/cl/190479 mentions this issue: cmd/gobind: fix the try bot failures.

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

I thought yes, but the trybots are still unhappy: https://storage.googleapis.com/go-build-log/c485506b/linux-amd64-androidemu_0afe1c3b.log

Apparently the error message comes from the Go compiler (the message "type-checking package ..." is defined at go/internal/srcimporter/srcimporter.go) so Go compiler version might matter. Unfortunately I could not reproduce this error with my local machine with Go 1.12 and Go 1.13 beta 1.

I'm fine reverting my change to use go/packages once, if we cannot find a good solution.

@hyangah

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@hyangah That's interesting. Then I'd like to move on the CL https://go-review.googlesource.com/c/mobile/+/190479, that should make the situation better. Could you take a look? Thanks!

@bcmills bcmills changed the title x/tools/go/packages Load returns different results depending on build cache state x/tools/go/packages: Load returns different results depending on build cache state Aug 19, 2019

gopherbot pushed a commit to golang/mobile that referenced this issue Aug 26, 2019
cmd/gobind: fix build-tag, CGO and load trybot failures
* Correctly format build tags to pass into go/packages
* Removes CGO_ENABLED=0 from a packages.Load configuration
* Calls go/packages.Load twice to work around a build cache
* staleness issue

These bugs were introduced by CL 189597.

Updates golang/go#27234.
Updates golang/go#33687.

Change-Id: I3ae6737bf53bbecda0c7e25885b9c6aea5779332
Reviewed-on: https://go-review.googlesource.com/c/mobile/+/190479
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>

@gopherbot gopherbot added the Tools label Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.