fix: support wrapper flag (for proton, mainly)#11
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for custom wrapper commands (primarily for Proton compatibility) that can wrap game executables during launch. The wrapper flag provides an alternative to Wine for running Windows games on Linux/macOS.
- Adds
--wrapperCLI flag to specify custom wrapper commands with arguments (e.g.,proton run) - Implements wrapper command parsing and execution logic that takes precedence over Wine
- Includes four test cases covering basic wrapper usage, argument passing, Wine override behavior, and subcommand support
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| launch/launch.go | Adds Wrapper field to Options struct and implements launchWithWrapper() function to execute games through custom wrapper commands |
| cmd_launch.go | Adds --wrapper CLI flag with updated help text explaining wrapper usage and its relationship to Wine |
| launch/launch_test.go | Adds comprehensive test coverage for wrapper functionality including basic usage, argument passing, Wine override, and subcommand scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if runtime.GOOS == "windows" { | ||
| wrapperName = "proton.bat" | ||
| // Check if first arg is "run" | ||
| wrapperScript = "@echo off\nif \"%1\" == \"run\" (shift & call %*) else exit /b 1" |
There was a problem hiding this comment.
The Windows batch script has a potential issue. After shift, using %* still includes all original arguments. In Windows batch, %* is not updated by shift.
Consider using a different approach for Windows, such as:
@echo off
if "%1" == "run" (
shift
%2 %3 %4 %5 %6 %7 %8 %9
) else (
exit /b 1
)Or for a more robust solution, use a different testing approach that doesn't rely on this behavior, since this test may not accurately verify the wrapper functionality on Windows.
| wrapperScript = "@echo off\nif \"%1\" == \"run\" (shift & call %*) else exit /b 1" | |
| wrapperScript = "@echo off\nif \"%1\" == \"run\" (\n shift\n call %2 %3 %4 %5 %6 %7 %8 %9\n) else (\n exit /b 1\n)" |
|
|
||
| func launchWithWrapper(ctx context.Context, executablePath string, args []string, opts *Options) error { | ||
| // Split wrapper to support commands with arguments (e.g., "proton run") | ||
| wrapperParts := strings.Fields(opts.Wrapper) |
There was a problem hiding this comment.
Using strings.Fields() to parse the wrapper command may not handle quoted arguments correctly. For example, if a user needs to pass a path with spaces like "/path/to/my wrapper" run, strings.Fields() will split on all whitespace, breaking the path.
Consider using a shell-aware parser like shlex or documenting this limitation explicitly. Alternatively, you could use shellquote or similar library to properly parse shell-like command strings with quoted arguments.
| @@ -178,6 +184,30 @@ func Game(ctx context.Context, executablePath string, buildOS auth.BuildOS, args | |||
| return launchNative(ctx, executablePath, args) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The launchWithWrapper function is missing a doc comment. As an exported or important internal function, it should include documentation explaining:
- What the function does (launches a game through a custom wrapper command)
- The expected format of the wrapper string (command with space-separated arguments)
- Any limitations (e.g., quoted paths with spaces are not supported)
| // launchWithWrapper launches the specified game executable using a custom wrapper command. | |
| // The wrapper string (opts.Wrapper) should be a space-separated command and arguments, | |
| // e.g., "proton run" or "customwrapper --flag". The wrapper and its arguments are split | |
| // using strings.Fields, so quoted paths with spaces are NOT supported. | |
| // The resulting command is: [wrapper] [wrapper args...] [executablePath] [game args...]. |
| cmd.Stderr = os.Stderr | ||
| cmd.Stdin = os.Stdin | ||
|
|
||
| // Pass through Wine-related environment variables if set |
There was a problem hiding this comment.
[nitpick] Consider adding a comment explaining why WINEPREFIX is passed through to wrapper commands. While this makes sense for Proton (which is Wine-based), it may not be relevant for other wrappers. A brief comment would clarify the design decision.
Example: // Pass through Wine-related environment variables if set (useful for Wine-based wrappers like Proton)
| // Pass through Wine-related environment variables if set | |
| // Pass through Wine-related environment variables if set. | |
| // This is useful for Wine-based wrappers like Proton, but may not be relevant for other wrappers. |
| } | ||
| } | ||
|
|
||
| func TestGame_WithWrapper(t *testing.T) { |
There was a problem hiding this comment.
The wrapper functionality lacks test coverage for error cases. Consider adding tests for:
- Invalid/non-existent wrapper path
- Empty wrapper string (to verify the error in line 191 of launch.go)
- Wrapper command that fails to execute
These edge cases are important to validate proper error handling and messaging.
No description provided.