compilepkg: cgo assembly uses the C compiler #3648
Conversation
|
I used this example that uses the DataDog/zstd library to test this: https://github.com/evanj/ddzstdbazel This gets part of the way there! It fixes a "native" build e.g. |
|
This might also fix #2006 |
There was a problem hiding this comment.
Thanks, this is a very helpful contribution!
@tyler-french Could you test this at Uber (assuming you have some Assembler CGo deps)?
CC @jsharpe
|
Hmm looks like I didn't run the correct set of tests on my local branch; As noted in #2006:
I may need to modify that test. Let me look into that, and the build constraints are a great idea. Thanks. |
|
I will attempt to fix the failing tests in a separate PR. It is related, but does not depend on this. Once that PR is merged, I'll update this to the latest main branch. See #3652 I still need to add the build constraints. I'll do that in the next couple of days. |
…3652) The native "go build" tool compiles assembly in a Cgo package with the C compiler, and compiles assembly in a pure Go package with the Go assembler. The build_constraints test previously mixed Go assembler files with Cgo files. This caused the native go test to fail with: package using cgo has Go assembly file asm_linux_amd64.s Splitting this into two separate packages fixes this. This problem was reported in: #2006 This will fix failing tests in my attempt to fix that issue: #3648
|
okay some progress: I'll get back to this in a bit; looks like I need to use some more build constraints to get the test to work on Windows |
This changes compilepkg to use the C compiler for .S and .s files to use the C compiler, like go build does. Previously it would use the Go assembler, which is used for pure Go packages. This should help fix issue: #3411 I have added a cgo test for this issue that is intended to work with both arm64 and amd64 machines. I have only tested it on Linux amd64 and Darwin arm64. Without this change it fails to build with the following output. This fails to parse the assembly file because it uses the Go assembler. ERROR: rules_go/tests/core/cgo/asm/BUILD.bazel:3:11: GoCompilePkg tests/core/cgo/asm/asm.a failed: (Exit 1): builder failed: error executing command (from target //tests/core/cgo/asm:asm) bazel-out/k8-opt-exec-2B5CBBC6/bin/external/go_sdk/builder_reset/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src tests/core/cgo/asm/cgoasm.go -src tests/core/cgo/asm/asm_amd64.S ... tests/core/cgo/asm/asm_amd64.S:4: expected identifier, found "." asm: assembly of tests/core/cgo/asm/asm_amd64.S failed compilepkg: error running subcommand external/go_sdk/pkg/tool/linux_amd64/asm: exit status 1
The tests were failing on Windows because rules_go incorrectly believed this was a "native" Go package. I think this should work on Windows.
|
Did you intend to enable auto-merge on this PR? It was enabled then disabled. Thanks. |
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
This changes compilepkg to use the C compiler for .S and .s files to use the C compiler, like go build does. Previously it would use the Go assembler, which is used for pure Go packages. This should help fix issue: #3411
I have added a cgo test for this issue that is intended to work with both arm64 and amd64 machines. I have only tested it on Linux amd64 and Darwin arm64. Without this change it fails to build with the following output. This fails to parse the assembly file because it uses the Go assembler.
Which issues(s) does this PR fix?
Fixes # #3411
Other notes for review
This seems to pass
bazelisk test //tests/core/...on my machine.