-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
base: dev
Are you sure you want to change the base?
Conversation
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
|
In general what should the mirror folder contain? Shouldn't we check if it is there AND has content? Currently it is checked as a FILE not a directory too, |
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. |
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? MAGISKTMP="$(magisk --path)" || MAGISKTMP=/sbin
MIRROR="$MAGISKTMP/.magisk/mirror"
if [ ! -d "$MIRROR" ] || [ -z "$(ls -A "$MIRROR" 2>/dev/null)" ]; then
MIRROR=""
fi |
Can't you just check if it is not empty? Because if it doesn't exist, then it will return false anyways |
Ohh that's true, then the folder check is not required here, 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? |
is_dir_empty() {
find -L "$1" -mindepth 1 -maxdepth 1 -print -quit | grep -q .
[ $? -eq 1 ]
} |
I guess so |
Should I update the code to use the folder contents check instead of the folder exists check? |
yes |
…existence of the folder
src/commonMain/kotlin/app/revanced/library/installation/installer/Constants.kt
Outdated
Show resolved
Hide resolved
MIRROR="${'$'}MAGISKTMP/.magisk/mirror" | ||
if [ -z "$(ls -A "${'$'}MIRROR" 2>/dev/null)" ]; then | ||
MIRROR="" | ||
fi |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/commonMain/kotlin/app/revanced/library/installation/installer/Constants.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: oSumAtrIX <[email protected]>
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. |
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.