Execute Steam InstallScripts during container setup#1464
Conversation
Install scripts are identified by the EDepotFileFlag.InstallScript flag on files in depot manifests, not from a depot-level metadata key. Rewrite collectScripts to load depot manifests and scan for flagged files. Remove old collectScripts unit tests that relied on the previous metadata-based API; keep utility method tests. Clean up diagnostic debug logging from XServerScreen.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Steam install-script support: data models, a VDF parser with env-var expansion, an executor that discovers scripts from downloaded depots, applies registry changes, generates run-process commands, integrates command chaining into Steam launches, and includes parser/executor tests. ChangesSteam Install-Script Support
Sequence Diagram(s)sequenceDiagram
participant XServerScreen
participant InstallScriptExecutor
participant DepotManifest
participant InstallScriptParser
participant WineRegistry
participant GuestProgram
XServerScreen->>InstallScriptExecutor: collectScripts(steamApp, appInfo, gameDir, installDir, language, appId)
InstallScriptExecutor->>DepotManifest: load depot manifests
DepotManifest-->>InstallScriptExecutor: install-script entries
InstallScriptExecutor->>InstallScriptParser: parse(scriptFile)
InstallScriptParser-->>InstallScriptExecutor: InstallScript (registryActions, runProcessActions)
XServerScreen->>InstallScriptExecutor: applyRegistryKeys(container, scripts, language)
InstallScriptExecutor->>WineRegistry: write string/DWORD values (system.reg / user.reg)
XServerScreen->>InstallScriptExecutor: getRunProcessCommands(container, scripts, screenInfo, is64Bit)
InstallScriptExecutor-->>XServerScreen: RunProcessCommand list
XServerScreen->>GuestProgram: execute chained commands (pre-install, install-script runs, game)
GuestProgram->>InstallScriptExecutor: markRunProcessComplete(hasRunKey)
InstallScriptExecutor->>WineRegistry: write completion dword
sequenceDiagram
participant GameLaunch as Game Launch
participant InstallScript as InstallScript Executor
participant Chain as Command Chain
participant Guest as Guest Program
GameLaunch->>InstallScript: collectScripts & applyRegistryKeys
GameLaunch->>InstallScript: getRunProcessCommands
GameLaunch->>Chain: build unified command chain (preInstall + installScript)
Chain->>Guest: execute step N
Guest-->>Chain: termination callback (exit code)
Chain->>Guest: wineserver -k (between steps)
Chain->>Guest: execute final game executable
Guest-->>GameLaunch: game running
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 3256-3266: The termination callback currently marks chained steps
complete unconditionally (advancing the chain even on non-zero exit or
cancellation); update the ChainedCommand termination logic used when building
chainedPreInstall and chainedInstallScript so it only calls
PreInstallSteps.markStepDone(container, cmd.marker) or
InstallScriptExecutor.markRunProcessComplete(container, cmd.hasRunKey) after
verifying the process exited successfully (exit code == 0) and was not
cancelled; ensure any cancellation or non-zero exit prevents advancing the chain
and propagates failure so retries occur on next launch.
In
`@app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.kt`:
- Around line 121-133: The skip-check uses action.hasRunKey before the fallback
key is computed, so actions without an explicit hasRunKey always return false in
hasAlreadyRun and will rerun; fix by computing effectiveHasRunKey before calling
hasAlreadyRun and pass that same effectiveHasRunKey into both the hasAlreadyRun
check and the RunProcessCommand/markRunProcessComplete flow (i.e., compute
effectiveHasRunKey = action.hasRunKey ?:
"Software\\GameNative\\InstallScript\\${script.sourcePath.hashCode()}\\${action.name}"
up front, use it in hasAlreadyRun(effectiveHasRunKey) and in the
RunProcessCommand constructor), and apply the same change to the other similar
block that constructs commands (the second occurrence near the
wrapAsGuestExecutable/RunProcessCommand usage).
- Around line 139-147: markRunProcessComplete is always editing .wine/system.reg
regardless of the hive in hasRunKey; update it (and the corresponding
hasAlreadyRun reader) to select the correct reg file based on the hive prefix
(e.g., if hasRunKey startsWith "HKCU\\" use the per-user reg file instead of
system.reg), then open WineRegistryEditor on that chosen File before calling
setCreateKeyIfNotExist and setDwordValue (use the same hive-parsing logic you
already use with stripHivePrefix/keyPath/valueName so reads and writes target
the same hive).
- Around line 170-176: stripHivePrefix currently only removes uppercase hive
strings and so leaves lowercase prefixes like "hklm\" intact; update
stripHivePrefix to perform case-insensitive prefix removal (e.g., convert
keyPath to a consistent case for matching or use a case-insensitive regex) and
then return the original casing minus the matched prefix so behavior matches
applyRegistryKeys(ignoreCase = true). Target the internal fun
stripHivePrefix(keyPath: String) and ensure it strips any of "HKLM\\",
"HKEY_LOCAL_MACHINE\\", "HKCU\\", "HKEY_CURRENT_USER\\" regardless of case.
In `@app/src/main/java/app/gamenative/utils/installscript/InstallScriptParser.kt`:
- Around line 133-137: The language override keys are stored using the raw VDF
section name (childName) but looked up with language.lowercase() in
mergeWithLanguage(); change the storage to normalize the key (e.g.,
childName.lowercase()) when inserting into the languageOverrides map inside
InstallScriptParser (where RegistryValues is created) so lookups in
mergeWithLanguage() succeed for variants like "English" or "ENGLISH".
- Around line 60-61: The mapping for "%ROOTDRIVE%" in InstallScriptParser is
currently set to "C" which causes expansions like "%ROOTDRIVE%\foo" => "C\foo";
update the mapping for the "ROOTDRIVE" entry (the map or method in
InstallScriptParser that returns default variables) to use "C:" instead of "C"
so path expansions produce valid Windows paths (e.g., "C:\foo"). Ensure any
other places that synthesize ROOTDRIVE defaults use the same "C:" value.
In
`@app/src/test/java/app/gamenative/utils/installscript/InstallScriptExecutorTest.kt`:
- Around line 51-53: The test currently checks for a key named "FrenchValue"
which is actually a value; change the assertion to verify that the value isn't
present by replacing assertFalse(merged.strings.containsKey("FrenchValue")) with
an assertion that checks values (e.g.,
assertFalse(merged.strings.containsValue("FrenchValue")) or
assertNull/doesNotContain on merged.strings.values) so the test checks for
accidental leakage of the French value from the fallback-language logic; update
the assertion in the InstallScriptExecutorTest that uses merged.strings
accordingly.
🪄 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: 2ae77802-9720-4b5a-9af1-ed3041ed30d5
📒 Files selected for processing (9)
app/src/main/java/app/gamenative/data/DepotInfo.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/KeyValueUtils.ktapp/src/main/java/app/gamenative/utils/PreInstallSteps.ktapp/src/main/java/app/gamenative/utils/installscript/InstallScriptData.ktapp/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.ktapp/src/main/java/app/gamenative/utils/installscript/InstallScriptParser.ktapp/src/test/java/app/gamenative/utils/installscript/InstallScriptExecutorTest.ktapp/src/test/java/app/gamenative/utils/installscript/InstallScriptParserTest.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Compute effectiveHasRunKey before skip-check so fallback-keyed actions don't re-run on every launch - Resolve registry file from hive prefix (HKCU → user.reg) instead of hardcoding system.reg in hasAlreadyRun/markRunProcessComplete - Make stripHivePrefix case-insensitive to match applyRegistryKeys - Lowercase language override keys when parsing to match mergeWithLanguage lookup
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.kt (2)
139-147:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPersist the same threshold you use for
MinimumHasRunValue.Line 199 skips only when the stored DWORD reaches
minimumHasRunValue, but Line 147 always writes1. Any action withMinimumHasRunValue > 1will rerun on the next launch, and this can even downgrade a higher value written by the installer itself back to1.💡 Proposed direction
data class RunProcessCommand( val executable: String, val hasRunKey: String?, + val completionValue: Int, ) ... + val completionValue = maxOf(1, action.minimumHasRunValue) - commands.add(RunProcessCommand(wrapped, effectiveHasRunKey)) + commands.add(RunProcessCommand(wrapped, effectiveHasRunKey, completionValue)) ... - fun markRunProcessComplete(container: Container, hasRunKey: String) { + fun markRunProcessComplete(container: Container, hasRunKey: String, completionValue: Int) { ... - editor.setDwordValue(stripHivePrefix(keyPath), valueName, 1) + editor.setDwordValue(stripHivePrefix(keyPath), valueName, completionValue)The caller that consumes
RunProcessCommandshould passcompletionValuethrough when it marks the action complete.Also applies to: 189-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.kt` around lines 139 - 147, markRunProcessComplete currently always writes a DWORD value of 1 which mismatches the threshold checked elsewhere (MinimumHasRunValue) and can downgrade higher values; change markRunProcessComplete (and the similar write in the other block referenced) to accept or be passed the same completionValue used by RunProcessCommand (or the MinimumHasRunValue) and write that value instead of 1 via WineRegistryEditor.setDwordValue(stripHivePrefix(keyPath), valueName, completionValue) so the persisted registry value matches the threshold used when deciding to skip reruns.
121-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQuote the executable before wrapping it in
cmd /c.Line 127 builds
cmdLinefrom the rawprocessvalue, so expanded paths under%INSTALLDIR%orC:\Program Files...are split on spaces unless the VDF already included quotes. That makes valid install scripts fail to launch.💡 Proposed fix
- val cmdLine = if (action.command.isNotEmpty()) { - "${action.process} ${action.command}" - } else { - action.process - } + val quotedProcess = if ( + action.process.startsWith("\"") && action.process.endsWith("\"") + ) { + action.process + } else { + "\"${action.process}\"" + } + val cmdLine = if (action.command.isNotEmpty()) { + "$quotedProcess ${action.command}" + } else { + quotedProcess + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.kt` around lines 121 - 133, The cmdLine built in InstallScriptExecutor (variable cmdLine used to create RunProcessCommand and passed into wrapAsGuestExecutable) uses action.process raw which splits paths with spaces when wrapped in cmd /c; change construction to quote the executable path if it isn't already quoted (e.g., produce "\"<process>\" <command>" when action.command is present or just "\"<process>\"" otherwise), preserving existing surrounding quotes, then pass that quoted cmdLine into wrapAsGuestExecutable; this ensures executables under %INSTALLDIR% or "C:\Program Files..." are not split when run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.kt`:
- Around line 139-147: markRunProcessComplete currently always writes a DWORD
value of 1 which mismatches the threshold checked elsewhere (MinimumHasRunValue)
and can downgrade higher values; change markRunProcessComplete (and the similar
write in the other block referenced) to accept or be passed the same
completionValue used by RunProcessCommand (or the MinimumHasRunValue) and write
that value instead of 1 via
WineRegistryEditor.setDwordValue(stripHivePrefix(keyPath), valueName,
completionValue) so the persisted registry value matches the threshold used when
deciding to skip reruns.
- Around line 121-133: The cmdLine built in InstallScriptExecutor (variable
cmdLine used to create RunProcessCommand and passed into wrapAsGuestExecutable)
uses action.process raw which splits paths with spaces when wrapped in cmd /c;
change construction to quote the executable path if it isn't already quoted
(e.g., produce "\"<process>\" <command>" when action.command is present or just
"\"<process>\"" otherwise), preserving existing surrounding quotes, then pass
that quoted cmdLine into wrapAsGuestExecutable; this ensures executables under
%INSTALLDIR% or "C:\Program Files..." are not split when run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9af89a6a-f160-41d8-b9a6-1ac0878ad0e0
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.ktapp/src/main/java/app/gamenative/utils/installscript/InstallScriptParser.kt
|
Tip For best results, initiate chat on the files or code changes.
Aligned with the spec ✅
Potential gaps worth considering
Would you like to open a follow-up issue for the exit-code-based completion tracking or the uninstall cleanup behavior? |
|
Tip For best results, initiate chat on the files or code changes.
Regarding my earlier questions — would you like me to open follow-up GitHub issues for:
|
|
Hi @playday3008. Thank you for the PR! Could you give me examples of games this fixes (besides NFS Rivals)? I want to try them out on my end too |
In theory any game that has setup step in installScript.vdf, such as EA and Ubisoft titles, although I saw that we have Ubisoft step separately, this PR may allow to remove that "hacky" step |
|
Also, need to check out: |
…sses Wrapper now captures installer exit code via &&/|| branching to a file before wineserver cleanup. Only marks HasRunKey complete on exit code 0, matching Steam behavior. Non-zero exits log a warning and retry next launch.
2 participants
Description
Executes Steam InstallScript VDF files during container setup. Discovers install scripts from depot manifests via
EDepotFileFlag.InstallScript, parses them with JavaSteam'sKeyValue, then:Supports env var expansion, language-specific registry overrides, OS filtering, and
HasRunKeytracking.Tested with NFS Rivals — EA App installer ran successfully via chained Wine process.
Recording
Screen_Recording_20260521_043724_GameNative_1.mp4
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Run Steam InstallScript VDFs during container setup to auto-install prerequisites before game launch. Writes registry keys pre-Wine and chains installers via Wine, using
HasRunKeyand installer exit codes to avoid re-running.New Features
EDepotFileFlag.InstallScript.KeyValue: env var expansion, language overrides, OS filters; defaultHasRunKeywhen missing.system.reg/user.regbefore starting Wine.XServerScreenalongsidePreInstallSteps.InstallScriptParserandInstallScriptExecutor.Bug Fixes
HasRunKeybefore skip-check so fallback-keyed actions don’t re-run.user.reg, HKLM →system.reg) for reads/writes and completion marks.UbisoftConnectStepfor Steam games, handled by InstallScripts.Written for commit c442dfb. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Refactor
Improvements
Tests