[v3] allow build with garble #3192
Conversation
WalkthroughThe pull request introduces a series of updates to the project's changelog, documenting new features, modifications, and bug fixes. Key additions include commands for updating build assets and generating runtime, enhancements to window positioning, and improvements to the Events API. Changes also encompass a refactor of the Taskfile and updates to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
|
Events are working
But |
|
I think there's a bit more to do but it's fairly easy. |
![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
🧹 Outside diff range and nitpick comments (3)
v3/pkg/application/application_debug.go (2)
43-47: Consider adding minimal debug info for Garble builds.While Garble strips debug information, it might be helpful to log a message indicating that the application was built with Garble, providing some minimal context for debugging.
Here's a suggested enhancement:
// BuildInfo is nil when build with garble if BuildInfo == nil { + a.info("Build Info: Application built with Garble (debug information stripped)") return }
Line range hint
1-47: Address method invocation issues with Garble.Based on the PR comments, there are issues with
Call.ByIDandCall.ByNamenot locating target methods due to Garble's obfuscation. This file handles the immediate BuildInfo issue, but the method invocation problem needs to be addressed.Would you like me to:
- Generate a solution for preserving method names during Garble builds?
- Create a GitHub issue to track the method invocation problems?
v3/internal/go-common-file-dialog/cfd/DialogConfig.go (1)
18-19: LGTM: Type preservation is correctly implementedThe type preservation for FileFilter is implemented using the standard pattern with reflect.TypeOf. This is crucial for maintaining type information when building with Garble.
Consider documenting this pattern in a central location (e.g., developer docs) as it might be needed for other types that should not be obfuscated when using Garble. Based on the PR comments about method discovery issues, you might need to apply similar preservation techniques to other types involved in method calls like
Call.ByIDandCall.ByName.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
mkdocs-website/docs/en/changelog.md(2 hunks)v3/internal/go-common-file-dialog/cfd/DialogConfig.go(2 hunks)v3/pkg/application/application_debug.go(1 hunks)
🔇 Additional comments (4)
v3/pkg/application/application_debug.go (1)
43-47: LGTM! Proper handling of nil BuildInfo for Garble builds.
The added nil check is essential as Garble strips debug information, making BuildInfo nil. This change prevents potential panic scenarios.
Let's verify the behavior with Garble:
v3/internal/go-common-file-dialog/cfd/DialogConfig.go (2)
6-6: LGTM: Import addition is appropriate
The reflect package import is necessary for type preservation and is correctly grouped with other standard library imports.
18-19: Verify other types that might need preservation
Let's check for other types in the codebase that might need similar preservation when using Garble.
mkdocs-website/docs/en/changelog.md (1)
Line range hint 140-152: Verify binding generator changes compatibility with Garble.
The binding generator now uses calls by ID by default, which is relevant to the known issue with Garble obfuscation affecting method targeting. This change might need additional documentation or considerations for Garble users.
Consider adding a note about Garble compatibility in this section:
The binding generator now uses calls by ID by default. The `-id` CLI option has been removed. Use the `-names` CLI option to switch back to calls by name. By [@fbbdev](https://github.com/fbbdev) in [#3468](https://github.com/wailsapp/wails/pull/3468)
+Note: When building with Garble, method targeting through IDs may be affected by obfuscation. Additional configuration might be required.Let's verify the impact of these changes on Garble builds:
| @@ -41,7 +41,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - Fixed `AlwaysOnTop` not working on Mac by [leaanthony](https://github.com/leaanthony) in [#3841](https://github.com/wailsapp/wails/pull/3841) | |||
| - [darwin] Fixed `application.NewEditMenu` including a duplicate `PasteAndMatchStyle` role in the edit menu on Darwin by [johnmccabe](https://github.com/johnmccabe) in [#3839](https://github.com/wailsapp/wails/pull/3839) | |||
| - [linux] Fixed aarch64 compilation [#3840](https://github.com/wailsapp/wails/issues/3840) in [#3854](https://github.com/wailsapp/wails/pull/3854) by [kodflow](https://github.com/kodflow) | |||
|
|
|||
| - Fixed garble build by @5aaee9 in [#3192](https://github.com/wailsapp/wails/pull/3192) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the changelog entry with more context about Garble integration.
The current entry "Fixed garble build" is too brief. Consider expanding it to provide more context about the Garble integration and known limitations:
-Fixed garble build by @5aaee9 in [#3192](https://github.com/wailsapp/wails/pull/3192)
+Added support for building with Garble (Go binary obfuscator) by @5aaee9 in [#3192](https://github.com/wailsapp/wails/pull/3192). Note: Method invocation through `Call.ByID` and `Call.ByName` may be affected by obfuscation.📝 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.
| - Fixed garble build by @5aaee9 in [#3192](https://github.com/wailsapp/wails/pull/3192) | |
| Added support for building with Garble (Go binary obfuscator) by @5aaee9 in [#3192](https://github.com/wailsapp/wails/pull/3192). Note: Method invocation through `Call.ByID` and `Call.ByName` may be affected by obfuscation. |
|
Hi @5aaee9 - Are you still interested in adding Garble support to v3? I just tested this PR and noticed that the bindings don't work, because part of the path to the method has been obfuscated: To reproduce the bindings issue, just generate a project and build with garble. I added |
Description
This patch make wails v3 app able to build with garble
Tested build on Windows and macOS
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Bug Fixes
AlwaysOnTopon Mac and compilation for aarch64 on Linux.Changes
go-webview2library.