feat(imagefs): avoid extraction when changing container variant#484
feat(imagefs): avoid extraction when changing container variant#484darkcl wants to merge 1 commit intoutkarshdalal:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a recursive directory-merge utility with progress callbacks and refactors ImageFsInstaller to centralize imagefs extraction, support variant-specific directories, and perform progress-aware merging and install guards. Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant ImageFsInstaller as ImageFsInstaller
participant Extract as extractImageFs()
participant AssetMgr as AssetManager
participant TarUtils as TarCompressorUtils
participant VariantDir as VariantDir
participant FileUtils as FileUtils
participant Merge as mergeImageFsFolder()
Caller->>ImageFsInstaller: installFromAssetsFuture(...)
ImageFsInstaller->>Extract: extractImageFs(context, assetMgr, variant, progress, imageFs, rootDir)
alt imagefs from assets
Extract->>AssetMgr: open asset stream
AssetMgr-->>Extract: asset data
else imagefs from download
Extract->>TarUtils: download & provide stream
TarUtils-->>Extract: archive stream
end
Extract->>VariantDir: determine/getVariantDir(context, variant)
Extract->>TarUtils: extract archive into variantDir (with progress)
Extract-->>ImageFsInstaller: extraction complete / result
alt variant merge required
ImageFsInstaller->>Merge: mergeImageFsFolder(context, variant, rootDir, progress, isClean)
Merge->>FileUtils: mergeDirectoryRecursively(variantDir, rootDir, onProgress)
FileUtils-->Merge: onProgress callbacks (done, total)
Merge-->>ImageFsInstaller: merge complete
end
ImageFsInstaller-->>Caller: installation complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java">
<violation number="1" location="app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java:129">
P2: Merging the cached imagefs into rootDir uses Files.copy without preserving permissions, which can drop executable bits on binaries and break container startup after a variant merge.</violation>
<violation number="2" location="app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java:146">
P2: Cache reuse is based solely on `variantDir.exists()` without any integrity/marker check, so a partially extracted or failed cache can be treated as valid on subsequent runs and permanently merge corrupted data into the container.</violation>
</file>
<file name="app/src/main/java/com/winlator/core/FileUtils.java">
<violation number="1" location="app/src/main/java/com/winlator/core/FileUtils.java:195">
P2: mergeDirectoryRecursively walks the entire directory tree twice (once to count files and again to copy). This doubles traversal overhead for large imagefs directories and can significantly slow installs or variant merges.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| File downloaded = new File(context.getFilesDir(), imagefsFile); | ||
|
|
||
| File variantDir = getVariantDir(context, containerVariant); | ||
| if (variantDir.exists()) { |
There was a problem hiding this comment.
P2: Cache reuse is based solely on variantDir.exists() without any integrity/marker check, so a partially extracted or failed cache can be treated as valid on subsequent runs and permanently merge corrupted data into the container.
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/xenvironment/ImageFsInstaller.java, line 146:
<comment>Cache reuse is based solely on `variantDir.exists()` without any integrity/marker check, so a partially extracted or failed cache can be treated as valid on subsequent runs and permanently merge corrupted data into the container.</comment>
<file context>
@@ -158,6 +124,74 @@ else if (downloaded.exists()){
+ File downloaded = new File(context.getFilesDir(), imagefsFile);
+
+ File variantDir = getVariantDir(context, containerVariant);
+ if (variantDir.exists()) {
+ mergeImageFsFolder(context, containerVariant, rootDir, onProgress, false);
+ return true;
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/xenvironment/ImageFsInstaller.java (1)
104-188:⚠️ Potential issue | 🟠 MajorDon’t treat any existing variant dir as a valid install.
If extraction fails or is interrupted, the variant directory can exist but be incomplete; the next run will skip extraction and return success, leaving a corrupted imagefs. Add a success marker (or cleanup on failure) and only skip when it’s present.🧩 Suggested fix (marker + cleanup)
- File variantDir = getVariantDir(context, containerVariant); - if (variantDir.exists()) { + File variantDir = getVariantDir(context, containerVariant); + File markerFile = new File(variantDir, ".imagefs_complete"); + if (variantDir.isDirectory() && markerFile.isFile()) { mergeImageFsFolder(context, containerVariant, rootDir, onProgress, false); return true; + } else if (variantDir.exists()) { + FileUtils.delete(variantDir); // clean partial/failed extraction } @@ - mergeImageFsFolder(context, containerVariant, rootDir, onProgress, true); - return success; + if (success) { + markerFile.getParentFile().mkdirs(); + markerFile.createNewFile(); + mergeImageFsFolder(context, containerVariant, rootDir, onProgress, true); + } else { + FileUtils.delete(variantDir); + } + return success;
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/winlator/core/FileUtils.java`:
- Around line 188-218: The mergeDirectoryRecursively method currently calls
onProgress.onFileMerged(...) without checking for null; add a null-check for the
onProgress parameter (or require non-null) and only invoke onFileMerged when
onProgress != null. Locate mergeDirectoryRecursively and update the visitFile
(and any other places where onProgress is used) to guard the call with an if
(onProgress != null) check so callers can pass null safely.
In `@app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java`:
- Around line 127-135: The mergeImageFsFolder method invokes the onProgress
Callback inside the FileUtils.mergeDirectoryRecursively lambda without checking
for null, which will crash when no progress handler is supplied; update the
lambda in mergeImageFsFolder to null-guard onProgress (e.g., if (onProgress !=
null) { ... }) before calling onProgress.call(...) in both the isCleanInstall
and else branches so callbacks are only invoked when provided (identify the
callback reference as onProgress and the enclosing method as
mergeImageFsFolder/FileUtils.mergeDirectoryRecursively).
feat(imagefs): report merge directory progress
darkcl
force-pushed
the
glibc-bionic-swap
branch
from
715a84e to
17955fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java`:
- Around line 141-190: The extractImageFs method currently always calls
mergeImageFsFolder even when extraction failed; change it to only call
mergeImageFsFolder if extraction succeeded or the variantDir actually exists
(e.g., if (success || variantDir.exists()) { mergeImageFsFolder(...) }), so you
don't merge an empty/nonexistent folder; also simplify the contains check by
removing the redundant "== true" in the Arrays.asList(...).contains(imagefsFile)
condition. Ensure you reference extractImageFs, variantDir, success, and
mergeImageFsFolder when making the edits.
🧹 Nitpick comments (4)
app/src/main/java/com/winlator/core/FileUtils.java (1)
188-225: MissingvisitFileFailedhandler may cause silent failures or aborts.If any file cannot be read during traversal (permissions, broken symlink, etc.), the default behavior throws an
IOExceptionand aborts the entire merge. Consider adding avisitFileFailedoverride to log and continue, or to propagate errors gracefully.Also,
Files.copywithLinkOption.NOFOLLOW_LINKSwill attempt to copy symlinks as symlinks rather than following them. If this is intentional for preserving the imagefs structure, consider documenting it; otherwise, symlinks may cause unexpected behavior when the target doesn't exist on the destination.♻️ Suggested improvement for error resilience
Files.walkFileTree(source, new SimpleFileVisitor<>() { `@Override` public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { Path targetDir = target.resolve(source.relativize(dir)); Files.createDirectories(targetDir); return FileVisitResult.CONTINUE; } `@Override` public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { Path targetFile = target.resolve(source.relativize(file)); Files.copy(file, targetFile, LinkOption.NOFOLLOW_LINKS, StandardCopyOption.REPLACE_EXISTING); long done = processed.incrementAndGet(); if (onProgress != null) { onProgress.onFileMerged(done, total); } return FileVisitResult.CONTINUE; } + + `@Override` + public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException { + Log.w("FileUtils", "Failed to merge file: " + file + " - " + exc.getMessage()); + return FileVisitResult.CONTINUE; + } });app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java (3)
102-124: UncaughtIOExceptionfromextractImageFswill surface asExecutionException.The
extractImageFsmethod (line 104) declaresthrows IOException, but this exception is not caught within the executor'sCallable. WhenIOExceptionoccurs, callers retrieving theFutureresult via.get()will receive anExecutionExceptionwrapping the originalIOException, which may not be the intended behavior (vs. returningfalselike other failure paths).Consider wrapping the call in a try-catch to maintain consistent error handling:
♻️ Suggested fix for consistent error handling
return Executors.newSingleThreadExecutor().submit(() -> { clearRootDir(context, rootDir); - boolean success = extractImageFs(context, assetManager, containerVariant, onProgress, imageFs, rootDir); + boolean success; + try { + success = extractImageFs(context, assetManager, containerVariant, onProgress, imageFs, rootDir); + } catch (IOException e) { + Log.e("ImageFsInstaller", "Failed to extract imagefs: " + e.getMessage()); + return false; + } if (success) {
127-139: Prefer primitivebooleanoverBooleanwrapper.The
isCleanInstallparameter is used as a simple flag and is never null-checked. Using the primitivebooleanavoids potential NPE and unnecessary boxing.♻️ Minor fix
- private static void mergeImageFsFolder(Context context, String containerVariant, File rootDir, Callback<Integer> onProgress, Boolean isCleanInstall) throws IOException { + private static void mergeImageFsFolder(Context context, String containerVariant, File rootDir, Callback<Integer> onProgress, boolean isCleanInstall) throws IOException {
247-256: Method nameisImportedWineProtonno longer reflects its behavior.The simplified logic now returns
truefor anywine-*orproton-*directory, including bundled versions. The method name suggests it only identifies imported installations. Consider renaming toisWineOrProtonDirfor clarity, or add a comment explaining the intentional preservation of all Wine/Proton directories.♻️ Suggested rename for clarity
- private static boolean isImportedWineProton(Context context, String fileName) { + /** Returns true for any Wine or Proton directory to preserve during reinstalls. */ + private static boolean isWineOrProtonDir(String fileName) { String lowerName = fileName.toLowerCase(); // Not a Wine/Proton directory if (!lowerName.startsWith("wine-") && !lowerName.startsWith("proton-")) { return false; } return true; }And update the call site at line 268:
- if (isImportedWineProton(context, fileName)) { + if (isWineOrProtonDir(fileName)) {
| private static boolean extractImageFs(Context context, AssetManager assetManager, String containerVariant, Callback<Integer> onProgress, ImageFs imageFs, File rootDir) throws IOException { | ||
| final byte compressionRatio = 22; | ||
|
|
||
| String imagefsFile = containerVariant.equals(Container.GLIBC) ? "imagefs_gamenative.txz" : "imagefs_bionic.txz"; | ||
| File downloaded = new File(context.getFilesDir(), imagefsFile); | ||
|
|
||
| File variantDir = getVariantDir(context, containerVariant); | ||
| if (variantDir.exists()) { | ||
| mergeImageFsFolder(context, containerVariant, rootDir, onProgress, false); | ||
| return true; | ||
| } | ||
|
|
||
|
|
||
| boolean success = false; | ||
|
|
||
| if (Arrays.asList(context.getAssets().list("")).contains(imagefsFile) == true){ | ||
| final long contentLength = (long) (FileUtils.getSize(assetManager, imagefsFile) * (100.0f / compressionRatio)); | ||
| AtomicLong totalSizeRef = new AtomicLong(); | ||
| Log.d("Extraction", "extracting " + imagefsFile); | ||
|
|
||
| success = TarCompressorUtils.extract(TarCompressorUtils.Type.XZ, assetManager, imagefsFile, variantDir, (file, size) -> { | ||
| if (size > 0) { | ||
| long totalSize = totalSizeRef.addAndGet(size); | ||
| if (onProgress != null) { | ||
| final int progress = (int) (((float) totalSize / contentLength) * 100 / 2); | ||
| onProgress.call(progress); | ||
| } | ||
| } | ||
| return file; | ||
| }); | ||
| } | ||
|
|
||
| else if (downloaded.exists()){ | ||
| final long contentLength = (long) (FileUtils.getSize(downloaded) * (100.0f / compressionRatio)); | ||
| AtomicLong totalSizeRef = new AtomicLong(); | ||
| Log.d("Extraction", "extracting " + imagefsFile); | ||
| success = TarCompressorUtils.extract(TarCompressorUtils.Type.XZ, downloaded, variantDir, (file, size) -> { | ||
| if (size > 0) { | ||
| long totalSize = totalSizeRef.addAndGet(size); | ||
| if (onProgress != null) { | ||
| final int progress = (int) (((float) totalSize / contentLength) * 100 / 2); | ||
| onProgress.call(progress); | ||
| } | ||
| } | ||
| return file; | ||
| }); | ||
| } | ||
| mergeImageFsFolder(context, containerVariant, rootDir, onProgress, true); | ||
| return success; | ||
| } |
There was a problem hiding this comment.
mergeImageFsFolder called even when extraction fails; potential merge on empty directory.
When neither the asset nor the downloaded file exists, success remains false, yet mergeImageFsFolder (line 188) is still called on variantDir. This will either:
- Fail with
IOExceptionifvariantDirdoesn't exist - Merge an empty/incomplete directory if a previous partial extraction left artifacts
Consider guarding the merge call:
🐛 Proposed fix
else if (downloaded.exists()){
// ... extraction logic ...
}
- mergeImageFsFolder(context, containerVariant, rootDir, onProgress, true);
- return success;
+
+ if (success) {
+ mergeImageFsFolder(context, containerVariant, rootDir, onProgress, true);
+ }
+ return success;
}Additionally, line 156 has a redundant == true comparison that can be simplified:
- if (Arrays.asList(context.getAssets().list("")).contains(imagefsFile) == true){
+ if (Arrays.asList(context.getAssets().list("")).contains(imagefsFile)) {🤖 Prompt for AI Agents
In `@app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java` around
lines 141 - 190, The extractImageFs method currently always calls
mergeImageFsFolder even when extraction failed; change it to only call
mergeImageFsFolder if extraction succeeded or the variantDir actually exists
(e.g., if (success || variantDir.exists()) { mergeImageFsFolder(...) }), so you
don't merge an empty/nonexistent folder; also simplify the contains check by
removing the redundant "== true" in the Arrays.asList(...).contains(imagefsFile)
condition. Ensure you reference extractImageFs, variantDir, success, and
mergeImageFsFolder when making the edits.
|
Is this ready for merge? :) |
|
If so, can you please check that using proton 10 arm64ec and also importing custom wine continues to work? |
|
So the symlink technique was not working? |
i tried importing proton 10 arm64ec it works on bionic container... but i don't know how to import custom wine (and where to download the files 🤔 i tried files from https://github.com/K11MCH1/Winlator101/releases/tag/wine_col but they are not for bionic?) if it works then this pr is ready for merge
i tried the symlink method, opening a game will show a black screen and the log said sth similar to steamclient_loader.exe not found...(log already lost...) maybe i missed some handling? the symlink structure is looking like this: files
|
|
Oh, can you show me the error? We may need to extract the drm tzst
Utkarsh
…On Thu, Feb 5, 2026, 7:42 AM darkcl ***@***.***> wrote:
*darkcl* left a comment (utkarshdalal/GameNative#484)
<#484 (comment)>
If so, can you please check that using proton 10 arm64ec and also
importing custom wine continues to work?
i tried importing proton 10 arm64ec it works on bionic container... but i
don't know how to import custom wine (and where to download the files 🤔 i
tried files from
https://github.com/K11MCH1/Winlator101/releases/tag/wine_col but they are
not for bionic?) if it works then this pr is ready for merge
So the symlink technique was not working?
i tried the symlink method, opening a game will show a black screen and
the log said sth similar to steamclient_loader.exe not found...(log already
lost...) maybe i missed some handling? the symlink structure is looking
like this:
files
- imagefs (link to bionic or glibc folder)
- imagefs_home (the original imagefs home folder, the steamclient is
store here)
- imagefs_opt (the original imagefs opt folder)
- bionic/imagefs
- home (link to imagefs_home)
- opt (link to imagefs_opt)
- other stuff
- glibc/imagefs
- home (link to imagefs_home)
- opt (link to imagefs_opt)
- other stuff
—
Reply to this email directly, view it on GitHub
<#484 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4SWC2MTDL37IU5RFQ5OO34KKRHVAVCNFSM6AAAAACT4IAYM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTQNJQGY4DINBVHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
i think what happened is that home contains the containers, and got overwritten because of symlinking... so maybe just simple symlinking of the imagefs folder is not enough. we may also need to symlink home or ensure that it's preserved somehow. Any ideas? Otherwise your current approach is still an improvement. |
you mean the wine error or the symlink one? for wine, it show this 
for symlink... i've already remove the branch and the log already gone, i can try again in another branch but it will take some time |
2 participants
Description
This PR solves #452, changes include:
{variant}/imagefsif needed{variant}/imagefstoimagefs/optfolder (as discussed in Make it possible to swap between glibc and bionic without reinstall #452)Test
Tested on retroid pocket 5
Before
~30s - 1min load time
before.mp4
After
~6s load time
after_2.mp4
Summary by cubic
Avoid re-extracting imagefs when switching between glibc and bionic by storing a per-variant imagefs and merging it into the root on change. Cuts load time from ~30–60s to ~6s and addresses #452.
Written for commit 17955fa. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes / Performance