Skip to content

fix: Use Magisk mirror only when it is really possible #67

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

energypatrikhu
Copy link

Updated the mount script's magisk mirror check, because when using the mount argument via the CLI, the mount script fails with a 'No such file or directory' error.

@laur89
Copy link

laur89 commented Apr 21, 2025

Haven't used magisk for a few years now, but their docs suggest current implementation is sound.

If it's really the mirror that's not existing, then either

  1. we should raise it w/ Magisk project, or;
  2. make the check safer by adding to line #58 something like:
    [ -e "${'$'}MIRROR" ] || unset MIRROR

@energypatrikhu
Copy link
Author

energypatrikhu commented Apr 21, 2025

In general what should the mirror folder contain?
In the docs it says it is used as Partition mirrors, but as for me it is empty..

Shouldn't we check if it is there AND has content?

Currently it is checked as a FILE not a directory too,
this code snippet was copied from the revanced-manager's mount script.

@oSumAtrIX
Copy link
Member

It is empty for me as well. Not sure why it is empty though. I guess we can check the contents of mirror folder. If it is empty, unset it. Please move the code block back to its original location at the bottom though.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@energypatrikhu
Copy link
Author

As a temp fix for my own project, I added a check for that, if it's good maybe I can add it to the code?
I check if the mirror folder don't exists or if it is, but it's empty

MAGISKTMP="$(magisk --path)" || MAGISKTMP=/sbin
MIRROR="$MAGISKTMP/.magisk/mirror"
if [ ! -d "$MIRROR" ] || [ -z "$(ls -A "$MIRROR" 2>/dev/null)" ]; then
    MIRROR=""
fi

@oSumAtrIX
Copy link
Member

Can't you just check if it is not empty? Because if it doesn't exist, then it will return false anyways

@energypatrikhu
Copy link
Author

Ohh that's true, then the folder check is not required here,
then using

if [ -z "$(ls -A "$MIRROR" 2>/dev/null)" ]; then

should be enough, no?

Or is there a better way to check if it's empty?

@laur89
Copy link

laur89 commented May 22, 2025

is_dir_empty() {
  find -L "$1" -mindepth 1 -maxdepth 1 -print -quit | grep -q .
  [ $? -eq 1 ]
}

@oSumAtrIX
Copy link
Member

should be enough, no?

I guess so

@energypatrikhu
Copy link
Author

Should I update the code to use the folder contents check instead of the folder exists check?

@oSumAtrIX
Copy link
Member

yes

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…existence of the folder
@oSumAtrIX oSumAtrIX changed the base branch from main to dev May 27, 2025 13:28
Comment on lines 59 to 62
MIRROR="${'$'}MAGISKTMP/.magisk/mirror"
if [ -z "$(ls -A "${'$'}MIRROR" 2>/dev/null)" ]; then
MIRROR=""
fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no reason to first set MIRROR and then unset it again. Invert the if condition and only set the variable then.

Copy link
Author

@energypatrikhu energypatrikhu May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inverting the if check means we would have to write the path twice

then this would change

MIRROR="${'$'}MAGISKTMP/.magisk/mirror"
if [ -z "$(ls -A "${'$'}MIRROR" 2>/dev/null)" ]; then
    MIRROR=""
fi

to

if [ -n "$(ls -A "${'$'}MAGISKTMP/.magisk/mirror" 2>/dev/null)" ]; then
    MIRROR="${'$'}MAGISKTMP/.magisk/mirror"
fi

as we need to check if the .magisk/mirror is not empty.

I think the how it is currently is good because it avoids the need to write the path twice,
but if you prefer the inverted logic I can change it.

@oSumAtrIX oSumAtrIX changed the title fix: Update magisk mirror check to fix 'No such file or directory' error fix: Use Magisk mirror only when it is really possible May 27, 2025
@oSumAtrIX oSumAtrIX self-requested a review May 28, 2025 19:36
energypatrikhu and others added 2 commits May 28, 2025 22:18

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
Co-authored-by: oSumAtrIX <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@energypatrikhu
Copy link
Author

Added back check for magisk installation (0820352), because using this script with other root methods would fail, removing it was my bad.

Also probably would have to fix RV Manager too, as there is no check for magisk installation either, and fix check at Line 138 too.
Should I create a PR for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants