chore(build): add build tags and missing flags/variables #2202
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis pull request updates the build and release configurations. The changes include adding environment variables and modifying build flags in the Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Mk as Makefile
participant YQ as yq Tool (via Docker)
participant Docker as Docker Container
Dev->>Mk: Trigger release/release-snapshot
Mk->>Mk: Execute shell to get CMT_VERSION
Mk->>YQ: Extract DARWIN_TAGS & LINUX_TAGS from .goreleaser.yml
YQ-->>Mk: Return tags values
Mk->>Docker: Run container with env vars (CMT_VERSION, DARWIN_TAGS, LINUX_TAGS)
Docker-->>Mk: Build & release artifacts
Mk->>Dev: Echo release status
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
helder-moreira
force-pushed
the
chore/builds-missing-tags
branch
from
24551ff to
2182063
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contrib/make/release.mk (1)
9-17: Consider caching Docker YQ results.The YQ commands are executed for every invocation of the Makefile targets. Consider caching the results in variables that are evaluated only once.
-DOCKER_YQ = docker run --rm -v $(PWD):/work -w /work mikefarah/yq:4 +DOCKER_YQ := docker run --rm -v $(PWD):/work -w /work mikefarah/yq:4 -DARWIN_TAGS := $(shell $(DOCKER_YQ) e \ +DARWIN_TAGS != $(DOCKER_YQ) e \ '.builds | map(select(.id == "darwin")) | .[0].tags | join(",")' \ .goreleaser.yml) -LINUX_TAGS := $(shell $(DOCKER_YQ) e \ +LINUX_TAGS != $(DOCKER_YQ) e \ '.builds | map(select(.id == "linux")) | .[0].tags | join(",")' \ .goreleaser.yml)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.goreleaser.yml(3 hunks)CHANGELOG.md(1 hunks)contrib/make/build.mk(3 hunks)contrib/make/release.mk(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🔇 Additional comments (8)
contrib/make/release.mk (2)
7-7: LGTM! Good use ofgo listto extract CometBFT version.The command correctly extracts the version from go.mod using
go list -mandsed.
24-26: LGTM! Good addition of debug output and environment variables.The echo statements help with debugging, and the environment variables are properly quoted.
Also applies to: 32-34
contrib/make/build.mk (3)
28-28: LGTM! Consistent with release.mk.The CMT_VERSION extraction is identical to release.mk, ensuring consistency.
128-128: LGTM! Good addition of -trimpath flag.The -trimpath flag helps create reproducible builds by removing file system paths from the binary.
56-57: Verify the impact of database backend change.The switch from rocksdb to pebbledb as the default database backend is a significant change.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for any hardcoded rocksdb references that might need updating rg -l 'rocksdb' --type goLength of output: 25
Database Backend Update Impact Verified
The search for "rocksdb" in the Go codebase produced no results, indicating that no parts of the repository rely on the old RocksDB backend. The build flag update to set the default to PebbleDB appears isolated and without additional side effects.
.goreleaser.yml (3)
5-6: LGTM! Good environment variable configuration.The GO111MODULE and DEFAULT_DB_BACKEND environment variables are properly set.
36-38: LGTM! Consistent use of environment variables.The build flags correctly use the environment variables defined in the Makefiles:
- DARWIN_TAGS/LINUX_TAGS for BuildTags
- CMT_VERSION for CMTSemVer
- DEFAULT_DB_BACKEND for DBBackend
Also applies to: 78-80
40-40: Verify linker flags for both platforms.The extldflags differ between Darwin and Linux builds:
- Darwin: '-lrocksdb -lstdc++ -lz -lbz2'
- Linux: '-static -lm -lbz2'
Ensure these flags are appropriate for each platform.
Also applies to: 82-82
✅ Verification successful
🌐 Web query:
What are the recommended linker flags for Go CGO builds on Darwin vs Linux?💡 Result:
When building Go programs with CGO across different operating systems, linker flag requirements differ significantly between Darwin (macOS) and Linux due to platform-specific constraints:
Linux Recommendations
Static Linking
Use external linker mode combined with static flags:go build -ldflags "-linkmode external -extldflags '-static'"This forces full static compilation by invoking GCC's linker[3][5].
Limitations: May fail if system libraries lack static versions[3][5].Dynamic Linking (Default)
Works out-of-box when using standard CGO:go buildAutomatically links against system's shared libraries[3][8].
Darwin (macOS) Considerations
System Library Constraints
Apple's libSystem cannot be statically linked[3][10].# Partial static linking (non-system libs only) go build -ldflags "-linkmode external -extldflags '-static'"Framework Linking
For macOS frameworks:// #cgo darwin LDFLAGS: -framework CoreFoundationCross-Platform Build Directives
Use conditional compilation in source files:
// #cgo linux LDFLAGS: -L/usr/local/lib -lfoo // #cgo darwin LDFLAGS: -L/opt/homebrew/lib -lfoo import "C"Key differences table:
Platform Static Linking Support Common Flags Notes Linux Full via -static-extldflags '-static'Requires static libs available Darwin Partial -frameworkflagsSystem libs remain dynamic For architecture-specific paths (x86_64 vs ARM), use separate build tags rather than combined
GOOS/GOARCHsyntax[9]. When encountering "undefined reference" errors on Linux, verify library order inLDFLAGSmatches linker requirements[1][8].Citations:
- 1: cmd/cgo: probable cgo gcc linker issue "undefined reference" golang/go#29296
- 2: cgo:
go linkdetects all linker flags as unsupported when using custom cc toolchain bazel-contrib/rules_go#3886- 3: https://mt165.co.uk/blog/static-link-go/
- 4: https://pkg.go.dev/cmd/link
- 5: https://ragoragino.dev/tech/2021-03-23-cgo-static/
- 6: https://dev.to/jacktt/go-build-in-advance-4o8n
- 7: https://pkg.go.dev/cmd/cgo
- 8: https://dev.to/metal3d/understand-how-to-use-c-libraries-in-go-with-cgo-3dbn
- 9: https://groups.google.com/g/golang-nuts/c/_rUnWNbHwH8
- 10: cmd/link: [cgo] Linker error with Go tip on darwin, ld: illegal text-relocation golang/go#9411
Linker Flags Reviewed for Darwin vs. Linux
Darwin (macOS):
The configuration uses
-lrocksdb -lstdc++ -lz -lbz2
which aligns with macOS requirements since full static linking isn’t supported. This set ensures that the necessary libraries—especially RocksDB—are linked dynamically, which is appropriate for Darwin.Linux:
The flag set is
-static -lm -lbz2
which is consistent with a fully static CGO build as recommended (i.e. using external linker flags to enforce static linking). Note that when using-static, it’s important to ensure that all required libraries (including math and bz2) have available static versions. If any library is missing its static variant, the build may fail; in that case, it might be necessary to switch to dynamic linking or adjust the flags accordingly.Both configurations follow the platform-specific recommendations for Go CGO builds. Please verify that the required static libraries are available on Linux and that the dynamic libraries are correctly installed on Darwin.
No description provided.