fix(windows): hold mode with RetroBat and relative config paths#674
Merged
wizzomafizzo merged 4 commits intomainfrom Apr 15, 2026
Merged
fix(windows): hold mode with RetroBat and relative config paths#674wizzomafizzo merged 4 commits intomainfrom
wizzomafizzo merged 4 commits intomainfrom
Conversation
…d mode RetroBat launchers use the EmulationStation HTTP API instead of direct process tracking, returning nil for the process handle. When hold mode triggered StopActiveLauncher, only the nil trackedProcess was checked and the game kept running. Add lastLauncher tracking to the Windows platform (matching the MiSTer pattern) so StopActiveLauncher calls the launcher's custom Kill function when one is defined. Closes #642 (hold mode)
…rectory Resolve relative paths in media_dir, index_root, and custom launcher media_dirs relative to the executable directory instead of the process CWD. Also set cmd.Dir for custom launcher execute commands so relative paths in commands resolve predictably. Extract path resolution into pkg/helpers/pathutil to avoid circular imports between config and helpers. Closes #642 (relative paths)
📝 WalkthroughWalkthroughAdds path resolution utilities and applies them to launcher-related configuration and helpers to resolve relative paths against the executable directory. Separately, Windows platform launcher shutdown now records the last-used launcher and invokes a custom Changes
Sequence Diagram(s)sequenceDiagram
participant Platform
participant LastLauncher
participant CustomKill
participant OSProcess
Note over Platform,LastLauncher: StopActiveLauncher invoked
Platform->>LastLauncher: check lastLauncher
alt lastLauncher.Kill exists
Platform->>LastLauncher: clear lastLauncher
Platform->>CustomKill: invoke Kill(callback) (unlocked)
CustomKill-->>Platform: return (may be error)
Platform->>Platform: set trackedProcess = nil
else no custom Kill
Platform->>OSProcess: if trackedProcess != nil -> kill process
OSProcess-->>Platform: process terminated
Platform->>Platform: clear trackedProcess
end
Platform-->>Platform: return success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
pkg/helpers/launchers_test.go (1)
249-275: Add a regression case for relativeExecutepaths too.This update only checks that
MediaDirsare resolved againstExeDir(). The PR also changes custom launcher execution to run withcmd.Dir = ExeDir(), and that behavior is still untested here. A case with non-emptyExecuteand a relative path would make the Windows path fix much safer.As per coding guidelines,
**/*.go: Write tests for all new code — see TESTING.md and pkg/testing/README.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/launchers_test.go` around lines 249 - 275, Add a new regression test alongside TestParseCustomLaunchers_EmptyExecuteLeavesLaunchNil that supplies a LaunchersCustom with a non-empty relative Execute (e.g., "tools/runner.exe") and relative MediaDirs, call ParseCustomLaunchers, then assert that the resulting launcher has the Execute path resolved against ExeDir() and that the created launch function will run with working directory set to ExeDir() (i.e., exercise l.Launch to capture the command's Dir or otherwise inspect the parsed command.Dir if ParseCustomLaunchers returns it); use the same mocks (mocks.NewMockPlatform / mockPlatform.ID) and reference ParseCustomLaunchers, LaunchersCustom, Execute and ExeDir() in the assertions to ensure the relative Execute behavior is covered.pkg/helpers/pathutil/pathutil.go (1)
32-50: Propagate executable path lookup failure instead of silently degrading to CWD behavior.Line 35 and Line 48 silently fall back to
""/ unchanged relative paths. Downstream, this can make command execution and config-path behavior CWD-dependent again (for example wherecmd.Dir = ExeDir()is used). Consider returning an error (or a(string, bool)success flag) so callers can fail fast or log and explicitly choose fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/pathutil/pathutil.go` around lines 32 - 50, Change ExeDir and ResolveRelativePath to propagate executable lookup failures instead of returning empty/unchanged values: modify ExeDir to return (string, error) and return the os.Executable error when os.Executable() fails, and modify ResolveRelativePath to return (string, error) and call the new ExeDir, returning an error if ExeDir fails; keep the existing behavior of returning the input path unchanged only when it is absolute or empty but return nil error in that case, and update all callers to handle the new (string, error) signature (or convert the error to an explicit fallback decision where appropriate).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/configlaunchers_test.go`:
- Around line 689-699: The test uses a hardcoded "/absolute/path" which is not
recognized as absolute on Windows, causing ResolveRelativePath()/IndexRoots()
assertions to fail; update the fixture to generate a real OS-appropriate
absolute path (e.g., use t.TempDir() or filepath.Abs on a created temp dir) and
replace the hardcoded string in the test so roots[1] compares against that
generated abs path (keep assertions referencing IndexRoots(),
ResolveRelativePath(), and ExeDir() behavior unchanged).
In `@pkg/helpers/paths_test.go`:
- Around line 878-881: The test case uses a Unix-only literal "/usr/share/games"
so filepath.IsAbs returns false on Windows; update the test to create a runtime
absolute path via t.TempDir() (or os.MkdirTemp) and use that value for both the
path and expected fields so ResolveRelativePath sees an absolute path
cross-platform; locate the test case for ResolveRelativePath and replace the
hardcoded path/expected with a call to t.TempDir() (or a temp directory
variable) to ensure the assertion is platform independent.
In `@pkg/helpers/pathutil/pathutil_test.go`:
- Around line 33-35: The tests currently skip when ExeDir() returns empty
instead of exercising the fallback logic; add a deterministic seam so you can
simulate os.Executable failing and assert the fallback behavior. Modify the test
for ExeDir (in pathutil_test.go) to inject or override the function ExeDir uses
to obtain the executable path (e.g., introduce a package-level variable or
constructor that points to os.Executable) so the test can replace it with a stub
that returns an error/empty string, then assert the fallback path is
returned/used instead of calling t.Skip; apply the same pattern to the other
skipped case to explicitly validate the fallback branch.
- Around line 48-50: Replace the hardcoded Unix absolute path in the table test
case named "absolute path unchanged" with a platform-native absolute directory
from t.TempDir(); i.e., in the test that asserts filepath.IsAbs behavior (look
for the table entry with name "absolute path unchanged" and code that relies on
filepath.IsAbs()), set path := t.TempDir() (or assign expected := t.TempDir()
and use that value for both path and expected) so the test uses an OS-native
absolute path on Windows and Unix.
In `@pkg/platforms/windows/platform.go`:
- Around line 193-209: The customKill branch currently unlocks processMu without
clearing launcher state, so stale lastLauncher/trackedProcess can be invoked
again; inside the StopActiveLauncher flow (the code using p.lastLauncher,
p.trackedProcess and p.processMu), set p.lastLauncher = nil and p.trackedProcess
= nil while still holding p.processMu (and any other stop-state like
setActiveMedia(nil) if relevant), then unlock and call
customKill(&config.Instance{}); this ensures the stop is idempotent and no stale
Kill callback is invoked on subsequent calls.
---
Nitpick comments:
In `@pkg/helpers/launchers_test.go`:
- Around line 249-275: Add a new regression test alongside
TestParseCustomLaunchers_EmptyExecuteLeavesLaunchNil that supplies a
LaunchersCustom with a non-empty relative Execute (e.g., "tools/runner.exe") and
relative MediaDirs, call ParseCustomLaunchers, then assert that the resulting
launcher has the Execute path resolved against ExeDir() and that the created
launch function will run with working directory set to ExeDir() (i.e., exercise
l.Launch to capture the command's Dir or otherwise inspect the parsed
command.Dir if ParseCustomLaunchers returns it); use the same mocks
(mocks.NewMockPlatform / mockPlatform.ID) and reference ParseCustomLaunchers,
LaunchersCustom, Execute and ExeDir() in the assertions to ensure the relative
Execute behavior is covered.
In `@pkg/helpers/pathutil/pathutil.go`:
- Around line 32-50: Change ExeDir and ResolveRelativePath to propagate
executable lookup failures instead of returning empty/unchanged values: modify
ExeDir to return (string, error) and return the os.Executable error when
os.Executable() fails, and modify ResolveRelativePath to return (string, error)
and call the new ExeDir, returning an error if ExeDir fails; keep the existing
behavior of returning the input path unchanged only when it is absolute or empty
but return nil error in that case, and update all callers to handle the new
(string, error) signature (or convert the error to an explicit fallback decision
where appropriate).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 254987de-2443-4500-90c2-c3031eba4a6f
📒 Files selected for processing (10)
pkg/config/configlaunchers.gopkg/config/configlaunchers_test.gopkg/helpers/launchers.gopkg/helpers/launchers_test.gopkg/helpers/paths.gopkg/helpers/paths_test.gopkg/helpers/pathutil/pathutil.gopkg/helpers/pathutil/pathutil_test.gopkg/platforms/windows/platform.gopkg/platforms/windows/platform_test.go
Comment on lines
+878
to
+881
| name: "absolute path unchanged", | ||
| path: "/usr/share/games", | ||
| expected: "/usr/share/games", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the test file
fd -t f "paths_test.go" --exec head -n 910 {} \;Repository: ZaparooProject/zaparoo-core
Length of output: 22206
🏁 Script executed:
# Get the test file content around lines 878-881
fd -t f "paths_test.go" -x cat -n {} | sed -n '850,910p'Repository: ZaparooProject/zaparoo-core
Length of output: 1711
🏁 Script executed:
# Find the ResolveRelativePath implementation
fd -t f "paths.go" pkg/helpers/ -x cat -n {}Repository: ZaparooProject/zaparoo-core
Length of output: 22071
🏁 Script executed:
# Quick verification of filepath.IsAbs behavior on Unix-like paths
python3 << 'EOF'
import os
import sys
# Simulate what filepath.IsAbs does in Go
# On Windows, only paths starting with drive letters or \\ are absolute
# On Unix, paths starting with / are absolute
test_path = "/usr/share/games"
# Simulate Unix behavior
unix_is_abs = test_path.startswith("/")
print(f"Unix filepath.IsAbs('{test_path}'): {unix_is_abs}")
# Simulate Windows behavior
windows_is_abs = (len(test_path) > 1 and test_path[1] == ':') or test_path.startswith("\\\\")
print(f"Windows filepath.IsAbs('{test_path}'): {windows_is_abs}")
print(f"\nConclusion: Path '{test_path}' is portable-unsafe - different IsAbs() result on Windows vs Unix")
EOFRepository: ZaparooProject/zaparoo-core
Length of output: 268
Use a runtime absolute path instead of a Unix-specific literal.
"/usr/share/games" is only guaranteed absolute on Unix. On Windows, filepath.IsAbs("/usr/share/games") returns false, so ResolveRelativePath treats it as relative and prepends ExeDir(), causing the test to fail. Use t.TempDir() to generate a platform-specific absolute path for consistent cross-platform test behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/helpers/paths_test.go` around lines 878 - 881, The test case uses a
Unix-only literal "/usr/share/games" so filepath.IsAbs returns false on Windows;
update the test to create a runtime absolute path via t.TempDir() (or
os.MkdirTemp) and use that value for both the path and expected fields so
ResolveRelativePath sees an absolute path cross-platform; locate the test case
for ResolveRelativePath and replace the hardcoded path/expected with a call to
t.TempDir() (or a temp directory variable) to ensure the assertion is platform
independent.
Comment on lines
+33
to
+35
| if exeDir == "" { | ||
| t.Skip("ExeDir() returned empty, cannot test relative path resolution") | ||
| } |
There was a problem hiding this comment.
Fallback branches are skipped rather than validated.
Line 33 and Line 82 skip when ExeDir() is empty, but the new fallback behavior for that condition is part of the new production logic and remains unverified. Please add a deterministic test seam so os.Executable failure / empty ExeDir paths are explicitly asserted.
As per coding guidelines "Write tests for all new code — see TESTING.md and pkg/testing/README.md".
Also applies to: 82-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/helpers/pathutil/pathutil_test.go` around lines 33 - 35, The tests
currently skip when ExeDir() returns empty instead of exercising the fallback
logic; add a deterministic seam so you can simulate os.Executable failing and
assert the fallback behavior. Modify the test for ExeDir (in pathutil_test.go)
to inject or override the function ExeDir uses to obtain the executable path
(e.g., introduce a package-level variable or constructor that points to
os.Executable) so the test can replace it with a stub that returns an
error/empty string, then assert the fallback path is returned/used instead of
calling t.Skip; apply the same pattern to the other skipped case to explicitly
validate the fallback branch.
Fix fieldalignment in Windows Platform struct, use CommandContext in test, and add --fix to all cross-lint task definitions so auto-fixable issues are resolved in-place.
Use platform-native absolute paths in tests instead of hardcoded Unix paths. Clear lastLauncher in StopActiveLauncher for idempotency. Remove Linux target from cross-lint (native lint already covers it, zig cc can't resolve system C deps like GTK/X11/pcsclite).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/platforms/windows/platform.go`:
- Around line 193-200: The code calls the launcher Kill function with an empty
config instance (customKill(&config.Instance{})) which violates the interface;
update the Platform to store the active runtime config (e.g., add a field like
config *config.Instance on the Platform struct when created/initialized) and
pass that stored config into p.lastLauncher.Kill instead of a newly allocated
empty struct. Locate references to p.lastLauncher.Kill / customKill and replace
the empty-instance call with customKill(p.config) (ensuring p.config is set
where Platform is constructed and protected by p.processMu as needed), keeping
the existing trackedProcess niling and unlocking behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c9d5f3d-6161-4e62-b24e-e43c7b770e30
📒 Files selected for processing (6)
pkg/config/configlaunchers_test.gopkg/helpers/paths_test.gopkg/helpers/pathutil/pathutil_test.gopkg/platforms/windows/platform.gopkg/platforms/windows/platform_test.goscripts/tasks/cross-lint.yml
✅ Files skipped from review due to trivial changes (2)
- pkg/helpers/paths_test.go
- pkg/helpers/pathutil/pathutil_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/config/configlaunchers_test.go
- pkg/platforms/windows/platform_test.go
Comment on lines
+193
to
+200
| customKill := p.lastLauncher.Kill | ||
| p.lastLauncher = platforms.Launcher{} | ||
|
|
||
| if customKill != nil { | ||
| p.trackedProcess = nil | ||
| log.Debug().Msg("killed tracked process") | ||
| p.processMu.Unlock() | ||
| log.Debug().Msg("using custom Kill function for launcher") | ||
| if err := customKill(&config.Instance{}); err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Current Windows call site:"
sed -n '190,205p' pkg/platforms/windows/platform.go
echo
echo "Kill implementations that accept *config.Instance:"
python <<'PY'
from pathlib import Path
import re
pattern = re.compile(r'Kill:\s*func\(\s*(\w+)\s+\*config\.Instance[^)]*\)\s*error\s*\{', re.M)
for path in Path(".").rglob("*.go"):
text = path.read_text(encoding="utf-8")
for match in pattern.finditer(text):
var = match.group(1)
brace = text.find("{", match.end() - 1)
depth = 0
end = brace
while end < len(text):
ch = text[end]
if ch == "{":
depth += 1
elif ch == "}":
depth -= 1
if depth == 0:
end += 1
break
end += 1
body = text[brace:end]
uses_cfg = bool(re.search(rf"\b{re.escape(var)}\.", body))
line = text.count("\n", 0, match.start()) + 1
print(f"{path}:{line}: uses_cfg={uses_cfg}")
PYRepository: ZaparooProject/zaparoo-core
Length of output: 860
🏁 Script executed:
cat pkg/platforms/platforms.go | grep -A 20 "type Launcher struct"Repository: ZaparooProject/zaparoo-core
Length of output: 1449
🏁 Script executed:
rg "Kill:\s*func" --type go -A 5 -B 2Repository: ZaparooProject/zaparoo-core
Length of output: 2201
Pass the real runtime config into launcher.Kill.
The Launcher.Kill interface accepts a *config.Instance parameter, indicating implementations may need access to the active configuration. Line 200 calls customKill(&config.Instance{}) with an empty instance instead of the actual runtime config. While current implementations all ignore this parameter, passing an empty instance violates the interface contract and blocks future implementations from accessing launcher settings or resolving paths from config.
Store the config in the Platform struct and pass it here:
Suggested fix
type Platform struct {
+ cfg *config.Instance
activeMedia func() *models.ActiveMedia
setActiveMedia func(*models.ActiveMedia)
customPlatformToSystem map[string]string
systemToCustomPlatforms map[string][]string
trackedProcess *os.Process
launchBoxPipe *LaunchBoxPipeServer
steamTracker *steamtracker.WindowsPlatformIntegration
lastLauncher platforms.Launcher
processMu syncutil.RWMutex
platformMappingsMu syncutil.RWMutex
launchBoxPipeLock syncutil.Mutex
}
func (p *Platform) StartPost(
cfg *config.Instance,
_ platforms.LauncherContextManager,
activeMedia func() *models.ActiveMedia,
setActiveMedia func(*models.ActiveMedia),
_ *database.Database,
) error {
+ p.cfg = cfg
p.activeMedia = activeMedia
p.setActiveMedia = setActiveMedia
...
}
...
- if err := customKill(&config.Instance{}); err != nil {
+ if err := customKill(p.cfg); err != nil {
log.Warn().Err(err).Msg("custom Kill function failed")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/platforms/windows/platform.go` around lines 193 - 200, The code calls the
launcher Kill function with an empty config instance
(customKill(&config.Instance{})) which violates the interface; update the
Platform to store the active runtime config (e.g., add a field like config
*config.Instance on the Platform struct when created/initialized) and pass that
stored config into p.lastLauncher.Kill instead of a newly allocated empty
struct. Locate references to p.lastLauncher.Kill / customKill and replace the
empty-instance call with customKill(p.config) (ensuring p.config is set where
Platform is constructed and protected by p.processMu as needed), keeping the
existing trackedProcess niling and unlocking behavior intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lastLaunchertracking toStopActiveLauncher, matching the MiSTer platform patternmedia_dir,index_root, and custom launchermedia_dirsconfig paths relative to the executable directory instead of the unpredictable process CWDcmd.Dirfor custom launcherexecutecommands so relative paths resolve relative to the executableExeDirandResolveRelativePathintopkg/helpers/pathutilto avoid circular imports between config and helpersCloses #642
Summary by CodeRabbit
Bug Fixes
New Features