ci: strip debug symbols from release binaries - #2243
Conversation
Pass -ldflags="-s -w" via GO_BUILD_FLAGS in the dist and dist-fips jobs so release builds omit the Go symbol table and DWARF debug info. Local developer builds are unaffected.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Makefile has been modified to include Go linker flags ( Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 108-109: Summary: GO_BUILD_FLAGS is set to '-ldflags=-s -w' which
gets word-split into two args; change the env value so the entire -ldflags
payload is preserved as one argument. Instruction: update both GO_BUILD_FLAGS
definitions in the workflow to embed the ldflags in an extra pair of quotes (so
the expanded value becomes a single quoted argument, e.g. "-ldflags=-s -w") and
verify the Makefile targets that use GO_BUILD_FLAGS (the build targets
referenced around the Makefile rule names that expand GO_BUILD_FLAGS) continue
to expand it as a single argument; this prevents the shell from splitting "-s
-w" into separate tokens during go build.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 106256ae-7aad-4b06-854a-5c5d9cba4896
📒 Files selected for processing (1)
.github/workflows/ci.yaml
Make performs textual substitution of $(GO_BUILD_FLAGS), leaving it to the shell to tokenize. Without inner quotes the shell split the value into "-ldflags=-s" and a bare "-w", which go build rejects. Quoting the value as -ldflags="-s -w" preserves it as a single argument.
|
@lindy65 this PR changes the binary generation - no code change so nothing to test |
pavelzeman
left a comment
pavelzeman
left a comment
There was a problem hiding this comment.
Approved.
It doesn't look like we're doing anything FIPS specific to plugins, but it'd be good to have Eva confirm.
Right I'll add Eva as reviewer and Alejandro, as he's the one usually creating the FIPS build for playbooks |
esarafianou
left a comment
esarafianou
left a comment
There was a problem hiding this comment.
@JulienTant shouldn't we change the Makefile server build step instead so that the debug symbols are stripped not only from the dev binaries created during CI but also from the final release artifact?
Plugins are released from our internal centralized delivery repo that triggers the plugin's makefile.
agarciamontoro
left a comment
agarciamontoro
left a comment
There was a problem hiding this comment.
Do we know how this affects the generation of stacktraces when we receive panic reports from Sentry?
|
I think we'll want to co-ordinate this PR with the change in mattermost/mattermost, specifically when it comes to striping the debug info and retaining it for released builds (just in case it's ever needed). |
Move -ldflags="-s -w" from CI env vars into the Makefile, gated by the existing MM_DEBUG convention. Stripping is now applied to every caller of make server / make dist, including the centralized release pipeline, not just the dev binaries built in this repo's CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Completely missed that! Thanks. Fixed with b2d0fc3 I also made a chance to make sure that when "MM_DEBUG" is set, we don't strip the debug symbols. Curious if it's something we might be interested for the main repo @pavelzeman? @agarciamontoro Not sure what is used for Sentry specifically, but stacktraces are complete and unaffected. I ran a small test locally to made sure of it. If Sentry reuse the panic stacktrace, it should work fine/ |
agarciamontoro
left a comment
agarciamontoro
left a comment
There was a problem hiding this comment.
Thanks, @JulienTant! I also verified the stacktraces are intact, so this looks safe to me :)
|
Ping @esarafianou |
|
Please don't merge quite yet - we need to decide if we are shipping two sets of bits (with and without debug symbols) or if we strip the symbols and ship a separate .debug info package (that could be used with a debugger). this applies here, in monorepo and anywhere else where we'd want to revisit the size of shipping binaries |
JulienTant
added
the
Do Not Merge
Copy! |
4 participants
Summary
Pass
-ldflags="-s -w"viaGO_BUILD_FLAGSin thedistanddist-fipsCI jobs so release builds omit the Go symbol table (-s) and DWARF debug info (-w). Local developer builds (make distrun outside CI) are unaffected. Go'sBuildInfosection is preserved, sogo version -mstill works on the stripped binaries.Binary size impact
Measured locally by building each target with and without
-ldflags="-s -w":Consistent ~30% reduction per binary; the bundle ships all five, so ~60 MiB off the uncompressed tarball.
Ticket Link
N/A
Checklist
Gated by experimental feature flagUnit tests updated🤖 Generated with Claude Code