Conversation
Introduce Magisk-based installation support: add an abstract MagiskInstaller implementing install/uninstall/getInstallation logic for deploying patched APKs as Magisk modules (writes module.prop, copies patched APK to module, restarts/kills apps). Add platform-specific installers: AdbMagiskInstaller (ADB root) and LocalMagiskInstaller (local root via LocalShellCommandRunner, Closeable). Add Magisk-related constants (module paths, COPY_APK_TO_MODULE command, MAGISK_MODULE_PROP template) used by the installer.
| rm -f "$DIR/log" | ||
|
|
||
| { | ||
| echo "Induction check for $package_name" |
There was a problem hiding this comment.
What does induction check mean
There was a problem hiding this comment.
Basically making sure that the system is ready for the app to be injected/introduced into the system by overlaying on top of the original app
There was a problem hiding this comment.
Just a mix of both, right now that whole name has been replaced by the watchdog and STAGE_APK variables
There was a problem hiding this comment.
Standardized naming on these commits
secp192k1@041519a
secp192k1/revanced-manager@8f8efa5
|
|
||
| @Suppress("MemberVisibilityCanBePrivate") | ||
| internal object Constants { | ||
| object Constants { |
There was a problem hiding this comment.
Why isnt this internal anymore?
There was a problem hiding this comment.
Why isnt this internal anymore?
Manager using it on RootInstaller.kt
| } | ||
|
|
||
| /* | ||
| * When bind-mounting a patched APK over the unpatched/stock APK, Android's PM |
There was a problem hiding this comment.
Why is this commented out (this should be handled by root installer if not already)
There was a problem hiding this comment.
Why is this commented out (this should be handled by root installer if not already)
First, no, its not being handled in by root installer (.install())
For the Magisk module path this is fine since pm install lets the PM extract native libs automatically, however, for the bind-mount path, if a patch introduces a new .so native lib that wasnt in the stock APK, it will crash with UnsatisfiedLinkError (logcat shown before)
I left that function commented-out as a reference for when that's addressed, but when it is, it will belong in root install. But before addressing any of this, remember that writing to the app's lib directory will fail on read-only system partitions (/system/app, /product/app, etc)
| * | ||
| * @throws PackageNameRequiredException If the [Apk] does not have a package name. | ||
| */ | ||
| override suspend fun install(apk: Apk): RootInstallerResult { |
There was a problem hiding this comment.
Seems like this doesnt handle native libs mounting, like the extractNativeLibs utils function from earlier?
There was a problem hiding this comment.
Seems like this doesnt handle native libs mounting, like the extractNativeLibs utils function from earlier?
More detailed comment here #127 (comment)
But basically since we switched to PM for mounting, its not needed, it will be needed for bind-mounting though
| CREATE_INSTALLATION_PATH().waitFor() | ||
| MOUNT_APK(packageName)().waitFor() | ||
| CREATE_INSTALLATION_PATH(packageName)().waitFor() | ||
| prepareApk(packageName) |
There was a problem hiding this comment.
Cant prepare apk handle CREATE_INSTALLATION_PATH?
There was a problem hiding this comment.
Like, in the constant it moves a file to a folder, but then it can make sure to create the folder if it doesnt exist.
There was a problem hiding this comment.
There was a problem hiding this comment.
Cant prepare apk handle CREATE_INSTALLATION_PATH?
Seems like it already does that?
Yes true my bad, its already running the whole creating dirs command before moving the file, so CREATE_INSTALLATION_PATH was useless here, removed on d1f1076
| override suspend fun uninstall(packageName: String): RootInstallerResult { | ||
| logger.info("Uninstalling $packageName Magisk module") | ||
|
|
||
| val formattedPackageName = packageName.replace('.', '_') |
There was a problem hiding this comment.
In which case would . be unsafe to use here? Isnt
/my.pkg.name/ a valid folder name
There was a problem hiding this comment.
In which case would . be unsafe to use here? Isnt
/my.pkg.name/ a valid folder name
Yes its a valid folder name, but because formattedPackageName doubles as the Magisk module id in module.prop (id=revanced___FORMATTED_PKG__), which by convention should only contain alphanumeric characters and underscores
Since the module id must match the directory name under the modules dir (/data/adb/modules/), both are kept consistent and dot-free
| val modulePath = MODULE_PATH(formattedPackageName) | ||
|
|
||
| // Read the flag before removing the module directory. | ||
| val newlyInstalled = EXISTS("$modulePath/.newly_installed")().exitCode == 0 |
There was a problem hiding this comment.
Move this val to where its used (above the if).
There was a problem hiding this comment.
Move this val to where its used (above the if).
The val has to stay where it is, .newly_installed is inside modulePath, which is deleted a few lines later by DELETE(modulePath), moving the check to just above the if means the directory is already gone and EXISTS always returns false, so the app would never get uninstalled. The early read is intentional ("// Read the flag before removing the module directory.")
| return RootInstallation( | ||
| INSTALLED_APK_PATH(packageName)().output.ifEmpty { null }, | ||
| patchedApkPath, | ||
| MOUNT_GREP(patchedApkPath)().exitCode == 0, |
There was a problem hiding this comment.
Any reason we still have the mounted boolean https://github.com/ReVanced/revanced-library/pull/127/changes#r3061197338
There was a problem hiding this comment.
Any reason we still have the mounted boolean https://github.com/ReVanced/revanced-library/pull/127/changes#r3061197338
Lmao... 2d19f45
Also the val above as useless so I moved it inside the root installation call
| #!/system/bin/sh | ||
| pm uninstall "__PATCHED_PKG__" | ||
| rm -f "/data/adb/revanced/__PKG_NAME__.apk" | ||
| rm -f "/data/adb/service.d/revanced_handle_disabled___FORMATTED_PKG__.sh" |
There was a problem hiding this comment.
Shouldnt the module uninstall script delete itself too?
There was a problem hiding this comment.
Shouldnt the module uninstall script delete itself too?
Nope. Magisk runs the uninstall script and then removes the entire module directory itself, so the script is cleaned up automatically. The explicit rm -f for the handle-disabled script is necessary precisely because that file lives outside the module directory at /data/adb/service.d/ : Magisk will never touch it
| cp /proc/sys/kernel/random/boot_id "${DIR}/.boot_token" | ||
|
|
||
| LOG="${DIR}/log" | ||
| MAX_LOG_LINES=200 |
There was a problem hiding this comment.
Why do we need logging here? Is it to display stuff in magisk manager?
There was a problem hiding this comment.
I think its fine to not have any logging code for the module script, like logging to magisk manager is fine, but no dump to a log file
There was a problem hiding this comment.
Why do we need logging here? Is it to display stuff in magisk manager?
No, this logging is strictly for the magisk module, since manager wouldn't be able to access the logs and to keep it organized by category (manager logs vs module logs) its also keeps the logs separated between multiple modules
I think its fine to not have any logging code for the module script, like logging to magisk manager is fine, but no dump to a log file
Any other alternative that comes to mind?
Also keep in mind, along side with my reply above, each log file has a MAX_LOG_LINES=200
| # (install-create/write/commit) and is less prone to early-boot binder failures. | ||
| # NOTE: On Xiaomi devices (MIUI/HyperOS), pm install may still fail due to | ||
| # package verification restrictions. Manual install via ReVanced Manager | ||
| # may be required in that case. |
There was a problem hiding this comment.
Inside lol, how do we handle that in manager tbh or like how tf do you prompt the user to install the apk on Xiamoi programatically, ideally library can handle Xiomi installation business logic by itself
There was a problem hiding this comment.
Inside lol, how do we handle that in manager tbh or like how tf do you prompt the user to install the apk on Xiaomi programatically, ideally library can handle Xiomi installation business logic by itself
How basically everyone else solves this issue, by the phone props, for example:
[ro.fota.oem]: [Xiaomi]
[ro.product.bootimage.manufacturer]: [Xiaomi]
[ro.product.manufacturer]: [Xiaomi]
[ro.product.odm.manufacturer]: [Xiaomi]
[ro.product.product.brand]: [Xiaomi]
[ro.product.product.manufacturer]: [Xiaomi]
[ro.product.system.brand]: [Xiaomi]
[ro.product.system.manufacturer]: [Xiaomi]
[ro.product.system_ext.brand]: [Xiaomi]
[ro.product.system_ext.manufacturer]: [Xiaomi]
[ro.product.vendor.manufacturer]: [Xiaomi]
[ro.product.vendor_dlkm.manufacturer]: [Xiaomi]
Note: Remember that these are keys from my device with the value specifically being Xiaomi, some keys like ...vendor_dlkm... might not exist on other Xiaomi devices
Also this will almost always be a hassle to completely fix since there isnt a good and reliable way to tell MIUI and HyperOS apart (without a lot of checks)
Co-authored-by: oSumAtrIX <johan.melkonyan1@web.de>
…anced-library into feat/magisk-module
4 participants
Closes #127
Note: I had to rollback the AGP version during local development and testing to
8.13.2since there is a version mismatch. But this shouldn't be an issue once they match