Feat/shared game containers - #1757
Conversation
📝 WalkthroughWalkthroughThe change adds an opt-in Shared Container Base preference and emulation toggle, uses symlinks instead of copies for common DLLs when enabled, aligns SQLite symlink handling, and adds architecture, development, and feature documentation. ChangesShared Container Base
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsGroupEmulation
participant PrefManager
participant ContainerManager
participant FileUtils
User->>SettingsGroupEmulation: enable Shared Container Base
SettingsGroupEmulation->>PrefManager: persist preference
ContainerManager->>PrefManager: read preference during DLL extraction
ContainerManager->>FileUtils: symlink common DLLs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/walkthrough.artifact.md (1)
27-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSeparate completed verification from the manual test procedure.
The “Verification Results” section records only a build result; lines 32-38 describe planned checks rather than observed outcomes. Record pass/fail results for symlink creation, container deletion isolation, and game launch, or rename this section to “Manual Verification Steps.”
🤖 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 @.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/walkthrough.artifact.md around lines 27 - 38, Rename the “Manual Verification Path” subsection to “Manual Verification Steps” unless the checks were actually performed. If verification results are documented, replace the planned instructions with observed pass/fail outcomes covering symlink creation, container deletion isolation, and game launch.
🧹 Nitpick comments (1)
.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/implementation_plan.artifact.md (1)
41-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the test plan with the production abstraction.
The implementation delegates to
FileUtils.symlink, but the proposed test only verifiesOs.symlink. Test throughContainerManagerand verifyFileUtils.symlinkbehavior, including replacement of an existing destination, so the test covers the actual contract being changed.🤖 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 @.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/implementation_plan.artifact.md around lines 41 - 43, Update the Automated Tests plan to test through ContainerManager and mock or verify FileUtils.symlink rather than Os.symlink. Include coverage for the feature-enabled path and replacement of an existing destination, while retaining the existing container creation regression tests.
🤖 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
@.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/implementation_plan.artifact.md:
- Around line 16-34: Replace all absolute file:///E:/workspace/... references in
.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/implementation_plan.artifact.md
lines 16-34 with repository-relative links, preserving the referenced files and
sections. Apply the same repository-relative link conversion to every
render_diffs(...) reference in
.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/walkthrough.artifact.md lines
10-25.
- Around line 32-35: The UI-toggle work must remain marked incomplete because
the Shared Container Base implementation is absent. In
.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/implementation_plan.artifact.md
lines 32-35, .artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/task.artifact.md
lines 3-7, and
.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/walkthrough.artifact.md lines
12-15, revise completion claims to indicate the SettingsGroupEmulation toggle
and its preference, strings, and symlink logic are still pending.
In `@DEVELOPMENT_NOTES.md`:
- Around line 40-45: Update the roadmap entries in DEVELOPMENT_NOTES.md to
reflect that the shared-container feasibility work, experimental “Shared Base”
toggle, and approved implementation have shipped in this change; mark Phases 2
and 3 as delivered or otherwise clearly identify the section as a historical
pre-implementation snapshot, while preserving the listed scope.
---
Outside diff comments:
In @.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/walkthrough.artifact.md:
- Around line 27-38: Rename the “Manual Verification Path” subsection to “Manual
Verification Steps” unless the checks were actually performed. If verification
results are documented, replace the planned instructions with observed pass/fail
outcomes covering symlink creation, container deletion isolation, and game
launch.
---
Nitpick comments:
In
@.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/implementation_plan.artifact.md:
- Around line 41-43: Update the Automated Tests plan to test through
ContainerManager and mock or verify FileUtils.symlink rather than Os.symlink.
Include coverage for the feature-enabled path and replacement of an existing
destination, while retaining the existing container creation regression tests.
🪄 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: 6118bc50-d053-4f38-9a92-fd6a39822ac0
📒 Files selected for processing (6)
.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/implementation_plan.artifact.md.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/task.artifact.md.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/walkthrough.artifact.mdAGENTS.mdARCHITECTURE.mdDEVELOPMENT_NOTES.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/walkthrough.artifact.md">
<violation number="1" location=".artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/walkthrough.artifact.md:10">
P2: The walkthrough contains unresolved `render_diffs(...)` template callouts on four lines. These appear to be placeholder tags that should have been replaced with the actual diff content for each modified file. As written, they'll render as plain text `render_diffs(...)` which looks broken and provides no value to reviewers. Replace each with the corresponding diff snippet or remove them if the walkthrough is meant to be prose-only.</violation>
</file>
<file name="ARCHITECTURE.md">
<violation number="1" location="ARCHITECTURE.md:16">
P2: ARCHITECTURE.md lists Box64 and FEXCore as part of `src/main/cpp` native components, but no source files for either exist in that directory. The cpp tree contains proot, virglrenderer, patchelf, adrenotools, winlator, and other native code. Box64/FEXCore are referenced and configured in Java/Kotlin (Container.java, PrefManager.kt), not compiled from source here. Update the doc to accurately reflect what lives in src/main/cpp, or describe Box64/FEXCore separately under native integration.</violation>
<violation number="2" location="ARCHITECTURE.md:36">
P2: The doc claims navigation uses 'Type-safe routes' with navigation-compose, but the project uses traditional string-based sealed class routes (PluviaScreen.route: String) and navController.navigate(String). True type-safe routes (Navigation 2.8+) require @Serializable route classes and compile-time safe APIs. Update the doc to accurately describe the current string-based sealed class approach.</violation>
</file>
<file name=".artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/task.artifact.md">
<violation number="1" location=".artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/task.artifact.md:3">
P1: The task list marks all implementation items as complete (`[x]`), but none of the referenced source files (`PrefManager.kt`, `strings.xml`, `SettingsGroupEmulation.kt`, `ContainerManager.java`) are actually modified in this PR. These checkmarks are misleading—they should be unchecked (`[ ]`) to accurately reflect that the implementation has not been delivered yet.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -0,0 +1,7 @@ | |||
| # Phase 3 — Shared-Container Base Implementation Task List | |||
There was a problem hiding this comment.
P1: The task list marks all implementation items as complete ([x]), but none of the referenced source files (PrefManager.kt, strings.xml, SettingsGroupEmulation.kt, ContainerManager.java) are actually modified in this PR. These checkmarks are misleading—they should be unchecked ([ ]) to accurately reflect that the implementation has not been delivered yet.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/task.artifact.md, line 3:
<comment>The task list marks all implementation items as complete (`[x]`), but none of the referenced source files (`PrefManager.kt`, `strings.xml`, `SettingsGroupEmulation.kt`, `ContainerManager.java`) are actually modified in this PR. These checkmarks are misleading—they should be unchecked (`[ ]`) to accurately reflect that the implementation has not been delivered yet.</comment>
<file context>
@@ -0,0 +1,7 @@
+# Phase 3 — Shared-Container Base Implementation Task List
+
+- [x] Update `PrefManager.kt` with `use_shared_container_base` preference
+- [x] Add localized strings in `strings.xml`
+- [x] Add the experimental toggle to `SettingsGroupEmulation.kt`
</file context>
| ### 1. Persistence & Settings | ||
| I added a new boolean preference `useSharedContainerBase` to `PrefManager.kt`. This allows the user to opt-in to the experimental feature. | ||
|
|
||
| render_diffs(file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/PrefManager.kt) |
There was a problem hiding this comment.
P2: The walkthrough contains unresolved render_diffs(...) template callouts on four lines. These appear to be placeholder tags that should have been replaced with the actual diff content for each modified file. As written, they'll render as plain text render_diffs(...) which looks broken and provides no value to reviewers. Replace each with the corresponding diff snippet or remove them if the walkthrough is meant to be prose-only.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/walkthrough.artifact.md, line 10:
<comment>The walkthrough contains unresolved `render_diffs(...)` template callouts on four lines. These appear to be placeholder tags that should have been replaced with the actual diff content for each modified file. As written, they'll render as plain text `render_diffs(...)` which looks broken and provides no value to reviewers. Replace each with the corresponding diff snippet or remove them if the walkthrough is meant to be prose-only.</comment>
<file context>
@@ -0,0 +1,37 @@
+### 1. Persistence & Settings
+I added a new boolean preference `useSharedContainerBase` to `PrefManager.kt`. This allows the user to opt-in to the experimental feature.
+
+render_diffs(file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/PrefManager.kt)
+
+### 2. UI Integration
</file context>
|
|
||
| ### 3. Navigation & State Management | ||
| - **UI Framework**: Jetpack Compose with Material 3. | ||
| - **Navigation**: `navigation-compose` with Type-safe routes. |
There was a problem hiding this comment.
P2: The doc claims navigation uses 'Type-safe routes' with navigation-compose, but the project uses traditional string-based sealed class routes (PluviaScreen.route: String) and navController.navigate(String). True type-safe routes (Navigation 2.8+) require @serializable route classes and compile-time safe APIs. Update the doc to accurately describe the current string-based sealed class approach.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ARCHITECTURE.md, line 36:
<comment>The doc claims navigation uses 'Type-safe routes' with navigation-compose, but the project uses traditional string-based sealed class routes (PluviaScreen.route: String) and navController.navigate(String). True type-safe routes (Navigation 2.8+) require @Serializable route classes and compile-time safe APIs. Update the doc to accurately describe the current string-based sealed class approach.</comment>
<file context>
@@ -0,0 +1,51 @@
+
+### 3. Navigation & State Management
+- **UI Framework**: Jetpack Compose with Material 3.
+- **Navigation**: `navigation-compose` with Type-safe routes.
+- **DI**: Hilt for dependency injection.
+- **Database**: Room for game metadata and local state.
</file context>
| - `:app`: The main Android application. | ||
| - `src/main/java/com/winlator`: Core engine logic (Java/Kotlin mix). | ||
| - `src/main/java/app/gamenative`: Modern app logic (Kotlin). | ||
| - `src/main/cpp`: Native components including Box64, FEXCore, and various patches. |
There was a problem hiding this comment.
P2: ARCHITECTURE.md lists Box64 and FEXCore as part of src/main/cpp native components, but no source files for either exist in that directory. The cpp tree contains proot, virglrenderer, patchelf, adrenotools, winlator, and other native code. Box64/FEXCore are referenced and configured in Java/Kotlin (Container.java, PrefManager.kt), not compiled from source here. Update the doc to accurately reflect what lives in src/main/cpp, or describe Box64/FEXCore separately under native integration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ARCHITECTURE.md, line 16:
<comment>ARCHITECTURE.md lists Box64 and FEXCore as part of `src/main/cpp` native components, but no source files for either exist in that directory. The cpp tree contains proot, virglrenderer, patchelf, adrenotools, winlator, and other native code. Box64/FEXCore are referenced and configured in Java/Kotlin (Container.java, PrefManager.kt), not compiled from source here. Update the doc to accurately reflect what lives in src/main/cpp, or describe Box64/FEXCore separately under native integration.</comment>
<file context>
@@ -0,0 +1,51 @@
+- `:app`: The main Android application.
+ - `src/main/java/com/winlator`: Core engine logic (Java/Kotlin mix).
+ - `src/main/java/app/gamenative`: Modern app logic (Kotlin).
+ - `src/main/cpp`: Native components including Box64, FEXCore, and various patches.
+- `:ubuntufs`: A dynamic feature module containing the base Linux rootfs (`imagefs`).
+
</file context>
…ation paths This commit addresses feedback from PR utkarshdalal#1757: - Implemented 'Shared Container Base' toggle and symlink logic. - Fixed absolute paths in documentation artifacts. - Aligned symlink usage in SteamBootstrap.kt. - Updated DEVELOPMENT_NOTES.md roadmap.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/winlator/container/ContainerManager.java (1)
333-347: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not treat failed symlink creation as successful extraction.
FileUtils.symlinkinapp/src/main/java/com/winlator/core/FileUtils.java, Lines [134]-[146], deletes the destination and swallowsErrnoException. IfOs.symlinkfails, extraction continues with a missing DLL; in the first path, an existing DLL may already have been deleted. Make symlink creation return a success status and fall back to copying or abort extraction.Also applies to: 351-375
🤖 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/com/winlator/container/ContainerManager.java` around lines 333 - 347, Update FileUtils.symlink to return whether Os.symlink succeeded instead of swallowing failure after deleting the destination. In ContainerManager extraction paths using useSharedBase, check that result and fall back to FileUtils.copy or abort extraction before reporting success, including the additional path around lines 351–375. Preserve listener-selected destinations and avoid treating a missing DLL as successfully extracted.
🧹 Nitpick comments (1)
app/src/main/java/com/winlator/container/ContainerManager.java (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm approval and document this protected compatibility-layer change.
This modifies
com.winlator.container.*and introduces a dependency onapp.gamenative.PrefManager. Please attach explicit approval and add a concise comment explaining the boundary and why shared-base symlinking only applies during extraction of new containers.As per coding guidelines,
com.winlator.container.*must not be modified without explicit approval, and modifications tocom.winlatormust remain minimal and documented.Also applies to: 333-347, 351-375
🤖 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/com/winlator/container/ContainerManager.java` at line 8, Obtain and record explicit approval for modifying the protected com.winlator.container compatibility layer, then add a concise comment near the PrefManager dependency and affected extraction logic documenting the boundary and that shared-base symlinking applies only while extracting new containers. Keep changes to com.winlator minimal and limited to this documented compatibility behavior.Source: Coding guidelines
🤖 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/PrefManager.kt`:
- Around line 1340-1343: Update the sharedContainerBase persistence flow around
PrefManager.sharedContainerBase and
ContainerManager.extractCommonDlls/createContainerFuture so a changed value is
visible or its write is completed before container creation begins. Ensure
extraction reads the intended persisted setting and does not choose copy mode
due to the setter’s asynchronous write.
---
Outside diff comments:
In `@app/src/main/java/com/winlator/container/ContainerManager.java`:
- Around line 333-347: Update FileUtils.symlink to return whether Os.symlink
succeeded instead of swallowing failure after deleting the destination. In
ContainerManager extraction paths using useSharedBase, check that result and
fall back to FileUtils.copy or abort extraction before reporting success,
including the additional path around lines 351–375. Preserve listener-selected
destinations and avoid treating a missing DLL as successfully extracted.
---
Nitpick comments:
In `@app/src/main/java/com/winlator/container/ContainerManager.java`:
- Line 8: Obtain and record explicit approval for modifying the protected
com.winlator.container compatibility layer, then add a concise comment near the
PrefManager dependency and affected extraction logic documenting the boundary
and that shared-base symlinking applies only while extracting new containers.
Keep changes to com.winlator minimal and limited to this documented
compatibility behavior.
🪄 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: fcf4eec9-1d14-481b-bb3b-1deea217ea54
📒 Files selected for processing (8)
.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/implementation_plan.artifact.md.artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/walkthrough.artifact.mdDEVELOPMENT_NOTES.mdapp/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/SteamBootstrap.ktapp/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupEmulation.ktapp/src/main/java/com/winlator/container/ContainerManager.javaapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- .artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/walkthrough.artifact.md
- DEVELOPMENT_NOTES.md
- .artifacts/efc1708b-9af8-4d4e-81f6-4a531aa2ec8c/implementation_plan.artifact.md
| private val SHARED_CONTAINER_BASE = booleanPreferencesKey("shared_container_base") | ||
| var sharedContainerBase: Boolean | ||
| get() = getPref(SHARED_CONTAINER_BASE, false) | ||
| set(value) = setPref(SHARED_CONTAINER_BASE, value) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)PrefManager\.kt$|ContainerManager\.|pref.*Base|shared_container_base' || true
echo "== PrefManager outline =="
ast-grep outline app/src/main/java/app/gamenative/PrefManager.kt || true
echo "== read PrefManager relevant sections =="
sed -n '1,180p' app/src/main/java/app/gamenative/PrefManager.kt
sed -n '1310,1360p' app/src/main/java/app/gamenative/PrefManager.kt
echo "== search shared_container_base and sharedContainerBase usages =="
rg -n "shared_container_base|sharedContainerBase" app/src/main || true
echo "== ContainerManager outline if present =="
if [ -f app/src/main/java/app/gamenative/ContainerManager.kt ]; then
ast-grep outline app/src/main/java/app/gamenative/ContainerManager.kt || true
rg -n -C 6 "sharedContainerBase|shared_container_base|DLL|copy|symlink|extract" app/src/main/java/app/gamenative/ContainerManager.kt || true
fiRepository: utkarshdalal/GameNative
Length of output: 18187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ContainerManager.java outline/size =="
wc -l app/src/main/java/com/winlator/container/ContainerManager.java
ast-grep outline app/src/main/java/com/winlator/container/ContainerManager.java || true
echo "== read ContainerManager.java extraction-related sections =="
rg -n -C 8 "sharedContainerBase|shared_container_base|symlink|symlinks|copy|DLL|extract|Base64|Base" app/src/main/java/com/winlator/container/ContainerManager.java || true
echo "== read likely ContainerManager sections around matches =="
python3 - <<'PY'
from pathlib import Path
p=Path('app/src/main/java/com/winlator/container/ContainerManager.java')
lines=p.read_text().splitlines()
for needle in ["sharedContainerBase","shared_container_base","symlink","symlinks","copy"]:
print(f"\n-- around {needle!r} --")
for i,l in enumerate(lines,1):
if needle in l:
start=max(1,i-20); end=min(len(lines),i+45)
for n in range(start,end+1):
print(f"{n:5d}\t{lines[n-1]}")
print()
PY
echo "== call sites for createContainerContainerManager methods =="
rg -n -C 6 "ContainerManager.*create|create.*Container" app/src/main/java app/src/main/kotlin app/src/main/java/app/gamenative || true
echo "== inspect UI settings state logic =="
sed -n '90,125p' app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupEmulation.ktRepository: utkarshdalal/GameNative
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== call sites for container creation and sharedContainerBase toggle =="
rg -n -C 8 "createContainer(Future|Async|Default|Async|Future|Default)|PrefManager\.sharedContainerBase|int\.(settings_emulation_shared_container_base_title|settings_emulation_shared_container_base_subtitle)|sharedContainerBase =" app/src/main/java app/src/main/kotlin app/src/main/java/app/gamenative || true
echo "== read settings toggle implementation =="
sed -n '90,125p' app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupEmulation.kt
echo "== read container UI/action implementations likely invoking createContainerX =="
rg -n -C 12 "Create|create|New container|ContainerManager|createContainer" app/src/main/java/app/gamenative app/src/main/kotlin app/src/main/java/com/winlator | sed -n '1,220p' || trueRepository: utkarshdalal/GameNative
Length of output: 41560
Serialize sharedContainerBase writes with subsequent container creation.
PrefManager.sharedContainerBase setter writes asynchronously, while ContainerManager.extractCommonDlls reads it synchronously during createContainerFuture. A container started immediately after the setting change can install DLLs by copy instead of symlink; keep the extraction call site aware of and persisted until the write completes, or make the read reflect the intent before createContainerFuture starts.
🤖 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/PrefManager.kt` around lines 1340 - 1343,
Update the sharedContainerBase persistence flow around
PrefManager.sharedContainerBase and
ContainerManager.extractCommonDlls/createContainerFuture so a changed value is
visible or its write is completed before container creation begins. Ensure
extraction reads the intended persisted setting and does not choose copy mode
due to the setter’s asynchronous write.
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/container/ContainerManager.java">
<violation number="1" location="app/src/main/java/com/winlator/container/ContainerManager.java:344">
P1: Duplicating a shared-base container produces a prefix missing every common DLL: `FileUtils.copy` intentionally skips symlink sources. Preserve/recreate the links (or materialize their targets) in `duplicateContainer` so copied containers remain runnable.</violation>
</file>
<file name="app/src/main/java/app/gamenative/SteamBootstrap.kt">
<violation number="1" location="app/src/main/java/app/gamenative/SteamBootstrap.kt:227">
P3: When FileUtils.symlink fails silently (caught and logged internally), sqliteCompatLink is still assigned because the exception no longer propagates to the outer catch block. The stale pointer has no runtime impact since removeSqliteCompatLink gracefully bails out, but the inconsistency could complicate future debugging or cleanup logic. Consider wrapping the call in a result check, e.g. runCatching { FileUtils.symlink(...) }.onFailure { return } before assigning sqliteCompatLink.</violation>
</file>
<file name="app/src/main/java/app/gamenative/PrefManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/PrefManager.kt:1343">
P2: The `sharedContainerBase` setter delegates to `setPref` which performs an asynchronous DataStore write. However, `ContainerManager.extractCommonDlls` reads this value synchronously via `getSharedContainerBase()` during container creation. If a container is created shortly after toggling this setting, the async write may not have completed, causing the read to return the previous value — resulting in DLLs being copied instead of symlinked (or vice versa). Consider either making the write synchronous (e.g., `runBlocking`) for this preference, or passing the intended value directly to the container creation flow rather than re-reading it from the store.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| FileUtils.copy(new File(srcDir, dlname), dstFile); | ||
| File srcFile = new File(srcDir, dlname); | ||
| if (useSharedBase) { | ||
| FileUtils.symlink(srcFile, dstFile); |
There was a problem hiding this comment.
P1: Duplicating a shared-base container produces a prefix missing every common DLL: FileUtils.copy intentionally skips symlink sources. Preserve/recreate the links (or materialize their targets) in duplicateContainer so copied containers remain runnable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/com/winlator/container/ContainerManager.java, line 344:
<comment>Duplicating a shared-base container produces a prefix missing every common DLL: `FileUtils.copy` intentionally skips symlink sources. Preserve/recreate the links (or materialize their targets) in `duplicateContainer` so copied containers remain runnable.</comment>
<file context>
@@ -337,13 +339,19 @@ private void extractCommonDlls(String srcName, String dstName, JSONObject common
- FileUtils.copy(new File(srcDir, dlname), dstFile);
+ File srcFile = new File(srcDir, dlname);
+ if (useSharedBase) {
+ FileUtils.symlink(srcFile, dstFile);
+ } else {
+ FileUtils.copy(srcFile, dstFile);
</file context>
| private val SHARED_CONTAINER_BASE = booleanPreferencesKey("shared_container_base") | ||
| var sharedContainerBase: Boolean | ||
| get() = getPref(SHARED_CONTAINER_BASE, false) | ||
| set(value) = setPref(SHARED_CONTAINER_BASE, value) |
There was a problem hiding this comment.
P2: The sharedContainerBase setter delegates to setPref which performs an asynchronous DataStore write. However, ContainerManager.extractCommonDlls reads this value synchronously via getSharedContainerBase() during container creation. If a container is created shortly after toggling this setting, the async write may not have completed, causing the read to return the previous value — resulting in DLLs being copied instead of symlinked (or vice versa). Consider either making the write synchronous (e.g., runBlocking) for this preference, or passing the intended value directly to the container creation flow rather than re-reading it from the store.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/PrefManager.kt, line 1343:
<comment>The `sharedContainerBase` setter delegates to `setPref` which performs an asynchronous DataStore write. However, `ContainerManager.extractCommonDlls` reads this value synchronously via `getSharedContainerBase()` during container creation. If a container is created shortly after toggling this setting, the async write may not have completed, causing the read to return the previous value — resulting in DLLs being copied instead of symlinked (or vice versa). Consider either making the write synchronous (e.g., `runBlocking`) for this preference, or passing the intended value directly to the container creation flow rather than re-reading it from the store.</comment>
<file context>
@@ -1337,6 +1337,11 @@ object PrefManager {
+ private val SHARED_CONTAINER_BASE = booleanPreferencesKey("shared_container_base")
+ var sharedContainerBase: Boolean
+ get() = getPref(SHARED_CONTAINER_BASE, false)
+ set(value) = setPref(SHARED_CONTAINER_BASE, value)
+
// Game compatibility cache (JSON string)
</file context>
| if (existingTarget != "libsqlite3.so.0") { | ||
| if (existingTarget != null) link.delete() | ||
| Os.symlink("libsqlite3.so.0", link.absolutePath) | ||
| com.winlator.core.FileUtils.symlink("libsqlite3.so.0", link.absolutePath) |
There was a problem hiding this comment.
P3: When FileUtils.symlink fails silently (caught and logged internally), sqliteCompatLink is still assigned because the exception no longer propagates to the outer catch block. The stale pointer has no runtime impact since removeSqliteCompatLink gracefully bails out, but the inconsistency could complicate future debugging or cleanup logic. Consider wrapping the call in a result check, e.g. runCatching { FileUtils.symlink(...) }.onFailure { return } before assigning sqliteCompatLink.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/SteamBootstrap.kt, line 227:
<comment>When FileUtils.symlink fails silently (caught and logged internally), sqliteCompatLink is still assigned because the exception no longer propagates to the outer catch block. The stale pointer has no runtime impact since removeSqliteCompatLink gracefully bails out, but the inconsistency could complicate future debugging or cleanup logic. Consider wrapping the call in a result check, e.g. runCatching { FileUtils.symlink(...) }.onFailure { return } before assigning sqliteCompatLink.</comment>
<file context>
@@ -224,7 +224,7 @@ object SteamBootstrap {
if (existingTarget != "libsqlite3.so.0") {
if (existingTarget != null) link.delete()
- Os.symlink("libsqlite3.so.0", link.absolutePath)
+ com.winlator.core.FileUtils.symlink("libsqlite3.so.0", link.absolutePath)
}
sqliteCompatLink = link.absolutePath
</file context>
1 participant
Description
Implemented an experimental "Shared Container Base" storage optimization to address the high storage overhead of creating new containers. Currently, each new container copies over 800 system DLLs from
/opt/wine, incurring ~1.5GB–2.0GB of redundant data per game.This PR adds a toggle in Settings -> Emulation that, when enabled, symlinks these common system DLLs instead of copying them, significantly reducing the storage footprint.
Key Changes:
useSharedContainerBasestate management toPrefManager.ContainerManager.javato support conditional symlinking of system DLLs during the prefix extraction process.Recording
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.CONTRIBUTING.md.Summary by cubic
Adds an experimental Shared Container Base that symlinks common Wine system DLLs for new containers, cutting per-game storage by ~1.5–2.0GB. A new toggle in Settings → Emulation controls this; it’s off by default and only affects newly created containers.
New Features
shared_container_baseand read during container creation./opt/wine(or bundled Wine) instead of copying.Refactors
com.winlator.core.FileUtils.symlinkinSteamBootstrap.ktfor consistent symlink handling.Written for commit 78d4120. Summary will update on new commits.
Summary by CodeRabbit