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 returned packages with errors when specifying GOOS #36547

Open
hajimehoshi opened this issue Jan 14, 2020 · 22 comments
Open

Comments

@hajimehoshi
Copy link
Contributor

@hajimehoshi hajimehoshi commented Jan 14, 2020

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

$ go version
$ go version
go version go1.13.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/hajimehoshi/Library/Caches/go-build"
GOENV="/Users/hajimehoshi/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/hajimehoshi/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
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-build430597543=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Created these files in one directory.

go.mod:

module example.com/m

go 1.13

require golang.org/x/tools v0.0.0-20200114052453-d31a08c2edf2 // indirect 

main.go:

// +build ignore                                                                                                                                                                                                                              

package main

import (
        "fmt"
        "os"

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

func main() {
        cfg := &packages.Config{
                 Mode: packages.NeedName | packages.NeedFiles |
                        packages.NeedImports | packages.NeedDeps |
                        packages.NeedTypes | packages.NeedSyntax | packages.NeedTypesInfo,
                Env: append(os.Environ(), "GOOS=android", "CGO_ENABLED=1"),
        }

        allPkg, err := packages.Load(cfg, ".")
        if err != nil {
                panic(err)
        }
        for _, pkg := range allPkg {
                if len(pkg.Errors) > 0 {
                        panic(fmt.Sprintf("%v", pkg.Errors))
                }
        }
}

c.go:

package lib

import "C"

What did you expect to see?

No error

What did you see instead?

$ gl clean -cache && go run main.go
panic: [/Users/hajimehoshi/test/cgo/c.go:3:8: could not import C (no metadata for C)]

goroutine 1 [running]:
main.main()
        /Users/hajimehoshi/test/cgo/main.go:24 +0x2c6
exit status 2

This is similar to #36441

@gopherbot gopherbot added this to the Unreleased milestone Jan 14, 2020
@gopherbot gopherbot added the Tools label Jan 14, 2020
@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 14, 2020

@hajimehoshi hajimehoshi changed the title x/tools/go/packages: Load returned error when specifying GOOS x/tools/go/packages: Load returned packages with errors when specifying GOOS Jan 14, 2020
@hyangah

This comment has been minimized.

Copy link
Contributor

@hyangah hyangah commented Jan 14, 2020

If GOOS is not specified, does it work? I wonder if this is the same issue as #36441.

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 14, 2020

Hmm, I thought yes, but it looks like this depends on the cache state. Let me double check.

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 14, 2020

Oops, now I found this issue even without GOOS. Then this is the same as #36441.

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 14, 2020

Removing packages.ModeTypes was necessary to reproduce the issue.

@hajimehoshi hajimehoshi changed the title x/tools/go/packages: Load returned packages with errors when specifying GOOS x/tools/go/packages: Load returned packages with errors when specifying ModeTypes Jan 14, 2020
@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 14, 2020

Updated the first comment, but I'm fine to close this marking 'duplicated'.

@hajimehoshi hajimehoshi changed the title x/tools/go/packages: Load returned packages with errors when specifying ModeTypes x/tools/go/packages: Load returned packages with errors when specifying GOOS Jan 14, 2020
@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 14, 2020

Now I realized I omitted packages.Need*, and this was wrong. I updated the issue again. Sorry for the confusion.

If GOOS is not specified, does it work? I wonder if this is the same issue as #36441.

Then yes it worked without GOOS. This can be reproduced when specifying various packages.Mode* values. This sounds a different issue from #36441.

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 15, 2020

I'm taking a look at this.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 15, 2020

Change https://golang.org/cl/214738 mentions this issue: go/packages: ignore importing C

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 15, 2020

Change https://golang.org/cl/214738 mentions this issue: go/packages: ignore package C when analyzing its metadata

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 15, 2020

Change https://golang.org/cl/214943 mentions this issue: go/packges: fix incorrect needtypes and needsrcs logic

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 17, 2020

@matloob @hyangah Hi, this is not fixed yet with golang.org/x/tools v0.0.0-20200117012304-6edc0a871e69 (at least on my macOS).

@matloob

This comment has been minimized.

Copy link
Contributor

@matloob matloob commented Jan 17, 2020

Hm okay, I'll take another look.

@matloob matloob reopened this Jan 17, 2020
@matloob

This comment has been minimized.

Copy link
Contributor

@matloob matloob commented Jan 17, 2020

I think I'll need some help filling in my knowledge of android: I tried to go a go list in your example module: $ CGO_ENABLED=1 GOOS=android go list -e -json -compiled example.com/m

# runtime/cgo
gcc_android.c:6:10: fatal error: 'android/log.h' file not found
{
	"Dir": "/Users/matloob/Desktop/hajimehoshi",
	"ImportPath": "example.com/m",
	"Name": "lib",
	"Root": "/Users/matloob/Desktop/hajimehoshi",
	"Module": {
		"Path": "example.com/m",
		"Main": true,
		"Dir": "/Users/matloob/Desktop/hajimehoshi",
		"GoMod": "/Users/matloob/Desktop/hajimehoshi/go.mod",
		"GoVersion": "1.13"
	},
	"Match": [
		"example.com/m"
	],
	"CgoFiles": [
		"c.go"
	],
	"IgnoredGoFiles": [
		"main.go"
	],
	"Imports": [
		"C",
		"unsafe",
		"runtime/cgo",
		"syscall"
	],
	"Deps": [
		"errors",
		"internal/bytealg",
		"internal/cpu",
		"internal/oserror",
		"internal/race",
		"internal/reflectlite",
		"runtime",
		"runtime/cgo",
		"runtime/internal/atomic",
		"runtime/internal/math",
		"runtime/internal/sys",
		"sync",
		"sync/atomic",
		"syscall",
		"unsafe"
	]
}

I don't totally see what's going on but it looks like the problem is that we need the android C libs (like android/log.h to build this. So in general, we won't be able to get the true cgo-processed files unless we're running on a system configured correctly to build android. (And I guess this will be a problem for other platforms as well).

I'm really hesitant about stubbing out the C package with a fake empty package because the results are not totally correct. I wonder what our other options are. Can gomobile guarantee the android C libs exist by setting the right environment variables, etc? Can it inject a fake C package if it knows it will work?

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 17, 2020

Just FYI: this issue is reproduced not only with GOOS=android but also other values like GOOS=windows

@matloob

This comment has been minimized.

Copy link
Contributor

@matloob matloob commented Jan 17, 2020

Right: windows returns an error like the following:

# runtime/cgo
gcc_libinit_windows.c:7:10: fatal error: 'windows.h' file not found

I don't think we can get this to work in general.

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 17, 2020

Can gomobile guarantee the android C libs exist by setting the right environment variables, etc?

I think assuming this makes sense. I'd want a proper way to skip/ignore the reported issues around importing C.

Can it inject a fake C package if it knows it will work?

I was wondering how.

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 17, 2020

We discussed and the conclusion is that we should specify C compilers, or go/packages cannot do type-checking correctly. Then, the current error makes sense to me.

In the context of gomobile, we should specify environment variables like CC or CXX when calling packages.Load. These variables are defined cmd/gomobile/env.go, so we should be able to utilize them. If these do not work, we would need to ignore pkg.Errors as the final resort...

@hyangah

This comment has been minimized.

Copy link
Contributor

@hyangah hyangah commented Jan 17, 2020

@matloob I don't know much about go/packages but, philosophically, given that some analysis tools and analysis libraries can operate on even broken source code, I wonder why we need to enforce this strict restriction on cgo. What is the potential concern if we skip or ignore the cgo related errors? Will it cause the gobind or similar tools to generate incorrect binding code?

Is there any option to skip cgo but still analyze the go part?

@matloob

This comment has been minimized.

Copy link
Contributor

@matloob matloob commented Jan 17, 2020

It's okay to ignore errors. go/packages already tries to do the best it can, and if the information that gomobile needs doesn't depend on perfect code then it should work fine as is.

Specifically for this bug, what I meant was that if we can't depend on Load returning a package if cgo can't run. That's what this bug was originally filed for. But that doesn't mean that tools can't work on the results of go/packagse.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 21, 2020

Change https://golang.org/cl/214498 mentions this issue: cmd/gomobile: enable Cgo

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 21, 2020

OK I'll adopt the way to just ignore Errors for #27234.

gopherbot pushed a commit to golang/mobile that referenced this issue Jan 23, 2020
This CL gives CGO_ENABLED=1 explicitly when executing gobind since
Cgo is disabled by default when GOOS is given.

This CL also adds importing C to the tests to confirm that Cgo
works correctly.

This CL also updates go.mod since this change requries the change
in go/packages: golang.org/cl/214943

Updates golang/go#27234
Updates golang/go#36547

Change-Id: I66f9697f992f15b52fca7871e4e0ed64ca2b4965
Reviewed-on: https://go-review.googlesource.com/c/mobile/+/214498
Run-TryBot: Hajime Hoshi <hajimehoshi@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.