Add GoLang binary builder plan using GoTask #55
Conversation
This plan outlines the refactoring of the build action to use GoTask (go-task/task) as the build orchestrator instead of inline shell scripts. Primary goals: - Wails 2 builds (preserve current functionality) - Wails 3 builds (new support) Key architectural changes: - Taskfile.yml with modular task includes (wails2, wails3, common) - Cross-platform consistency with single task definitions - Local development parity - same tasks run locally and in CI - Auto-detection of Wails version via wails.json parsing Also includes v3 branch code snapshot for reference.
Summary of ChangesHello @Snider, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural overhaul to the build system, transitioning from ad-hoc shell scripts to a more structured and maintainable approach using GoTask. This refactoring enhances the build process for Wails applications, providing explicit support for both Wails v2 and the upcoming Wails v3, while also laying the groundwork for future multi-stack build capabilities. The changes promote cross-platform consistency, improve local development parity, and are accompanied by comprehensive documentation for easier adoption and understanding. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring of the build action. Moving to a modular, sub-action-based architecture and using GoTask for build orchestration are excellent improvements for maintainability, cross-platform consistency, and local development parity. The addition of extensive documentation and test fixtures is also highly commendable.
I've identified a few issues, primarily related to the new orchestration logic and shell script correctness. The most critical issue is a hardcoded path in the Windows signing action that will cause it to fail on standard runners. There are also some redundancies in the action calls and a bug in how build tags are passed to the compiler. Addressing these points will make the new architecture more robust and efficient.
| - name: Sign Windows binaries | ||
| shell: powershell | ||
| if: runner.os == 'Windows' && inputs.sign != 'false' && inputs.sign-windows-cert != '' | ||
| working-directory: ${{ inputs.app-working-directory }} | ||
| run: | | ||
| echo "Creating certificate file" | ||
| New-Item -ItemType directory -Path certificate | ||
| Set-Content -Path certificate\certificate.txt -Value '${{ inputs.sign-windows-cert }}' | ||
| certutil -decode certificate\certificate.txt certificate\certificate.pfx | ||
| echo "Signing our binaries" | ||
| & 'C:/Program Files (x86)/Windows Kits/10/bin/10.0.17763.0/x86/signtool.exe' sign /fd sha256 /tr http://ts.ssl.com /f certificate\certificate.pfx /p '${{ inputs.sign-windows-cert-password }}' .\build\bin\${{ inputs.build-name }}.exe | ||
| echo "Signing Installer" | ||
| & 'C:/Program Files (x86)/Windows Kits/10/bin/10.0.17763.0/x86/signtool.exe' sign /fd sha256 /tr http://ts.ssl.com /f certificate\certificate.pfx /p '${{ inputs.sign-windows-cert-password }}' .\build\bin\${{ inputs.build-name }}-amd64-installer.exe |
There was a problem hiding this comment.
The path to signtool.exe is hardcoded to a specific Windows SDK version (10.0.17763.0). This is very brittle and will cause the signing step to fail on any machine or GitHub-hosted runner with a different SDK version installed.
The path should be located dynamically at runtime. You can achieve this by searching common Windows Kits directories. Here is an example of how you could implement this in PowerShell:
$signToolPaths = @(
(Get-ChildItem -Path "${Env:ProgramFiles(x86)}\Windows Kits\10\bin" -Filter "signtool.exe" -Recurse -ErrorAction SilentlyContinue | Select-Object -Last 1),
(Get-ChildItem -Path "${Env:ProgramFiles(x86)}\Windows Kits\11\bin" -Filter "signtool.exe" -Recurse -ErrorAction SilentlyContinue | Select-Object -Last 1)
)
$signToolPath = ($signToolPaths | Where-Object { $_ } | Select-Object -Last 1).FullName
if (-not $signToolPath) {
Write-Error "signtool.exe not found in common Windows SDK locations."
exit 1
}
Write-Host "Found signtool.exe at: $signToolPath"
# ... rest of script ...
& $signToolPath sign ...| - name: Package & release | ||
| uses: ./actions/package | ||
| with: | ||
| files: | | ||
| */bin/* | ||
| package: ${{ inputs.package }} | ||
| build-name: ${{ inputs.build-name }} | ||
| os: ${{ steps.discovery.outputs.OS }} | ||
| arch: ${{ steps.discovery.outputs.ARCH }} | ||
| tag: ${{ steps.discovery.outputs.TAG }} | ||
| is-tag: ${{ steps.discovery.outputs.IS_TAG }} | ||
| short-sha: ${{ steps.discovery.outputs.SHORT_SHA }} | ||
| ref: ${{ steps.discovery.outputs.REF }} |
There was a problem hiding this comment.
This Package & release step appears to be redundant. The dir_orch step at line 53 calls the wails2 wrapper, which already contains its own Package & release step.
Executing the packaging logic twice will result in artifacts being uploaded twice and could cause the GitHub Release creation to fail on the second attempt. The responsibility for packaging should ideally lie within the stack-specific wrapper to keep the pipeline self-contained.
| - name: Discovery | ||
| id: discovery | ||
| uses: ./actions/discovery | ||
| with: | ||
| working-directory: ${{ inputs.app-working-directory }} |
There was a problem hiding this comment.
This Discovery step is redundant when the wails2 wrapper is called from the root action.yml, as the root action already performs a discovery step. This leads to unnecessary work and potential inconsistencies if the discovery logic were to diverge.
Since this action is designed to be callable on its own, a good solution would be to make this step conditional. It should only run if discovery data (like OS, arch, etc.) is not passed in via inputs. This would make the action both self-contained and efficient when used as part of the larger orchestration.
| esac | ||
| fi | ||
| if [[ -n "$norm_tags" ]]; then | ||
| build_options+=" -tags${norm_tags}" |
There was a problem hiding this comment.
The build tags are being added to the options string without quotes. Since norm_tags contains a space-separated list, the shell will interpret each tag as a separate argument to wails build rather than a single, space-delimited string for the -tags flag. This will cause the build to fail or behave unexpectedly.
The tags should be wrapped in single quotes to ensure they are passed as a single argument.
build_options+=" -tags '${norm_tags# }'"| > I help on lots of open source projects, im tired of doing the same thing over and over again.\ | ||
| > so, I'm going to put them all together in one place.\ | ||
| > Hopefully it will help you too. | ||
| General build action (multi-stack). | ||
|
|
||
| By default, the root action will best guess the builds you might want to run and delegate to the appropriate sub-action. You can also explicitly select a stack and enable/disable setup steps. | ||
|
|
||
| you should write out an action that cherry-picks the parts you need; the auto-detected method works for me, based on the file structures in the tdd/* folders |
There was a problem hiding this comment.
The introductory text is a bit informal and contains a sentence that might be confusing for new users. Improving the wording would enhance clarity and professionalism, making the action more approachable.
For example:
Lines 10-12:
- > I help on lots of open source projects, im tired of doing the same thing over and over again.\n- > so, I'm going to put them all together in one place.\n- > Hopefully it will help you too.
+ > I contribute to many open-source projects and found myself repeating the same build configurations. This action consolidates that work into a reusable, multi-stack builder that I hope you find useful too.Line 18:
- you should write out an action that cherry-picks the parts you need; the auto-detected method works for me, based on the file structures in the tdd/* folders
+ For advanced customization, you can call the modular sub-actions directly in your workflow to cherry-pick the exact steps you need. The auto-detection method is designed to work for common project layouts, like those found in the `tdd/*` examples.| description: "" | ||
| sign: | ||
| required: false | ||
| default: "false" | ||
| description: "" | ||
| package: | ||
| required: false | ||
| default: "true" | ||
| description: "" | ||
| build-name: | ||
| required: true | ||
| description: "" | ||
| build-platform: | ||
| required: false | ||
| default: "darwin/universal" | ||
| description: "" | ||
| app-working-directory: | ||
| required: false | ||
| default: "." | ||
| description: "" | ||
| # Orchestrator controls | ||
| AUTO_STACK: | ||
| required: false | ||
| default: "true" | ||
| description: "" | ||
| AUTO_SETUP: | ||
| required: false | ||
| default: "true" | ||
| description: "" | ||
| STACK: | ||
| required: false | ||
| default: "" | ||
| description: "" | ||
| # Optional hint from a prior discovery step | ||
| PRIMARY_STACK_SUGGESTION: | ||
| required: false | ||
| default: "" | ||
| description: "" |
There was a problem hiding this comment.
The descriptions for all inputs in this action are empty. Adding clear descriptions is crucial for maintainability and makes the action much easier for other developers to use and understand without having to read the underlying code.
| required: false | ||
| default: '' | ||
| description: "" | ||
| sign-macos-apple-password: | ||
| required: false | ||
| default: '' | ||
| description: "" | ||
| sign-macos-app-cert: | ||
| required: false | ||
| default: '' | ||
| description: "" | ||
| sign-macos-app-cert-password: | ||
| required: false | ||
| default: '' | ||
| description: "" | ||
| sign-macos-installer-id: | ||
| required: false | ||
| default: '' | ||
| description: "" | ||
| sign-macos-installer-cert: | ||
| required: false | ||
| default: '' | ||
| description: "" | ||
| sign-macos-installer-cert-password: | ||
| required: false | ||
| default: '' | ||
| description: "" | ||
| sign-windows-cert: | ||
| required: false | ||
| default: '' | ||
| description: "" | ||
| sign-windows-cert-password: | ||
| required: false | ||
| default: '' | ||
| description: "" |
There was a problem hiding this comment.
Many of the signing-related inputs have empty descriptions. Given the complexity of code signing, providing clear descriptions for what each input does and what value it expects is essential for a good user experience.
| if [ -f "frontend/package.json" ]; then has_frontend_pkg=1; found_files+=("frontend/package.json"); fi | ||
| # Light subtree search up to depth 2 for package.json and mkdocs.yml | ||
| if command -v find >/dev/null 2>&1; then | ||
| if find . -maxdepth 2 -mindepth 2 -name package.json | grep -q .; then has_sub_npm=1; fi |
There was a problem hiding this comment.
The find command used to detect a nested package.json does not exclude the node_modules directory. This could lead to false positives if a dependency contains its own package.json, potentially causing incorrect stack detection. The PowerShell equivalent in this file correctly handles this by exclusion.
if find . -maxdepth 2 -mindepth 2 -name package.json -not -path "./node_modules/*" | grep -q .; then has_sub_npm=1; fi
This plan outlines the refactoring of the build action to use GoTask
(go-task/task) as the build orchestrator instead of inline shell scripts.
Primary goals:
Key architectural changes:
Also includes v3 branch code snapshot for reference.