Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds Steam Runtime and Proton update flows, switches downloads from wget to curl/curlp, canonicalizes several paths, relocates env sourcing to basedir, introduces a containerized launch path (steamrt/run → launchwrapper-srt → wine), updates versions (DFC, Java, SteamRT, Proton GE), and removes XComposite checker tooling. ChangesIgnore & Documentation
Launch & Runtime
Setup & Paths
Updater changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Steam
participant LW as launchwrapper
participant LWE as launchwrapper-env
participant SRT as steamrt/run
participant LWSRT as launchwrapper-srt
participant Wine as Wine/Proton
User->>LW: invoke wrapper (game launch)
LW->>LW: source common.sh (absolute path)\nset -e
LW->>LWE: exec launchwrapper-env wine [args]
LWE->>LWE: source common.sh & load-env\nnormalize paths (readlink -f)\nconfigure Wine env & bind list
LWE->>SRT: exec steamrt/run launchwrapper-srt [args]
SRT->>SRT: initialize container runtime
SRT->>LWSRT: start launchwrapper-srt inside container
LWSRT->>LWSRT: determine basedir\nadjust PATH (WOW64/non-WOW64)\noptionally append host usr paths
LWSRT->>Wine: exec wine [original args]
Wine->>User: run game and return status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 6
🧹 Nitpick comments (11)
update-component.sh (3)
1-10: Stale top-of-file comment.The header still says it supports only dxvk and faf-client, but the script now also handles
java,steamrt, andproton.♻️ Proposed fix
#!/usr/bin/env bash # Update specific components -# Currently supports updating dxvk and the faf client +# Supports updating: dxvk, faf-client, java, steamrt, proton set -e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@update-component.sh` around lines 1 - 10, Update the stale script header in update-component.sh: replace or expand the top-of-file comment that currently reads "Currently supports updating dxvk and the faf client" to accurately reflect all supported components (dxvk, faf-client, java, steamrt, proton) or change it to a generic statement like "Supports updating various components (dxvk, faf-client, java, steamrt, proton)"; ensure this updated summary is the first comment block immediately after the shebang so callers and maintainers see the correct supported list.
118-141:update-protonis not atomic — a failed extract leaves a half-populated Proton dir.
update-steamrt(lines 90–116) carefully extracts intosteamrt.new, swapssteamrt→steamrt.old, then renames.update-protoninstead extracts straight into the cwd, so an interrupted/failedtar(network drop, OOM, bad archive) yields a partially extractedGE-ProtonX-XX/that subsequent runs may mistake for a complete install ([[ -d "$proton_path" ]]on line 125 only logs an overwrite note, it does not re-extract from scratch).Recommend mirroring the steamrt pattern, or at minimum removing the target dir before extraction so a partial result is never treated as valid.
♻️ Suggested approach (minimal)
function update-proton() { local proton_url="$1" block-print "Downloading Proton" curlp -o "proton.tar.gz" "$proton_url" block-print "Extracting Proton" local proton_path="$(tar -tf "proton.tar.gz" | head -n 1 | cut -d '/' -f 1)" + if [[ -z "$proton_path" ]]; then + echo "ERROR: could not determine proton archive top-level dir" >&2 + exit 1 + fi if [[ -d "$proton_path" ]]; then - echo "note: requested proton version already exists, overwriting" + echo "note: requested proton version already exists, removing for clean re-extract" + rm -rf "$proton_path" fi # exclude wine mono and gecko, they are somewhat large (~450MB combined) and we never use them tar \ --exclude="$proton_path/files/share/wine/mono" \ --exclude="$proton_path/files/share/wine/gecko" \ --exclude="$proton_path/protonfixes" \ -xvf "proton.tar.gz" rm "proton.tar.gz"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@update-component.sh` around lines 118 - 141, The update-proton function is not atomic: modify update-proton so it extracts into a temporary directory (e.g., "$proton_path.new") instead of directly into cwd, validate/complete the extraction there, then atomically rename/swap the directories (move existing "$proton_path" to "$proton_path.old" and rename "$proton_path.new" to "$proton_path"); ensure you still pass the same --exclude flags to tar, clean up any ".new" or ".old" on failure, and only call write-env ("proton_path" and "proton_download_url_current") after the rename succeeds; reference update-proton, proton_path, and write-env when making the change.
90-116: LGTM — atomic swap pattern for steamrt is well done.Extract-to-
steamrt.new→ rename current tosteamrt.old→ promotesteamrt.new→ cleanupsteamrt.oldis exactly the right pattern: an interrupted run is recoverable on the next invocation, and there's no window wheresteamrtdoesn't exist while a new one is half-extracted. Nice.One minor follow-up: consider also recovering from a stale
steamrt.oldleft behind by a previous interrupted run (e.g., move it out of the way or remove it before starting), otherwise that path is only cleaned up after a successful extraction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@update-component.sh` around lines 90 - 116, The update-steamrt function currently only removes steamrt.old after a successful swap, so a stale steamrt.old from a prior interrupted run can persist and never be cleaned; modify update-steamrt to detect and handle an existing "steamrt.old" early (for example before extraction or before renaming the current steamrt) by moving it out of the way or removing it (ensure you reference the same "steamrt.old" symbol), so the atomic-swap logic (mkdir steamrt.new, tar into steamrt.new, mv steamrt steamrt.old, mv steamrt.new steamrt) can run without accumulating old backups and still remain recoverable on interruption..gitignore (1)
15-15: AnchorGE-Proton*/to repo root for consistency.Other top-level directory entries (e.g.,
/steamrt,/jdk-*,/dxvk-*,/prefix) are anchored with a leading slash. Without it,GE-Proton*/would also match nested directories with the same naming pattern, which is likely not intended given Proton is extracted at the repo root.♻️ Proposed fix
/steamrt -GE-Proton*/ +/GE-Proton*/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 15, The .gitignore entry "GE-Proton*/" is not anchored and may match nested directories; update the entry to anchor it to the repository root by prefixing it with a leading slash (i.e., change "GE-Proton*/" to "/GE-Proton*/") so it only ignores top-level Proton extractions; make this edit in the .gitignore file where the "GE-Proton*/" pattern appears.common.sh (1)
46-49: Add-f(or--fail-with-body) tocurlpto surface HTTP errors.Without
--fail, curl exits with status 0 on 4xx/5xx and writes the server's error body into the output file. Downstreamtar -xvfwill then fail with a confusing message far from the root cause (a 404 or transient 5xx). This will bite particularly hard on theupdate-steamrtandupdate-protonpaths.
--fail-with-body(available since curl 7.76.0) is preferable as it returns an error code while preserving error details. If broader compatibility is needed, use--failalone.🛡️ Proposed fix
function curlp() { # curl with progress bar - curl --progress-bar -L "$@" + curl --fail-with-body --progress-bar -L "$@" }For older curl versions or if
--fail-with-bodyis unavailable, use--failinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common.sh` around lines 46 - 49, Update the curlp() helper to pass curl's failure flag so HTTP 4xx/5xx return non-zero; in the curlp() function add --fail-with-body to the curl invocation (or --fail as a fallback for older curl versions) so errors are surfaced instead of producing a successful exit code with an error body; ensure the change targets the curl --progress-bar -L "$@" invocation inside function curlp.launchwrapper-env (2)
10-10:pushd/popdprint to stdout and pollute downstream consumers.
launchwrapper-envis invoked bylaunchwrapperand bysetup.sh, and its stdout may be read by tools/log scrapers.pushd "$basedir"will emit the directory stack to stdout (andpopdlikewise). Redirect both to/dev/null, or just usecd "$basedir"since you don’t actually need a stack here.♻️ Proposed change
-pushd "$basedir" +pushd "$basedir" > /dev/null @@ -popd +popd > /dev/nullAlso applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launchwrapper-env` at line 10, The use of pushd "$basedir" (and the matching popd) in launchwrapper-env prints the directory stack to stdout and pollutes downstream consumers; change these to either redirect their output to /dev/null (e.g., pushd "$basedir" > /dev/null 2>&1 and popd > /dev/null 2>&1) or simply replace pushd/popd with cd "$basedir" since no directory stack is required — update the occurrences of pushd "$basedir" and the corresponding popd accordingly.
12-12:readlink -fon potentially missing/empty values can silently produce surprising paths.If
wineprefix,dxvk_config_file,dxvk_cache_dir, orproton_pathare unset/empty (e.g., upgrading without re-runningsetup.sh, orupdate-component.sh protonfailing midway and not writingproton_path),readlink -f ""returns the empty string and the export ends up asKEY="". Combined withset -e, that won’t fail here, but laterpressure-vesselinvocations will get an emptyproton_pathand produce confusing errors. Consider validating these inload-env(or right here) before exporting:for v in wineprefix dxvk_config_file dxvk_cache_dir proton_path; do [[ -n "${!v}" ]] || { echo "error: $v is not set; please re-run setup.sh" >&2; exit 1; } doneAlso applies to: 17-18, 33-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launchwrapper-env` at line 12, The exports that use readlink -f (e.g., export WINEPREFIX="$(readlink -f "$wineprefix")") can silently produce empty values if variables like wineprefix, dxvk_config_file, dxvk_cache_dir, or proton_path are unset; add a pre-export validation loop (run in load-env or at top of this file) that iterates over those variable names (wineprefix, dxvk_config_file, dxvk_cache_dir, proton_path) and fails fast with a clear error and non-zero exit if any are empty, before calling readlink -f and exporting them, so downstream pressure-vessel/proton invocations never receive empty paths.launchwrapper-srt (2)
5-5: Unusedbasedir.
basediris computed but never referenced in this script. Either remove it or use it (e.g., to sourcecommon.sh/load-envif you want consistent env semantics inside the container).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launchwrapper-srt` at line 5, The script defines basedir via basedir="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" but never uses it; either remove that line or use basedir to locate and source shared files—e.g., change calls that source environment or helpers to use "${basedir}/common.sh" or "${basedir}/load-env" (or similar) so the script consistently loads env/commons relative to its location; if you opt to remove it simply delete the basedir assignment.
13-15: Quote the literal and align indentation.For consistency with line 7 and to avoid surprises if
path_append_host_usrever contains whitespace/glob chars, quote"1". Also, this block uses 4-space indent while the block above uses 2-space — pick one.♻️ Proposed change
-if [[ "$path_append_host_usr" = 1 ]]; then - export PATH="$PATH:/run/host/usr/bin:/run/host/usr/sbin" -fi +if [[ "$path_append_host_usr" = "1" ]]; then + export PATH="$PATH:/run/host/usr/bin:/run/host/usr/sbin" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launchwrapper-srt` around lines 13 - 15, Update the conditional to quote the literal and match the surrounding indentation: change the test to compare "$path_append_host_usr" = "1" (quote the "1") and align the block's indentation to the existing style used above (use the same 2-space indentation for the export line and fi) so the PATH export lines and conditional use consistent quoting and spacing; locate the block around the variable path_append_host_usr and the export PATH assignment to apply this fix.launchwrapper (1)
17-24: Dead gamescope block — consider removing rather than commenting out.The
: <<'EOF' … EOFheredoc neutralizes the gamescope selection logic, but the resulting$GAMESCOPEvariable is never consumed by line 26 anyway (the new exec ignores it). Either restore gamescope support by passing it throughlaunchwrapper-env, or drop the dead block to keep the file readable. Leaving it as a heredoc invites bitrot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launchwrapper` around lines 17 - 24, The heredoc block neutralizing the gamescope logic (the GAMESCOPE variable and the use_gamescope check) is dead code because exec no longer consumes GAMESCOPE; either restore gamescope support by wiring GAMESCOPE into the startup path (e.g., pass it through launchwrapper-env or incorporate it before the exec that starts the game) or remove the entire heredoc and related GAMESCOPE/use_gamescope handling to avoid bitrot; update or delete references to GAMESCOPE and use_gamescope so the script is consistent with the new exec behavior.setup.sh (1)
157-173: Switching wineprefix/DXVK paths to relative may break existing installs on upgrade.Pre-PR, these were typically absolute paths written into
common-env. Existing users who only runupdate.sh(notsetup.sh) will retain their old absolute paths, but newlaunchwrapper-env/launchwrapper-srtcontainerized code now relies onpushd "$basedir"+readlink -fto resolve them. That works for either form, so nothing breaks today — but if you intend the relative form to be canonical (e.g., for portability of the install dir), document the expectation, or havesetup.shfail-soft warn users to re-run setup after the SteamRT migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` around lines 157 - 173, The change to write relative paths (wineprefix, dxvk_cache_dir, game_path, game_data_path, steam_path) can break upgrades; fix it by canonicalizing paths before calling write-env: resolve wineprefix, STEAM_PATH, GAME_PATH, and GAME_DATA_PATH with readlink -f (or otherwise convert to absolute) so the values written by write-env are stable, and add a fail-soft warning in setup.sh (near the wineprefix/dxvk_cache_dir block) telling users to re-run setup if they previously had absolute paths or after the SteamRT migration; reference the write-env calls and variables wineprefix, dxvk_cache_dir, GAME_PATH, GAME_DATA_PATH, STEAM_PATH, and the existing pushd "$basedir" + readlink -f logic to guide where to apply the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@launchwrapper`:
- Around line 11-15: The comment incorrectly claims pressure-vessel filters
LD_PRELOAD; update the launchwrapper to remove or revise the TODO and clarify
that LD_PRELOAD set by the script (export LD_PRELOAD="$(readlink -f
"$steam_path/ubuntu12_32/gameoverlayrenderer.so")") will propagate into the
SteamRT container via launchwrapper-env, and that the real concern is whether
that path is bind-mounted (PRESSURE_VESSEL_FILESYSTEMS_RW) and accessible inside
the container; either remove the misleading TODO, or replace it with a note to
prefer setting LD_PRELOAD inside the container after mount setup (or use
pressure-vessel env mechanisms) and also remove or mark the dead gamescope block
(lines containing the disabled gamescope code) as intentional dead code.
In `@launchwrapper-env`:
- Around line 45-57: The script currently builds bind_paths and exports
PRESSURE_VESSEL_FILESYSTEMS_RW using only basedir, game_path, and steam_path,
which will leave proton_path unmounted if Proton is outside basedir; modify the
bind_paths logic (the array used to compute PRESSURE_VESSEL_FILESYSTEMS_RW) to
either append "$(readlink -f "$proton_path")" when proton_path is set and
non-empty or add a runtime assertion that proton_path resides under basedir
(e.g., compare readlink -f "$proton_path" with readlink -f "$basedir") and fail
early with a clear error; update the code that constructs bind_paths (the
bind_paths array and its conditional for steam_path) so proton_path is handled
the same way as steam_path to avoid silent runtime failures.
In `@launchwrapper-srt`:
- Around line 7-11: The faf_use_wow64 conditional in the launchwrapper script is
dead code because faf_use_wow64 is never initialized; either wire faf_use_wow64
into the initialization chain (set it in setup.sh or export it from
launchwrapper-env) so the branch can be taken, or remove the branch and always
use the regular bin path; specifically either add a clear assignment/export of
faf_use_wow64 (true/1 or false/0) in setup.sh or launchwrapper-env before the
block that references faf_use_wow64, or delete the if/else that toggles between
"$proton_path/files/bin-wow64/" and "$proton_path/files/bin/" to avoid
maintaining unused bin-wow64 logic.
In `@set-client-paths.sh`:
- Around line 12-14: The jq invocation that sets installationPath and
preferencesFile uses readlink -f on $game_path and $game_data_path which can
return empty on error and silently write "" into client.prefs; update the script
to capture the results of readlink -f into temporaries (e.g. resolved_game_path
and resolved_prefs_path), validate they are non-empty before calling jq (or fall
back to the original $game_path/$game_data_path), and if they are empty either
abort with a clear error or use the original literal path so installationPath
and preferencesFile are not overwritten with an empty string; adjust the jq
--arg invocations to use these validated variables (referencing installationPath
/ preferencesFile, $game_path, $game_data_path, and the jq call that builds
wrapper).
In `@update-component.sh`:
- Around line 70-88: In update-java(), the overwrite check references an
undefined variable java_path_orig so it never triggers; change the existence
check to use the computed java_path (or capture the pre-extract path into
java_path_orig before extraction) — e.g., compute local java_path="$(tar -tf
"java.tar.gz" | head -n 1 | cut -d '/' -f 1)" and then test [[ -d "$java_path"
]] (or assign that value to java_path_orig and test it) before running tar -xvf,
so the "overwriting" warning is shown correctly.
In `@update.sh`:
- Around line 97-103: The branch comparing proton_download_url_target and
proton_download_url_current dispatches the wrong subcommand: change the call to
./update-component.sh from steamrt to proton so the Proton update path is used;
specifically, in the block where proton_download_url_target !=
proton_download_url_current (and where has_updates and perform_update are
set/checked), replace ./update-component.sh steamrt
"$proton_download_url_target" with ./update-component.sh proton
"$proton_download_url_target" to ensure the Proton installer is invoked.
---
Nitpick comments:
In @.gitignore:
- Line 15: The .gitignore entry "GE-Proton*/" is not anchored and may match
nested directories; update the entry to anchor it to the repository root by
prefixing it with a leading slash (i.e., change "GE-Proton*/" to "/GE-Proton*/")
so it only ignores top-level Proton extractions; make this edit in the
.gitignore file where the "GE-Proton*/" pattern appears.
In `@common.sh`:
- Around line 46-49: Update the curlp() helper to pass curl's failure flag so
HTTP 4xx/5xx return non-zero; in the curlp() function add --fail-with-body to
the curl invocation (or --fail as a fallback for older curl versions) so errors
are surfaced instead of producing a successful exit code with an error body;
ensure the change targets the curl --progress-bar -L "$@" invocation inside
function curlp.
In `@launchwrapper`:
- Around line 17-24: The heredoc block neutralizing the gamescope logic (the
GAMESCOPE variable and the use_gamescope check) is dead code because exec no
longer consumes GAMESCOPE; either restore gamescope support by wiring GAMESCOPE
into the startup path (e.g., pass it through launchwrapper-env or incorporate it
before the exec that starts the game) or remove the entire heredoc and related
GAMESCOPE/use_gamescope handling to avoid bitrot; update or delete references to
GAMESCOPE and use_gamescope so the script is consistent with the new exec
behavior.
In `@launchwrapper-env`:
- Line 10: The use of pushd "$basedir" (and the matching popd) in
launchwrapper-env prints the directory stack to stdout and pollutes downstream
consumers; change these to either redirect their output to /dev/null (e.g.,
pushd "$basedir" > /dev/null 2>&1 and popd > /dev/null 2>&1) or simply replace
pushd/popd with cd "$basedir" since no directory stack is required — update the
occurrences of pushd "$basedir" and the corresponding popd accordingly.
- Line 12: The exports that use readlink -f (e.g., export WINEPREFIX="$(readlink
-f "$wineprefix")") can silently produce empty values if variables like
wineprefix, dxvk_config_file, dxvk_cache_dir, or proton_path are unset; add a
pre-export validation loop (run in load-env or at top of this file) that
iterates over those variable names (wineprefix, dxvk_config_file,
dxvk_cache_dir, proton_path) and fails fast with a clear error and non-zero exit
if any are empty, before calling readlink -f and exporting them, so downstream
pressure-vessel/proton invocations never receive empty paths.
In `@launchwrapper-srt`:
- Line 5: The script defines basedir via basedir="$(dirname "$(readlink -f
"${BASH_SOURCE[0]}")")" but never uses it; either remove that line or use
basedir to locate and source shared files—e.g., change calls that source
environment or helpers to use "${basedir}/common.sh" or "${basedir}/load-env"
(or similar) so the script consistently loads env/commons relative to its
location; if you opt to remove it simply delete the basedir assignment.
- Around line 13-15: Update the conditional to quote the literal and match the
surrounding indentation: change the test to compare "$path_append_host_usr" =
"1" (quote the "1") and align the block's indentation to the existing style used
above (use the same 2-space indentation for the export line and fi) so the PATH
export lines and conditional use consistent quoting and spacing; locate the
block around the variable path_append_host_usr and the export PATH assignment to
apply this fix.
In `@setup.sh`:
- Around line 157-173: The change to write relative paths (wineprefix,
dxvk_cache_dir, game_path, game_data_path, steam_path) can break upgrades; fix
it by canonicalizing paths before calling write-env: resolve wineprefix,
STEAM_PATH, GAME_PATH, and GAME_DATA_PATH with readlink -f (or otherwise convert
to absolute) so the values written by write-env are stable, and add a fail-soft
warning in setup.sh (near the wineprefix/dxvk_cache_dir block) telling users to
re-run setup if they previously had absolute paths or after the SteamRT
migration; reference the write-env calls and variables wineprefix,
dxvk_cache_dir, GAME_PATH, GAME_DATA_PATH, STEAM_PATH, and the existing pushd
"$basedir" + readlink -f logic to guide where to apply the fix.
In `@update-component.sh`:
- Around line 1-10: Update the stale script header in update-component.sh:
replace or expand the top-of-file comment that currently reads "Currently
supports updating dxvk and the faf client" to accurately reflect all supported
components (dxvk, faf-client, java, steamrt, proton) or change it to a generic
statement like "Supports updating various components (dxvk, faf-client, java,
steamrt, proton)"; ensure this updated summary is the first comment block
immediately after the shebang so callers and maintainers see the correct
supported list.
- Around line 118-141: The update-proton function is not atomic: modify
update-proton so it extracts into a temporary directory (e.g.,
"$proton_path.new") instead of directly into cwd, validate/complete the
extraction there, then atomically rename/swap the directories (move existing
"$proton_path" to "$proton_path.old" and rename "$proton_path.new" to
"$proton_path"); ensure you still pass the same --exclude flags to tar, clean up
any ".new" or ".old" on failure, and only call write-env ("proton_path" and
"proton_download_url_current") after the rename succeeds; reference
update-proton, proton_path, and write-env when making the change.
- Around line 90-116: The update-steamrt function currently only removes
steamrt.old after a successful swap, so a stale steamrt.old from a prior
interrupted run can persist and never be cleaned; modify update-steamrt to
detect and handle an existing "steamrt.old" early (for example before extraction
or before renaming the current steamrt) by moving it out of the way or removing
it (ensure you reference the same "steamrt.old" symbol), so the atomic-swap
logic (mkdir steamrt.new, tar into steamrt.new, mv steamrt steamrt.old, mv
steamrt.new steamrt) can run without accumulating old backups and still remain
recoverable on interruption.
🪄 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: 06f86a78-dd1a-4af8-831e-1abe80082c9f
📒 Files selected for processing (15)
.gitignoreREADME.mdcommon.shinstall-shortcut.shlaunchwrapperlaunchwrapper-envlaunchwrapper-srtrunset-client-paths.shsetup.shsetup_dxvk.sh.patchtools/check_xcomposite.pyupdate-component.shupdate.shversions
💤 Files with no reviewable changes (2)
- setup_dxvk.sh.patch
- tools/check_xcomposite.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
setup.sh (2)
181-184: Component install ordering: confirmsteamrtis downloaded before anylaunchwrapper-envinvocation.
steamrtandprotonare downloaded at lines 183–184, thenlaunchwrapper-env wine wineboot -uruns at line 187. Sincelaunchwrapper-envnowexecs"$basedir/steamrt/run"(line 61 oflaunchwrapper-env), this ordering is required — please keep theseupdate-component.sh steamrt|protoncalls strictly before anylaunchwrapper-envusage. Consider adding a brief comment at line 181 to document that invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` around lines 181 - 184, The script currently must ensure steamrt and proton are downloaded before any use of launchwrapper-env because launchwrapper-env execs "$basedir/steamrt/run"; move or confirm the two update-component.sh calls for "steamrt" and "proton" remain immediately before any launchwrapper-env invocation (e.g., before the line that runs launchwrapper-env wine wineboot -u), and add a short comment above those update-component.sh lines noting the invariant ("steamrt/proton must be updated before any launchwrapper-env execs $basedir/steamrt/run") so future edits don’t reorder them.
157-172: Relative paths written tocommon-envrely on consumerscd-ing tobasedirfirst.
wineprefix,dxvk_config_file, anddxvk_cache_dirare now stored as bare relative names (prefix,dxvk.conf,dxvk-cache). This works becauselaunchwrapper-envwas updated topushd "$basedir"before resolving them withreadlink -f, andupdate-component.shdoes its owncd "$basedir". Be aware that any future script consuming these values must do the same — otherwisereadlink -fwill silently return a path relative to the caller's CWD. Worth a brief comment incommon-env/common.shnoting this contract, or alternatively writing them as"$basedir/prefix"etc. to be self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` around lines 157 - 172, The values written by write-env for wineprefix, dxvk_config_file and dxvk_cache_dir are relative (e.g., wineprefix="prefix") which will produce caller-dependent results when later resolved with readlink -f; update the write to store absolute paths (prefix with the base directory) or add a clear comment in common-env/common.sh documenting the contract that every consumer (e.g., launchwrapper-env and update-component.sh) must cd/pushd to $basedir before resolving these variables; target the write-env calls for wineprefix, dxvk_config_file and dxvk_cache_dir (and any other relative path variables) and either prepend "$basedir/" when writing them or add the contract comment near common-env/common.sh so future consumers know to cd into $basedir first.update-component.sh (2)
118-141:update-protonextracts over an existing install in place — stale files can persist.Unlike
update-steamrt, this overwrites the existing$proton_pathdirectory directly. If the new tarball renames or drops files (or if the user switches to a different Proton-GE version that maps to the same top-level directory), residual files from the previous install will linger insidefiles/. Consider applying the same stage-and-rename pattern asupdate-steamrt, or at minimumrm -rf "$proton_path"before extracting when the directory already exists. Note this is consistent with howupdate-javabehaves today, so it's a pre-existing pattern — flagging for visibility, not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@update-component.sh` around lines 118 - 141, The update-proton function currently extracts the tarball into the existing proton_path which can leave stale files; fix it by adopting the same pattern as update-steamrt/update-java: either remove the existing directory before extraction (rm -rf "$proton_path") or, better, extract into a temporary staging directory and atomically rename it to proton_path after successful extraction; ensure you reference update-proton, proton_path, proton_url, the tar extraction step and the write-env calls so the staging/cleanup happens before write-env "proton_path" and "proton_download_url_current" are written and only on successful extraction.
76-76: Shellcheck SC2155: declare and assign separately.
local foo="$(...)"masks the subshell's exit status, so a failure oftar -tf | head | cut(e.g., a corrupted archive) won't tripset -e. Low-impact here because the subsequenttar -xvfwould also fail, but worth fixing per shellcheck.♻️ Proposed fix (apply analogously to lines 42, 76, 124)
- local proton_path="$(tar -tf "proton.tar.gz" | head -n 1 | cut -d '/' -f 1)" + local proton_path + proton_path="$(tar -tf "proton.tar.gz" | head -n 1 | cut -d '/' -f 1)"Also applies to: 124-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@update-component.sh` at line 76, The local variable assignment for java_path is done in one step using a subshell which masks the subshell exit status; change to declare the variable first (local java_path) and then assign it in a separate statement (java_path="$(tar -tf "java.tar.gz" | head -n 1 | cut -d '/' -f 1)") so failures in the pipeline won't be hidden; apply the same pattern to the other similar occurrences (the other places that currently use local VAR="$(...)" in this script).launchwrapper-env (1)
33-33:readlink -f "$proton_path"on an unset/emptyproton_pathwill produce an empty string underset -e-safe code.If
common-envhas not yet hadproton_pathwritten (e.g., a partial setup or hand-edited env),readlink -f ""outputs an empty string, and the subsequentpressure-vesselinvocation will then run withproton_path=""— failing later with a less obvious error. Consider assertingproton_pathis non-empty here:🛡️ Proposed guard
+if [[ -z "$proton_path" ]]; then + echo "error: proton_path is not set; did setup.sh / update-component.sh proton run successfully?" >&2 + exit 1 +fi export proton_path="$(readlink -f "$proton_path")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launchwrapper-env` at line 33, The line exporting proton_path via export proton_path="$(readlink -f "$proton_path")" can yield an empty string when proton_path is unset, so add an explicit guard before this transformation: check that the variable proton_path is non-empty (e.g., test -n "$proton_path") and fail with a clear error message and non-zero exit if it is empty, then only call readlink -f and export the resolved path; reference the proton_path variable and the readlink -f usage to locate where to add the check and exit to prevent running pressure-vessel with an empty proton_path.update.sh (1)
73-79: Consider whether a Proton bump should re-runwinebootor refresh DXVK in the existing prefix.When the bundled Proton/Wine version changes, the existing
$wineprefixmay end up running against newer Wine binaries without an updater pass, and the previously installed DXVK DLLs are still those of the prior version.setup.shrunswineboot -uanddxvk-install.shafter Proton install;update.shdoes not. Worth confirming whether at least awineboot -u(and possibly re-runningupdate-component.sh dxvk) is desired here — otherwise users may hit subtle prefix/DLL version skew after a Proton bump.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@update.sh` around lines 73 - 79, When the Proton download URL changes (compare proton_download_url_target vs proton_download_url_current) ensure the update path not only calls update-component.sh proton but also refreshes the existing prefix: after a successful Proton update (inside the perform_update="yes" branch where update-component.sh proton is invoked) run a wineboot -u against the configured $wineprefix and re-run the DXVK installer (either dxvk-install.sh or update-component.sh dxvk) so the prefix DLLs match the new Wine/Proton binaries; guard these steps to only run if the $wineprefix exists and the proton update completed successfully.common.sh (1)
4-9:block-printno longer space-separates multiple args.Switching from
echo "$@"toprintf '%s' "$@"concatenates positional args without separators. All current callers pass a single string, so this is fine, but if a future caller writesblock-print "Downloading" "Steam Runtime", it will printDownloadingSteam Runtime. Considerprintf '%s\n' "$*"(orprintf '%s ' "$@"; echo) if you want IFS-separated joining; otherwise this is OK as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common.sh` around lines 4 - 9, The block-print function currently uses printf '%s' "$@" which concatenates multiple positional arguments without separators (e.g., "Downloading" "Steam Runtime" -> "DownloadingSteam Runtime"); update block-print (function name block-print) to join args with spaces or print each arg on its own line—for example replace the printf call with printf '%s\n' "$*" to preserve spaces between arguments or use printf '%s ' "$@"; echo to produce a single space-separated line—so future callers passing multiple args behave as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@update-component.sh`:
- Around line 90-116: In update-steamrt, prevent accidental nesting when
renaming by ensuring any stale "steamrt.old" is removed before calling mv
"steamrt" "steamrt.old" (or use mv -T "steamrt" "steamrt.old" on systems with
GNU mv); specifically, before the mv call in update-steamrt, check for and rm
-rf "steamrt.old" if it exists so the rename cannot move the new steamrt into an
existing directory, then proceed with the mv and subsequent cleanup and
write-env steps.
---
Nitpick comments:
In `@common.sh`:
- Around line 4-9: The block-print function currently uses printf '%s' "$@"
which concatenates multiple positional arguments without separators (e.g.,
"Downloading" "Steam Runtime" -> "DownloadingSteam Runtime"); update block-print
(function name block-print) to join args with spaces or print each arg on its
own line—for example replace the printf call with printf '%s\n' "$*" to preserve
spaces between arguments or use printf '%s ' "$@"; echo to produce a single
space-separated line—so future callers passing multiple args behave as expected.
In `@launchwrapper-env`:
- Line 33: The line exporting proton_path via export proton_path="$(readlink -f
"$proton_path")" can yield an empty string when proton_path is unset, so add an
explicit guard before this transformation: check that the variable proton_path
is non-empty (e.g., test -n "$proton_path") and fail with a clear error message
and non-zero exit if it is empty, then only call readlink -f and export the
resolved path; reference the proton_path variable and the readlink -f usage to
locate where to add the check and exit to prevent running pressure-vessel with
an empty proton_path.
In `@setup.sh`:
- Around line 181-184: The script currently must ensure steamrt and proton are
downloaded before any use of launchwrapper-env because launchwrapper-env execs
"$basedir/steamrt/run"; move or confirm the two update-component.sh calls for
"steamrt" and "proton" remain immediately before any launchwrapper-env
invocation (e.g., before the line that runs launchwrapper-env wine wineboot -u),
and add a short comment above those update-component.sh lines noting the
invariant ("steamrt/proton must be updated before any launchwrapper-env execs
$basedir/steamrt/run") so future edits don’t reorder them.
- Around line 157-172: The values written by write-env for wineprefix,
dxvk_config_file and dxvk_cache_dir are relative (e.g., wineprefix="prefix")
which will produce caller-dependent results when later resolved with readlink
-f; update the write to store absolute paths (prefix with the base directory) or
add a clear comment in common-env/common.sh documenting the contract that every
consumer (e.g., launchwrapper-env and update-component.sh) must cd/pushd to
$basedir before resolving these variables; target the write-env calls for
wineprefix, dxvk_config_file and dxvk_cache_dir (and any other relative path
variables) and either prepend "$basedir/" when writing them or add the contract
comment near common-env/common.sh so future consumers know to cd into $basedir
first.
In `@update-component.sh`:
- Around line 118-141: The update-proton function currently extracts the tarball
into the existing proton_path which can leave stale files; fix it by adopting
the same pattern as update-steamrt/update-java: either remove the existing
directory before extraction (rm -rf "$proton_path") or, better, extract into a
temporary staging directory and atomically rename it to proton_path after
successful extraction; ensure you reference update-proton, proton_path,
proton_url, the tar extraction step and the write-env calls so the
staging/cleanup happens before write-env "proton_path" and
"proton_download_url_current" are written and only on successful extraction.
- Line 76: The local variable assignment for java_path is done in one step using
a subshell which masks the subshell exit status; change to declare the variable
first (local java_path) and then assign it in a separate statement
(java_path="$(tar -tf "java.tar.gz" | head -n 1 | cut -d '/' -f 1)") so failures
in the pipeline won't be hidden; apply the same pattern to the other similar
occurrences (the other places that currently use local VAR="$(...)" in this
script).
In `@update.sh`:
- Around line 73-79: When the Proton download URL changes (compare
proton_download_url_target vs proton_download_url_current) ensure the update
path not only calls update-component.sh proton but also refreshes the existing
prefix: after a successful Proton update (inside the perform_update="yes" branch
where update-component.sh proton is invoked) run a wineboot -u against the
configured $wineprefix and re-run the DXVK installer (either dxvk-install.sh or
update-component.sh dxvk) so the prefix DLLs match the new Wine/Proton binaries;
guard these steps to only run if the $wineprefix exists and the proton update
completed successfully.
🪄 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: f6a2e483-88db-4efd-8e26-a9199038d3b4
📒 Files selected for processing (7)
common.shlaunchwrapperlaunchwrapper-envlaunchwrapper-srtsetup.shupdate-component.shupdate.sh
✅ Files skipped from review due to trivial changes (1)
- launchwrapper-srt
🚧 Files skipped from review as they are similar to previous changes (1)
- launchwrapper
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 11-18: Add a note to the README prerequisites that some kernels or
distro configs disable user namespaces and will cause setup.sh and update.sh to
exit with "cannot proceed: user namespaces not allowed"; document how to check
(e.g., sysctl kernel.unprivileged_userns_clone or
/proc/sys/kernel/unprivileged_userns_clone) and provide a safe remediation or
link (e.g., enable via sudo sysctl -w kernel.unprivileged_userns_clone=1 or
distro-specific guidance), and mention that both setup.sh and update.sh will
abort with that message so users should verify this prerequisite before running
those scripts.
In `@setup.sh`:
- Around line 101-104: Add a preflight check for the bubblewrap binary by
invoking the same helper used for other required tools: call ensure-bin bwrap
--version alongside the existing ensure-bin curl/jq/cabextract checks so bwrap
is validated early; ensure this check occurs before any heavy download/extract
work or before the first use of launchwrapper-env so the script fails fast if
bwrap is missing.
In `@update-component.sh`:
- Around line 168-175: The fallback case (case *) currently only prints usage
and exits successfully; update the case *) branch in the main argument dispatch
(the case statement handling subcommands) so it returns a non-zero exit status
for invalid subcommands (e.g., call exit 1 after printing the usage) to ensure
callers detect failure when an unknown component is passed.
In `@update.sh`:
- Around line 13-16: The script currently aborts early when
/proc/sys/kernel/apparmor_restrict_unprivileged_userns is set, which prevents
unrelated update steps (autoupdate-notify, git self-update, download/extract,
FAF client, Java, Proton, SteamRT) from running; change the behavior so the
AppArmor userns check only blocks the runtime/container-specific update path
instead of exiting the entire script—i.e., remove the global exit and relocate
or duplicate the check into the container/runtime update function or the
specific block that starts the runtime/container, and ensure non-container
updates (autoupdate-notify, git self-update, download/extract) run regardless of
that kernel setting.
🪄 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: 2f701601-a6b4-4576-b6ad-d35e2ae0d5ea
📒 Files selected for processing (8)
README.mdcommon.shlaunchwrapperlaunchwrapper-envlaunchwrapper-srtsetup.shupdate-component.shupdate.sh
✅ Files skipped from review due to trivial changes (2)
- launchwrapper-srt
- launchwrapper-env
🚧 Files skipped from review as they are similar to previous changes (1)
- launchwrapper
| *) | ||
| echo "Unknown component $1" | ||
| echo "Usage: $0 dxvk <dxvk version>" | ||
| echo " $0 faf-client <downlords-faf-client tag>" | ||
| echo " $0 java <java url>" | ||
| echo " $0 steamrt <steamrt url>" | ||
| echo " $0 proton <proton url>" | ||
| ;; |
There was a problem hiding this comment.
Return a non-zero status for invalid subcommands.
The fallback branch only prints usage and then exits successfully. Typos like an unsupported component name will look like a successful update to callers and scripts.
Suggested fix
*)
- echo "Unknown component $1"
- echo "Usage: $0 dxvk <dxvk version>"
- echo " $0 faf-client <downlords-faf-client tag>"
- echo " $0 java <java url>"
- echo " $0 steamrt <steamrt url>"
- echo " $0 proton <proton url>"
+ echo "Unknown component $1" >&2
+ echo "Usage: $0 dxvk <dxvk version>" >&2
+ echo " $0 faf-client <downlords-faf-client tag>" >&2
+ echo " $0 java <java url>" >&2
+ echo " $0 steamrt <steamrt url>" >&2
+ echo " $0 proton <proton url>" >&2
+ exit 1
;;
esac📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| *) | |
| echo "Unknown component $1" | |
| echo "Usage: $0 dxvk <dxvk version>" | |
| echo " $0 faf-client <downlords-faf-client tag>" | |
| echo " $0 java <java url>" | |
| echo " $0 steamrt <steamrt url>" | |
| echo " $0 proton <proton url>" | |
| ;; | |
| *) | |
| echo "Unknown component $1" >&2 | |
| echo "Usage: $0 dxvk <dxvk version>" >&2 | |
| echo " $0 faf-client <downlords-faf-client tag>" >&2 | |
| echo " $0 java <java url>" >&2 | |
| echo " $0 steamrt <steamrt url>" >&2 | |
| echo " $0 proton <proton url>" >&2 | |
| exit 1 | |
| ;; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@update-component.sh` around lines 168 - 175, The fallback case (case *)
currently only prints usage and exits successfully; update the case *) branch in
the main argument dispatch (the case statement handling subcommands) so it
returns a non-zero exit status for invalid subcommands (e.g., call exit 1 after
printing the usage) to ensure callers detect failure when an unknown component
is passed.
| if [[ -f /proc/sys/kernel/apparmor_restrict_unprivileged_userns ]] && [[ "$(cat /proc/sys/kernel/apparmor_restrict_unprivileged_userns)" != "0" ]]; then | ||
| echo "cannot proceed: user namespaces not allowed" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Don't gate the whole updater on user namespaces.
This runs before autoupdate-notify, git self-update, and the pure download/extract component updates. On a host with that AppArmor setting, ./update.sh cannot even refresh scripts, FAF client, Java, Proton, or SteamRT, even though those paths do not enter the runtime container.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@update.sh` around lines 13 - 16, The script currently aborts early when
/proc/sys/kernel/apparmor_restrict_unprivileged_userns is set, which prevents
unrelated update steps (autoupdate-notify, git self-update, download/extract,
FAF client, Java, Proton, SteamRT) from running; change the behavior so the
AppArmor userns check only blocks the runtime/container-specific update path
instead of exiting the entire script—i.e., remove the global exit and relocate
or duplicate the check into the container/runtime update function or the
specific block that starts the runtime/container, and ensure non-container
updates (autoupdate-notify, git self-update, download/extract) run regardless of
that kernel setting.
|
AppArmor userns restrictions seem to only really happen on Ubuntu; and furthermore not Debian nor most Ubuntu derivatives either. Probably best to just add something to the readme to disable userns enforcement The security benefits of the restriction are highly questionable at best |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@README.md`:
- Line 136: Update the user-facing sentence in README.md to use the hyphenated
adjective "open-source" instead of "open source"; find the line containing "I
stole a bunch of open source stuff from Steam that makes the game run better
(thanks Valve)" and replace "open source" with "open-source" so the sentence
reads "...open-source stuff...".
In `@setup.sh`:
- Around line 100-102: Update the comment that lists required programs to match
the actual checks: either remove "cabextract" from the comment if it's no longer
required, or add an ensure-bin call (ensure-bin cabextract --version) alongside
ensure-bin curl and ensure-bin jq; locate the requirement text near the existing
ensure-bin calls in setup.sh and make the comment and checks consistent.
- Around line 194-195: The test that checks for an existing directx path uses [[
-f "$basedir/directx" ]] which only matches regular files; change it to [[ -e
"$basedir/directx" ]] so leftover directories are also detected and removed
before extraction; update the conditional guarding rm -rv "$basedir/directx" to
use [[ -e "$basedir/directx" ]] while keeping rm -rv and mkdir -p
"$basedir/directx" as-is to ensure stale directories are removed and the
directory is recreated.
🪄 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: d30953db-4fe1-4d10-9636-f392c5d248db
📒 Files selected for processing (6)
.gitignoreREADME.mdlaunchwrapper-envlaunchwrapper-srtsetup.shupdate.sh
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- launchwrapper-srt
- launchwrapper-env
1 participant
They stopped giving TF2 codenames to steamrt since 4 :(
Launch FA using GE-Proton running in the Steam Runtime. Setup will fetch both GE-Proton and steamrt without depending on a Steam install. Should hopefully fix a bunch of strange issues and ease installation.
Unfortunately the FAF client depends on libraries that are not included in steamrt, so it still runs outside the container.
Fixes #42
Summary by CodeRabbit
Documentation
New Features
Chores