Added possibility to specify platform for dev command #3117
Conversation
WalkthroughThe recent updates to the Wails framework focus on refining build target handling. The code has been restructured to simplify loops and directly use target properties, removing dependencies on specific OS and runtime packages. A new Changes
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 (
|
|
Instant hot reload on WSL. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (6)
- v2/cmd/wails/build.go (5 hunks)
- v2/cmd/wails/flags/build.go (4 hunks)
- v2/cmd/wails/flags/common.go (1 hunks)
- v2/cmd/wails/flags/dev.go (4 hunks)
- v2/cmd/wails/internal/dev/dev.go (2 hunks)
- v2/pkg/commands/build/build.go (1 hunks)
Additional comments: 20
v2/cmd/wails/build.go (8)
108-113: The addition of a conditional check for the
OutputFilelength and the number of targets is a good practice to avoid unnecessary output when multiple targets are specified. This change seems to align with the PR's objective of enhancing the flexibility of the development environment setup.146-152: The new loop over targets with a check against a list of valid platform/architecture combinations is a good way to ensure that only supported targets are built. However, it would be beneficial to ensure that the list of valid platforms is comprehensive and includes all platforms that the application intends to support.
161-162: Setting the
PlatformandArchdirectly from the target properties simplifies the build logic and makes it more maintainable. This change is consistent with the PR's goal of improving the development command's flexibility.167-169: The warning about the compress flag being unsupported for universal binaries is a good addition for user feedback. However, it's important to ensure that this warning is only displayed when necessary and does not affect other platforms.
173-181: The continuation of the loop when cross-compiling is not supported for Linux and Mac is a good safeguard. However, it would be beneficial to ensure that this does not prevent users from cross-compiling if they have the necessary setup to do so.
183-185: The logic for setting the
BundleNamebased on the number of Mac targets is a good addition. It's important to ensure that this logic is only applied when necessary and does not interfere with other platform builds.188-190: The logic for setting the target filename based on the number of targets is a good addition. It's important to ensure that this logic correctly handles all possible combinations of platforms and architectures.
212-229: The addition of a dry run option is a useful feature for users to test the build process without actually compiling the binary. It's important to ensure that this option is well-documented and that users are aware of its existence.
v2/cmd/wails/flags/build.go (3)
46-60: The
Defaultmethod has been refactored to use a newdefaultTargetfunction, and thePlatformfield is set based on the target. Ensure that thedefaultTargetfunction is defined elsewhere in the codebase and that it provides a sensible default for the platform.74-76: The
GetTargetsmethod now returns aTargetsCollectiontype. Verify that theTargetsCollectiontype and theparseTargetsfunction are properly defined and implemented to handle the parsing of the platform string into a collection of targets.2-11: The removal of the "os" and "runtime" imports is consistent with the PR's objectives, as the logic for default platform and architecture has been refactored. However, ensure that this does not affect other parts of the code that may rely on these imports.
v2/cmd/wails/flags/common.go (1)
- 68-69: Verify if the order of platforms is significant for the build process. If so, deduplication might alter the intended order, which could lead to unexpected behavior.
v2/cmd/wails/flags/dev.go (3)
18-22: The addition of the "Platform" field to the
Devstruct with the corresponding flag and description aligns with the PR's objective to enhance platform specification for the development environment. This is a well-documented and clear change.40-50: The
Defaultmethod now initializes the "Platform" field with a value from thedefaultTargetfunction. Ensure that thedefaultTargetfunction accurately determines the default platform across different environments, especially since the feature has not been tested on Windows or macOS.120-133: The
GenerateBuildOptionsmethod now uses the "Platform" field to set architecture and platform options in thebuild.Optionsstruct. This is a key part of the new feature, allowing the development command to target the specified platform. It's important to ensure that theparseTargetsfunction correctly interprets the "Platform" field and that the first target in the resulting slice is the desired default.v2/cmd/wails/internal/dev/dev.go (3)
271-279: The implementation of
isWslfunction looks correct and should work as intended to detect if the application is running under WSL by checking the contents of/proc/version.319-323: The logic to set WSL-specific environment variables when building for Windows under WSL seems correct. It ensures that the necessary variables are passed through to the Windows environment.
268-283: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [271-323]
Please verify that the new WSL detection and environment variable setting works as expected on Windows, particularly under WSL, to ensure cross-platform compatibility.
v2/pkg/commands/build/build.go (2)
74-76: The implementation of
IsWindowsTargetPlatformlooks correct and follows the PR's objective to enhance platform specification. It's a simple string check, which should be sufficient for the intended use case.71-80: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1-12]
The AI-generated summary mentions the removal of "os" and "runtime" imports in another file (
v2/cmd/wails/flags/build.go), but they are still present in this file. If these imports are no longer needed here due to refactoring, they should be removed to keep the code clean.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- website/src/pages/changelog.mdx (1 hunks)
Additional comments: 2
website/src/pages/changelog.mdx (2)
17-18: The changelog has been updated correctly to reflect the addition of the new feature allowing users to specify the platform for the dev command.
15-18: The PR checklist may need to be updated to reflect that the changelog has indeed been updated with the new feature.
|
added extra platform validation |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- v2/cmd/wails/flags/common.go (1 hunks)
Additional comments: 4
v2/cmd/wails/flags/common.go (4)
3-12: The addition of new imports is necessary for the functionality being introduced. Ensure that these imports are used in the file and are not unnecessarily increasing the binary size or introducing unused dependencies.
23-40: The
defaultTargetfunction correctly uses environment variables to set the default platform and architecture, with a fallback to system defaults. This is a good practice for flexibility and configurability.43-55: The
MacTargetsCountmethod is a straightforward way to count the number of Mac targets. Ensure that the prefix check for "darwin" is consistent with how platforms are identified throughout the rest of the codebase.57-63: The
Stringmethod forTargetis implemented correctly and provides a clear string representation of the struct.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@leaanthony that commit causes issues further up and breaks other things and requires function changes 3+ levels up, so it made more sense to quit at the base level where the check occurs, was much simpler. |
/What are the issues? It should bubble back to the top and exit cleanly. |
|
@leaanthony you gonna also have to update GetTargets and GenerateBuildOptions if you want to return error the way AI suggested btw. so I opted against it ;p these will be broken with that change. targets := parseTargets(d.Platform) func (b *Build) GetTargets() TargetsCollection { |
|
Oh I see. Revert the commit then. |
|
@leaanthony Done. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- v2/cmd/wails/flags/common.go (1 hunks)
Additional comments: 1
v2/cmd/wails/flags/common.go (1)
- 1-103: Recommend testing the new feature on all supported platforms (Windows, macOS, and Linux) to ensure compatibility and correct behavior across different environments.
|
Bump, anything you want to see on this one? |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- v2/pkg/commands/build/build.go (1 hunks)
- website/src/pages/changelog.mdx (2 hunks)
Additional comments: 7
v2/pkg/commands/build/build.go (2)
74-76: The implementation of
IsWindowsTargetPlatformmethod is correct and follows the PR's objective to allow platform-specific functionality.74-76: Verify that the changelog and documentation have been updated to include information about the new
IsWindowsTargetPlatformmethod.website/src/pages/changelog.mdx (5)
18-18: The entry for the addition of the platform specification feature for the dev command is correctly documented and attributed to the contributor @pylotlight and PR #3117. This aligns with the PR objectives and AI-generated summaries.
19-19: The entry for the addition of Windows options support, specifically the
DisablePinchZoomconfiguration, is correctly documented and attributed to @tuuzed and PR 3115. This aligns with the AI-generated summaries but is not mentioned in the PR objectives provided by the user. It's possible that this change is part of another PR not covered by the current review.20-20: The entry for the addition of Apple Silicon hardware detection to
wails doctoris correctly documented and attributed to @almas1992 and PR 3129. This aligns with the AI-generated summaries but is not mentioned in the PR objectives provided by the user. It's possible that this change is part of another PR not covered by the current review.21-21: The entry for the removal of the quarantine attribute on macOS binaries is correctly documented and attributed to @leaanthony and PR 3118. This aligns with the AI-generated summaries but is not mentioned in the PR objectives provided by the user. It's possible that this change is part of another PR not covered by the current review.
25-25: The entry for the documentation update for
IsZoomControlEnabledandZoomFactoris correctly documented and attributed to @leaanthony and PR 3137. This aligns with the AI-generated summaries but is not mentioned in the PR objectives provided by the user. It's possible that this change is part of another PR not covered by the current review.
|
Nah, it all looks good. Cheers @PylotLight 👍 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| "darwin": true, | ||
| } | ||
|
|
||
| if !allowedPlatforms[platform] { |
There was a problem hiding this comment.
This checks breaks the ability to cross compile to another platform with a specific architecture (e.g. --platform windows/arm64 or --platform darwin/universal), because the argument platform is the arg cli --platform arg.
It also prevents the ability to compile for multiple platforms --platform windows/am64,windows/arm64.
There was a problem hiding this comment.
PR to resolve this one:
#3245
| @@ -219,32 +207,27 @@ func buildApplication(f *flags.Build) error { | |||
| pterm.Warning.Println("obfuscated flag overrides skipbindings flag.") | |||
| buildOptions.SkipBindings = false | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Closing the loop already here results in only calling the compilation compiledBinary, err := build.Build(buildOptions) for the last target in the targets list. Therefore multi platform builds now only build the last platform. This e.g. is preventing users from building a NSIS installer that contains a windows arm64 and amd64 build wails build --nsis --platform windows/arm64,windows/amd64.
There was a problem hiding this comment.
Ah moved a bracket which should fix this one.
|
Sorry to be late to this PR but during testing of the latest master commit, I came across some issues when working with the |
Ah time to take a stab at fixing it, despite this not being my PR to be being with :P |
This reverts commit 4b4fcdd.
|
I've reverted this PR on master |
Why? I already have a fix for it. |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
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
Refactor
New Features
Targettype andTargetsCollectionfor better build target management.Documentation
Bug Fixes
Chores
Style
NoColourfield to enhance output customization.