Skip to content

Comments

Update go_library build rule to compile assembly files individually#330

Merged
chrisnovakovic merged 8 commits intoplease-build:masterfrom
kelvinjhwong:compile-assembly-files-individually
Nov 5, 2025
Merged

Update go_library build rule to compile assembly files individually#330
chrisnovakovic merged 8 commits intoplease-build:masterfrom
kelvinjhwong:compile-assembly-files-individually

Conversation

@kelvinjhwong
Copy link
Contributor

@kelvinjhwong kelvinjhwong commented Nov 4, 2025

Currently, when compiling a Go package with Assembly files, the go_library build rule invokes go tool asm once on all Assembly files in the package. However, this caused errors that look like the below when multiple files declare symbols with the same name.

asm: pkg/linux_amd64/github.com/apache/arrow-go/v18/internal/utils/min_max_sse4_amd64.s:8: symbol LCDATA1 redeclared
asm: pkg/linux_amd64/github.com/apache/arrow-go/v18/internal/utils/min_max_sse4_amd64.s:242: symbol LCDATA2 redeclared
asm: pkg/linux_amd64/github.com/apache/arrow-go/v18/internal/utils/min_max_sse4_amd64.s:460: symbol LCDATA3 redeclared
asm: pkg/linux_amd64/github.com/apache/arrow-go/v18/internal/utils/min_max_sse4_amd64.s:691: symbol LCDATA4 redeclared
asm: pkg/linux_amd64/github.com/apache/arrow-go/v18/internal/utils/min_max_sse4_amd64.s:847: symbol LCDATA5 redeclared
asm: assembly failed

This is despite the <> suffix after the offending symbol name, which should've been enough to make the symbol visible in that file only from the perspective of go tool asm.

When building this same package without Please (i.e. just with go build), we don't run into these errors - it looks like this is because the Go tool invokes go tool asm on each Assembly individually:

So this PR updates the go_library build def's behaviour to more closely match the behaviour in the Go tool.

I was able to reproduce these errors by adding the new unit tests in this PR. The errors were fixed by my updates to the go_library build def.

@kelvinjhwong kelvinjhwong marked this pull request as ready for review November 4, 2025 11:46
@chrisnovakovic
Copy link
Contributor

Thanks, Kelvin - I've made some minor improvements to how the cmd is constructed (it's now more "Please-like"), made the invocation of basename POSIX-friendly so it works on FreeBSD and macOS, and switched to explicitly enumerating the output files rather than using output_dirs (which has the potential to cause build failures due to duplicated outputs in degenerate cases).

@chrisnovakovic chrisnovakovic merged commit 165de83 into please-build:master Nov 5, 2025
5 checks passed
@chrisnovakovic chrisnovakovic deleted the compile-assembly-files-individually branch November 5, 2025 16:14
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

Successfully merging this pull request may close these issues.

2 participants