-
Notifications
You must be signed in to change notification settings - Fork 256
fix: build extra static bins without ledger support #2027
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
Conversation
Signed-off-by: Artur Troian <[email protected]>
WalkthroughUpdates build configuration to add static Linux builds in goreleaser for amd64 and arm64 architectures, restructures Makefile BUILD_TAGS with conditional ledger support, and changes default test snapshot network from mainnet to sandbox-2. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.goreleaser.yaml (1)
168-168: Consider removing the redundant explicit name template.Line 167 indicates this is the default format. Since the specified template matches the default, this line could be removed to reduce configuration noise.
- name_template: "akash_{{ .Version }}_checksums.txt"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.goreleaser.yaml(2 hunks)Makefile(2 hunks)make/test-upgrade.mk(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: network-upgrade
- GitHub Check: release-dry-run
- GitHub Check: sims
- GitHub Check: coverage
- GitHub Check: build-macos
- GitHub Check: build-bins
- GitHub Check: lint-go
- GitHub Check: tests
🔇 Additional comments (5)
Makefile (1)
29-29: LGTM!Adding
osusergoandnetgoas default build tags is appropriate for enabling static linking and pure Go implementations of OS and network interfaces..goreleaser.yaml (3)
87-105: Static build configuration looks correct.The static build intentionally uses hardcoded tags
osusergo,netgo(Line 99) without ledger support, aligning with the PR objective to build static binaries without ledger. The--staticflag in extldflags (Line 105) enables full static linking.
106-124: LGTM!The ARM64 static build configuration correctly mirrors the AMD64 static build, using the same tag strategy and static linking flags.
155-164: LGTM!The static archive configuration appropriately bundles the static binaries with a clear naming convention (
_staticsuffix) that distinguishes them from the regular builds.make/test-upgrade.mk (1)
24-24: Infrastructure verification confirms sandbox-2 resources are accessible and operational.The verification script results show that both the chain metadata URL and the snapshot service for sandbox-2 are operational:
- Metadata endpoint: ✓ Accessible (returns valid meta.json)
- Snapshot service: Responds with HTTP 302 (operational and routing correctly)
The change from
mainnettosandbox-2is supported by functional infrastructure. Tests can proceed with the new default network source.
| ifneq (,$(findstring noledger,$(BUILD_OPTIONS))) | ||
| BUILD_TAGS := $(BUILD_TAGS),hidraw,ledger | ||
| endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional logic appears inverted.
The condition adds hidraw and ledger tags when noledger is present in BUILD_OPTIONS. This seems counterintuitive—if noledger is specified, one would expect ledger-related tags to be excluded, not added.
If the intent is to add ledger support by default and only exclude it when noledger is specified, the logic should be inverted:
-ifneq (,$(findstring noledger,$(BUILD_OPTIONS)))
+ifeq (,$(findstring noledger,$(BUILD_OPTIONS)))
BUILD_TAGS := $(BUILD_TAGS),hidraw,ledger
endifIf the current logic is intentional, please add a comment explaining why noledger triggers the addition of ledger tags.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ifneq (,$(findstring noledger,$(BUILD_OPTIONS))) | |
| BUILD_TAGS := $(BUILD_TAGS),hidraw,ledger | |
| endif | |
| ifeq (,$(findstring noledger,$(BUILD_OPTIONS))) | |
| BUILD_TAGS := $(BUILD_TAGS),hidraw,ledger | |
| endif |
🤖 Prompt for AI Agents
In Makefile around lines 42 to 44, the conditional is inverted: it appends
hidraw and ledger tags when BUILD_OPTIONS contains "noledger". Change the
condition so the tags are added only when "noledger" is NOT present (i.e.,
invert the findstring check), or alternatively move the tag addition to the
default path and add a branch that removes or omits those tags when "noledger"
is present; if leaving current behavior, add a clear comment explaining why the
presence of "noledger" must enable ledger tags.
|
Marked as stale; will be closed in five days. |
No description provided.