-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix -compiler flag for build, dev and generate commands #3121
Fix -compiler flag for build, dev and generate commands #3121
Conversation
WalkthroughThe recent updates to the Wails framework introduce the ability to specify a custom Go compiler for various operations, such as generating bindings and running development commands. This enhancement allows developers to use different versions of the Go compiler, which can be particularly useful when dealing with compatibility issues or specific build requirements. Changes
Assessment against linked issues
Poem
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 X ? TipsChat with CodeRabbit Bot (
|
Thanks for this 🙏 Please could you add an entry to the changelog located at |
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.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (6)
- v2/cmd/wails/flags/generate.go (2 hunks)
- v2/cmd/wails/generate.go (1 hunks)
- v2/cmd/wails/internal/dev/dev.go (1 hunks)
- v2/pkg/commands/bindings/bindings.go (2 hunks)
- v2/pkg/commands/bindings/bindings_test.go (4 hunks)
- v2/pkg/commands/build/build.go (1 hunks)
Additional comments: 15
v2/cmd/wails/flags/generate.go (4)
5-5: The addition of the
Compiler
field to theGenerateModule
struct is consistent with the PR's objective to allow specifying a different Go compiler. This change should be cross-checked with documentation updates and tests to ensure it's properly communicated and verified.17-21: The
Default
method correctly initializes theCompiler
field with "go", which is the default Go compiler. This method will help ensure that there is a consistent default value for theCompiler
field across the codebase.13-21: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [5-21]
Please ensure that the documentation and changelog are updated to reflect the addition of the
Compiler
field and theDefault
method in theGenerateModule
struct. This is crucial for users to understand the new feature and for maintaining the project's documentation integrity.
- 13-21: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [5-21]
Please verify if new tests have been added or existing tests have been updated to cover the new
Compiler
field and theDefault
method. This is important to ensure that the new functionality works as expected and to prevent regressions in the future.v2/cmd/wails/generate.go (1)
- 48-54: The addition of the
Compiler
field to thebindings.Options
struct is consistent with the PR's objective to fix the-compiler
flag functionality. Ensure thatf.Compiler
is properly initialized before this point, and if a default compiler should be used whenf.Compiler
is not specified, verify that there is logic elsewhere in the code to handle this case.v2/cmd/wails/internal/dev/dev.go (2)
63-67: The change dynamically uses the
f.Compiler
variable to run themod tidy
command, which aligns with the PR's objective to respect the-compiler
flag across commands. Ensure thatf.Compiler
is properly validated and sanitized before this point to prevent command injection vulnerabilities.61-67: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-67]
Please ensure that the changelog and documentation are updated to reflect the changes made in this PR, especially since the
-compiler
flag behavior is a user-facing feature.v2/pkg/commands/bindings/bindings.go (2)
18-24: The addition of the
Compiler
field to theOptions
struct is consistent with the PR's objective to handle the-compiler
flag correctly.47-59: The
GenerateBindings
function has been correctly updated to use theoptions.Compiler
value when running the "go mod tidy" and "go build" commands, which ensures that the specified compiler is used.v2/pkg/commands/bindings/bindings_test.go (2)
- 81-87: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [81-110]
The addition of the
Compiler
field to theOptions
struct in the test cases is consistent with the PR's objective to handle the-compiler
flag. Ensure that theCompiler
field is being used correctly within the test cases and that it aligns with the changes made in the rest of the codebase.
- 1-14: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-110]
Verify if new tests have been added or existing tests have been updated to cover the changes introduced by this PR, particularly the new
Compiler
field's functionality.v2/pkg/commands/build/build.go (4)
226-231: The addition of the
Compiler
field to thebindings.Options
struct within theGenerateBindings
function call is consistent with the PR's objective to respect the-compiler
flag. This change ensures that the specified compiler is used during the bindings generation process.226-231: Verify that the
Compiler
field is properly initialized before this call and that theGenerateBindings
function is correctly utilizing this field to invoke the specified compiler.226-231: Ensure that any tests related to the
GenerateBindings
function or thebuild
process are updated to account for the newCompiler
field.226-231: Remind the author to update the changelog and documentation to reflect the changes made in this PR, especially the new
Compiler
field and its impact on the build process.
package bindings | ||
|
||
import ( | ||
"github.com/matryer/is" | ||
"github.com/wailsapp/wails/v2/pkg/templates" | ||
"os" | ||
"path/filepath" | ||
"runtime" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/matryer/is" | ||
"github.com/wailsapp/wails/v2/pkg/templates" | ||
) | ||
|
||
const standardBindings = `// @ts-check |
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.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-110]
Please ensure that the changelog and documentation are updated to reflect the new changes introduced by this PR, especially the handling of the -compiler
flag.
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- website/docs/reference/cli.mdx (1 hunks)
- website/src/pages/changelog.mdx (1 hunks)
Additional comments: 2
website/docs/reference/cli.mdx (1)
- 231-238: The documentation update correctly reflects the PR's objective to fix the
-compiler
flag for thewails generate module
command by adding the-compiler
and-tags
flags to the documentation. This ensures that users are aware of the new functionality and how to use it.website/src/pages/changelog.mdx (1)
- 17-18: The changelog entry correctly documents the fix for the
-compiler
flag as described in the PR objectives. It also provides a link to the PR and credits the contributor, which is a good practice for transparency and recognition.
Sure, thanks for the quick response |
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- v2/cmd/wails/internal/dev/dev.go (1 hunks)
- v2/pkg/commands/bindings/bindings.go (2 hunks)
- v2/pkg/commands/build/build.go (1 hunks)
- website/src/pages/changelog.mdx (1 hunks)
Files skipped from review due to trivial changes (1)
- website/src/pages/changelog.mdx
Additional comments: 3
v2/cmd/wails/internal/dev/dev.go (1)
- 63-67: The change to use
f.Compiler
instead of a hard-coded "go" command aligns with the PR's objective to handle the-compiler
flag correctly. Ensure thatf.Compiler
is properly initialized before this call, and consider adding error handling iff.Compiler
is not set or invalid.v2/pkg/commands/bindings/bindings.go (1)
- 18-24: The addition of the
Compiler
field to theOptions
struct is consistent with the PR's objective to fix the-compiler
flag functionality.v2/pkg/commands/build/build.go (1)
- 231-235: The addition of the
Compiler
field in thebindings.Options
struct within theGenerateBindings
function correctly implements the PR's objective to respect the-compiler
flag. Ensure that theCompiler
value is properly validated and sanitized before use to prevent command injection vulnerabilities, especially since it will be used in shell commands.
tagString := buildtags.Stringify(genModuleTags) | ||
|
||
if options.GoModTidy { | ||
stdout, stderr, err = shell.RunCommand(workingDirectory, "go", "mod", "tidy") | ||
stdout, stderr, err = shell.RunCommand(workingDirectory, options.Compiler, "mod", "tidy") | ||
if err != nil { | ||
return stdout, fmt.Errorf("%s\n%s\n%s", stdout, stderr, err) | ||
} | ||
} | ||
|
||
stdout, stderr, err = shell.RunCommand(workingDirectory, "go", "build", "-tags", tagString, "-o", filename) | ||
stdout, stderr, err = shell.RunCommand(workingDirectory, options.Compiler, "build", "-tags", tagString, "-o", filename) | ||
if err != nil { | ||
return stdout, fmt.Errorf("%s\n%s\n%s", stdout, stderr, err) | ||
} |
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 shell.RunCommandWithEnv
function call at the end of the GenerateBindings
function should also respect the options.Compiler
value if it is intended to run a Go-related process.
- stdout, stderr, err = shell.RunCommandWithEnv(env, workingDirectory, filename)
+ stdout, stderr, err = shell.RunCommandWithEnv(env, workingDirectory, options.Compiler, "run", filename)
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.
tagString := buildtags.Stringify(genModuleTags) | |
if options.GoModTidy { | |
stdout, stderr, err = shell.RunCommand(workingDirectory, "go", "mod", "tidy") | |
stdout, stderr, err = shell.RunCommand(workingDirectory, options.Compiler, "mod", "tidy") | |
if err != nil { | |
return stdout, fmt.Errorf("%s\n%s\n%s", stdout, stderr, err) | |
} | |
} | |
stdout, stderr, err = shell.RunCommand(workingDirectory, "go", "build", "-tags", tagString, "-o", filename) | |
stdout, stderr, err = shell.RunCommand(workingDirectory, options.Compiler, "build", "-tags", tagString, "-o", filename) | |
if err != nil { | |
return stdout, fmt.Errorf("%s\n%s\n%s", stdout, stderr, err) | |
} | |
tagString := buildtags.Stringify(genModuleTags) | |
if options.GoModTidy { | |
stdout, stderr, err = shell.RunCommand(workingDirectory, options.Compiler, "mod", "tidy") | |
if err != nil { | |
return stdout, fmt.Errorf("%s\n%s\n%s", stdout, stderr, err) | |
} | |
} | |
stdout, stderr, err = shell.RunCommandWithEnv(env, workingDirectory, options.Compiler, "run", filename) | |
if err != nil { | |
return stdout, fmt.Errorf("%s\n%s\n%s", stdout, stderr, err) | |
} |
@xtrafrancyz - thank for this! |
Description
Fixes #3120
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tested
wails build
,wails dev
,wails generate module
with and without-compiler
flag.Test Configuration
Checklist:
website/src/pages/changelog.mdx
with details of this PRSummary by CodeRabbit
New Features
Refactor
Tests
Options
struct.Documentation
Compiler
field in theGenerateModule
struct.