mdempsky/unconvert

Support cross-builds and cgo

Opened this issue · 9 comments

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.

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

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

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

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.

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

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"

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"],
]

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?

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