revanced-library icon indicating copy to clipboard operation
revanced-library copied to clipboard

fix: Use Magisk mirror only when it is really possible

Open energypatrikhu opened this issue 8 months ago • 11 comments

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.

energypatrikhu avatar Apr 20 '25 21:04 energypatrikhu

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
    

laur89 avatar Apr 21 '25 01:04 laur89

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.

energypatrikhu avatar Apr 21 '25 02:04 energypatrikhu

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.

oSumAtrIX avatar May 22 '25 13:05 oSumAtrIX

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

energypatrikhu avatar May 22 '25 13:05 energypatrikhu

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

oSumAtrIX avatar May 22 '25 13:05 oSumAtrIX

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?

energypatrikhu avatar May 22 '25 13:05 energypatrikhu

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

laur89 avatar May 22 '25 13:05 laur89

should be enough, no?

I guess so

oSumAtrIX avatar May 22 '25 14:05 oSumAtrIX

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

energypatrikhu avatar May 22 '25 15:05 energypatrikhu

yes

oSumAtrIX avatar May 22 '25 15:05 oSumAtrIX

Added back check for magisk installation (https://github.com/ReVanced/revanced-library/pull/67/commits/0820352259ff57df9ed6d340cd4bd24ebb12c470), 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?

energypatrikhu avatar Jun 03 '25 16:06 energypatrikhu