fix(v2): strip debug symbols from wailsbindings binary for Go 1.25 compat#5220
fix(v2): strip debug symbols from wailsbindings binary for Go 1.25 compat#5220leaanthony wants to merge 1 commit intomasterfrom
Conversation
…mpat Go 1.25 defaults to DWARF5, which combined with CGO produces malformed Windows PE binaries (golang/go#75077). The wailsbindings binary is a temporary throwaway helper used only for binding generation, so stripping symbols with -ldflags -s -w is safe and avoids the bug entirely. Also fixes #4605, #4831 Fixes #4551
WalkthroughThe PR adds Go linker flags Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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 `@v2/test/4551/bindings_test.go`:
- Around line 13-15: Replace the silent skip with a hard failure: in
v2/test/4551/bindings_test.go where the code checks "if err != nil {
t.Skipf(...)" change it to "t.Fatalf(...)" (or t.Fatalf with the same formatted
message) so that when the bindings source cannot be read the test fails
(preserve the "%v" error interpolation on the existing err variable).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7963e7f3-345c-409b-bbed-b000b5591109
📒 Files selected for processing (2)
v2/pkg/commands/bindings/bindings.gov2/test/4551/bindings_test.go
| if err != nil { | ||
| t.Skipf("could not read bindings source: %v", err) | ||
| } |
There was a problem hiding this comment.
Fail instead of skip when the source file cannot be read.
At Line [14], t.Skipf can silently bypass this regression test if the path/setup breaks. This should be a hard failure so CI still protects the fix.
Suggested patch
data, err := os.ReadFile(sourceFile)
if err != nil {
- t.Skipf("could not read bindings source: %v", err)
+ t.Fatalf("could not read bindings source: %v", err)
}📝 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.
| if err != nil { | |
| t.Skipf("could not read bindings source: %v", err) | |
| } | |
| data, err := os.ReadFile(sourceFile) | |
| if err != nil { | |
| t.Fatalf("could not read bindings source: %v", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v2/test/4551/bindings_test.go` around lines 13 - 15, Replace the silent skip
with a hard failure: in v2/test/4551/bindings_test.go where the code checks "if
err != nil { t.Skipf(...)" change it to "t.Fatalf(...)" (or t.Fatalf with the
same formatted message) so that when the bindings source cannot be read the test
fails (preserve the "%v" error interpolation on the existing err variable).
There was a problem hiding this comment.
Pull request overview
This PR updates the v2 bindings-generation helper (wailsbindings) build to strip debug symbols via -ldflags "-s -w", mitigating the Go 1.25 DWARF5+CGO malformed Windows PE issue during TypeScript binding generation, and adds a regression test to ensure the flag remains present.
Changes:
- Add
-ldflags "-s -w"to thego buildinvocation used to compile the temporarywailsbindingshelper binary. - Add a regression test that asserts the bindings helper build command includes the strip-symbol ldflags.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| v2/pkg/commands/bindings/bindings.go | Adds -ldflags "-s -w" to the wailsbindings build command. |
| v2/test/4551/bindings_test.go | Adds a test asserting the presence of the ldflags in the bindings helper build command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| envBuild = shell.RemoveEnv(envBuild, "CC") | ||
|
|
||
| stdout, stderr, err = shell.RunCommandWithEnv(envBuild, workingDirectory, options.Compiler, "build", "-buildvcs=false", "-tags", tagString, "-o", filename) | ||
| stdout, stderr, err = shell.RunCommandWithEnv(envBuild, workingDirectory, options.Compiler, "build", "-buildvcs=false", "-tags", tagString, "-ldflags", "-s -w", "-o", filename) |
There was a problem hiding this comment.
The -ldflags value is hardcoded here even though production builds already add strip flags in v2/pkg/commands/build/base.go. To avoid these drifting out of sync (order/extra flags like -H windowsgui), consider factoring the strip-symbol ldflags into a shared helper/constant and reusing it for both the app build and the wailsbindings helper build.
| sourceFile := filepath.Join("..", "..", "pkg", "commands", "bindings", "bindings.go") | ||
| data, err := os.ReadFile(sourceFile) | ||
| if err != nil { | ||
| t.Skipf("could not read bindings source: %v", err) |
There was a problem hiding this comment.
This test skips when it can't read the source file. In CI/repo tests, failing to locate bindings.go should be a hard failure (otherwise the test can silently pass even if the path is wrong or the file moves). Prefer t.Fatalf(...) (or t.Fatal(...)) here instead of t.Skipf(...).
| t.Skipf("could not read bindings source: %v", err) | |
| t.Fatalf("could not read bindings source: %v", err) |
| } | ||
|
|
||
| source := string(data) | ||
| if !strings.Contains(source, `"-ldflags", "-s -w"`) { |
There was a problem hiding this comment.
The assertion depends on an exact source-code substring (including whitespace/formatting). A gofmt reflow of the RunCommandWithEnv(...) call could break this test without changing behavior. Consider making the check tolerant to formatting (e.g., search separately for "-ldflags" and "-s -w", or strip whitespace, or parse the file with go/parser and inspect the call args).
| if !strings.Contains(source, `"-ldflags", "-s -w"`) { | |
| if !strings.Contains(source, `"-ldflags"`) || !strings.Contains(source, `"-s -w"`) { |
|
🤖 PR Triage Review ✅ Accepted Strips debug symbols from wailsbindings binary for Go 1.25 compatibility (v2 CLI). Important compatibility fix. Platform: All (v2, CLI) Next Steps: Dispatching for testing on all platforms. Reviewed by Wails PR Reviewer Bot |
Summary
-ldflags "-s -w"to thewailsbindingsbuild command inv2/pkg/commands/bindings/bindings.gowailsbindingsbinary is a temporary throwaway helper used only for TypeScript binding generation, so stripping symbols is safe-w -sinbase.go) but was missing from the bindings helperTest plan
v2/test/4551/bindings_test.goverifies the ldflags are present in the build commandwails devandwails buildwithout the "not a valid Win32 application" errorAlso fixes #4605, #4831
Fixes #4551
Summary by CodeRabbit
Chores
Tests