refactor: Symlink imagefs_shared home in the variants + migrate old home contents#1030
Open
unbelievableflavour wants to merge 1 commit intoutkarshdalal:masterfrom
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImageFsInstaller now enforces a shared backing for the imagefs Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
unbelievableflavour
force-pushed
the
home-in-imagefs-shared
branch
from
6baab36 to
bf53a6c
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 1 file
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/xenvironment/ImageFsInstaller.java">
<violation number="1" location="app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java:397">
P2: Unconditionally deleting the existing shared home before migration can wipe shared user data when multiple variants or repeated runs already populated imagefs_shared/home.</violation>
<violation number="2" location="app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java:444">
P2: ensureSharedHomeRoot will attempt to create the /home symlink even when an existing non-symlink /home directory remains. If migration fails, FileUtils.symlink will fail because it only deletes the path (non-empty directories aren’t removed), leaving /home unshared and breaking the intended shared-home invariant.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java`:
- Around line 211-213: The code calls ensureSharedHomeRoot(context,
imageFs.getRootDir()) synchronously before creating the Future in
installIfNeededFuture(), which can block the caller; move that work into the
submitted task so the method remains fully asynchronous — replace the current
sequence (ensureSharedHomeRoot(...); return
Executors.newSingleThreadExecutor().submit(() -> true);) with a single
submit(...) that runs ensureSharedHomeRoot(context, imageFs.getRootDir()) inside
the callable/runnable and then returns true (i.e.,
Executors.newSingleThreadExecutor().submit(() -> { ensureSharedHomeRoot(context,
imageFs.getRootDir()); return true; })).
- Around line 395-445: ensureSharedHomeRoot currently pre-creates and may delete
an existing sharedHomeRoot during migration; change migrate flow so you do NOT
clobber an existing sharedHomeRoot and only create/replace it when migration
succeeds. Specifically, in ensureSharedHomeRoot/ migrateLegacyHomeToShared: stop
precreating sharedHomeRoot in getImageFsSharedDir/ensureSharedHomeRoot, first
check if sharedHomeRoot exists and is non-empty and if so skip migration; when
migrating (migrateLegacyHomeToShared), attempt a safe move or copy into a
temporary directory and on success atomically rename into sharedHomeRoot (or
create sharedHomeRoot only after copy succeeds), do not delete an existing
populated sharedHomeRoot, and only create the symlink (FileUtils.symlink) from
homePathInImageFs to sharedHomeRoot after migration completed successfully and
homePathInImageFs is absent or confirmed safe to replace. Reference
ensureSharedHomeRoot, migrateLegacyHomeToShared, getImageFsSharedDir,
sharedHomeRoot, and homePathInImageFs.
🪄 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: 76b66bf7-d9be-4f58-9c26-867cd4bc3630
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/core/WineUtils.javaapp/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java
app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java
Outdated
Show resolved
Hide resolved
unbelievableflavour
force-pushed
the
home-in-imagefs-shared
branch
3 times, most recently
from
889ca5e to
741ec6a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java`:
- Around line 405-413: The copy branch in ImageFsInstaller currently leaves
partial contents in sharedHomeRoot when FileUtils.copy(legacyHome,
sharedHomeRoot) returns false; modify the failure path in the method containing
this logic so that on a failed copy you remove the partially-created
sharedHomeRoot (recursively delete sharedHomeRoot or its contents) before
returning false, and also catch and log any exception thrown during the cleanup;
reference the FileUtils.copy call, sharedHomeRoot and legacyHome to locate the
code and ensure the cleanup runs only on failure and does not run when copy
returns true.
- Around line 449-457: The code may call sharedHomeRoot.mkdirs() without
checking its result, leading to creating a symlink to a non-existent target;
update ImageFsInstaller to verify directory creation by checking the boolean
return of sharedHomeRoot.mkdirs() (and/or re-checking sharedHomeRoot.exists()
after the call), and if creation failed log an error via Log.e with context
(including sharedHomeRoot.getPath()), abort the install path (return or throw)
before calling FileUtils.symlink, so the symlink is never created when the
directory does not actually exist; locate the logic around sharedHomeRoot,
homePathInImageFs and the FileUtils.symlink call to implement this check.
🪄 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: 79f10c8c-bb1d-4357-9d9e-75958b631afe
📒 Files selected for processing (1)
app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java
app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java
Outdated
Show resolved
Hide resolved
unbelievableflavour
force-pushed
the
home-in-imagefs-shared
branch
2 times, most recently
from
abf2ec2 to
36fbd11
Compare
unbelievableflavour
changed the title
unbelievableflavour
force-pushed
the
home-in-imagefs-shared
branch
2 times, most recently
from
03afa96 to
accb3d2
Compare
unbelievableflavour
force-pushed
the
home-in-imagefs-shared
branch
from
accb3d2 to
6f62240
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Share a single user home across container variants by backing
/homewithimagefs_shared/home. Adds safe migration from legacy home and ensures the symlink is created on install and when the imagefs is already current.imagefs_shared/homeexists viaImageFs.getImageFsSharedDir()and symlink each variant’s/hometo it on fresh install and when the imagefs is already valid.imagefs/homeinto the shared path withImageFSLegacyMigrator; no-op if already a symlink, overwrite existing shared home, fallback to copy+delete, and abort install on failure.Written for commit 6f62240. Summary will update on new commits.
Summary by CodeRabbit