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

Support cross-builds and cgo #26

Open
karalabe opened this issue Nov 10, 2017 · 9 comments
Open

Support cross-builds and cgo #26

karalabe opened this issue Nov 10, 2017 · 9 comments

Comments

@karalabe
Copy link

karalabe commented Nov 10, 2017

The type of syscall.Stdin differs on Linux (int) and Windows (syscall.Handle). As such, any calling code needs to always cast syscall.Stdin to an integer to use it. But this gets flagged by unconvert on Linux.

My current workaround is to do int(os.Stdin.Fd()) instead of syscall.Stdin. Would be nice to do without the hack.

@mdempsky
Copy link
Owner

To confirm, are you using unconvert's -all flag?

@karalabe
Copy link
Author

karalabe commented Nov 10, 2017

I'm using unconvert via the gometalinter using --enable=unconvert. Not sure exactly how the meta linter invokes unconvert.

@dmitshur
Copy link
Collaborator

According to https://github.com/alecthomas/gometalinter/blob/0262fb20957a4c2d3bb7c834a6a125ae3884a2c6/linters.go#L373, it looks like gometalinter invokes unconvert without -all or any other flags.

@mdempsky
Copy link
Owner

I see. Unconvert should support your case with the -all flag. If not, that's definitely a bug.

I'm not familiar with gometalinter though. I don't know how to have it run unconvert with -all, or if that's even possible without changing the source.

@karalabe
Copy link
Author

I did try running locally with the --all option on my repo, but it dies on some CGO packages:

$ unconvert --all ./cmd/puppeth/
/work/src/github.com/ethereum/go-ethereum/crypto/signature_cgo.go:27:2: could not import github.com/ethereum/go-ethereum/crypto/secp256k1 (invalid package name: "")
2017/11/10 18:34:47 couldn't load packages due to errors: github.com/ethereum/go-ethereum/crypto

@dnephin
Copy link

dnephin commented Jan 8, 2018

You can customize the command run by gometalinter using a config file:

{
  "Linters": {
    "unconvert": {"Command": "unconvert --all"}
  }
}

I tried --all on another repo, and I hit this problems with CGO as well.

snapshots/btrfs/btrfs.go:212:12: SubvolCreate not declared by package btrfs

However:

  • this function is declared in the btrfs package
  • there are no build tags on the file
  • it doesn't fail without --all

The only thing different about this package seems to be the import "C"

@mdempsky
Copy link
Owner

mdempsky commented Jan 11, 2019

The problem here is that when cross-compiling, the Go build system disables cgo support by default. When cgo is disabled, files that include import "C" are automatically omitted from the build, just as if they contained an unsatisfied //+build predicate or _${GOOS/GOARCH} file name suffix.

You can setup CGO_ENABLED=1 to force cgo to be available in cross-compiles, but then you'll also have to set CC_FOR_${GOOS}_${GOARCH} and CXX_FOR_${GOOS}_${GOARCH} for every build target that uses cgo.

It's probably unlikely that you have Plan 9, Solaris, AIX, etc. cross compilers handy, however. (And assuming your code even compiles on them.)

--

What I'm thinking is the best solution here is to provide more control over the set of environment variables that unconvert runs in -all mode. For example, maybe you don't even really care about every OS/arch, and you've setup a C cross-compiler for Windows and Linux x86. I think it would be reasonable to support a config file like:

[
  // Host build; cgo support implied.
  ["GOOS=linux", "GOARCH=amd64"],

  // Cross-compile builds with explicit cgo support via cross-compilers.
  ["GOOS=linux", "GOARCH=386", "CGO_ENABLED=1", "CC_FOR_TARGET=gcc -m32"],
  ["GOOS=windows", "GOARCH=amd64", "CGO_ENABLED=1", "CC_FOR_TARGET=mingw32-gcc"],

  // Go-only cross builds.
  ["GOOS=darwin", "GOARCH=amd64"],
  ["GOOS=openbsd", "GOARCH=arm"],
]

mdempsky added a commit that referenced this issue Jan 11, 2019
With this, users can supply a list of build configs (each specified as
a list of extra key=val environment variables to set) under which to
run unconvert. Unconvert will then load the packages under all
configs, and use the same logic as -all to merge them.

For example, on a linux/amd64 host with gcc cross-compilers for
linux/386, you might use

    unconvert -configs='[[], ["GOARCH=386", "CGO_ENABLED=1", "CC_FOR_TARGET=gcc -m32"]]' mypkgs...

to test both the native build and linux/386 build with cgo support.

Updates #26.
@mdempsky
Copy link
Owner

mdempsky commented Jan 11, 2019

I added support at master for a -configs flag. Please take a look and let me know if it's helpful.

Open questions:

  • Is JSON the best configuration format for this?
  • Is listing out the config on the command-line okay, or do folks want to store it in a file? (Even as is, you can always do something like -configs="$(cat unconvert.json)".)
  • Should I allow setting build flags (e.g., -tags) on a per-config basis?

@mdempsky mdempsky changed the title False positives with syscall.Stdin Support cross-builds and cgo Jan 11, 2019
@chipaca
Copy link
Contributor

chipaca commented Aug 25, 2021

I'm using the -configs option to limit cross-compilation to the combinations we support. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants