chore: add validations to RootInstaller #66

laur89 · 2025-04-20T15:33:47Z

add extra sanity check to mount script - bail if package is still mounted after unmount execution
RootInstaller:
- install():
  - ensure mount script installation exits w/ 0;
  - ensure mount script execution exits w/ 0;
- uninstall():
  - bail if package-to-be-unmounted is not mounted; indicates likely error on user part
  - ensure unmount script exits w/ 0;
related to feat: Improve unmount validation revanced-cli#362

- add extra sanity check to mount script - bail if package is still mounted after unmount execution - RootInstaller: - install(): - ensure mount script installation exits w/ 0; - ensure mount script execution exits w/ 0; - uninstall(): - bail if package-to-be-unmounted is not mounted; indicates likely error on user part - ensure unmount script exits w/ 0; - related to ReVanced/revanced-cli#362

src/commonMain/kotlin/app/revanced/library/installation/installer/Constants.kt

laur89 · 2025-04-20T15:45:13Z

src/commonMain/kotlin/app/revanced/library/installation/installer/RootInstaller.kt

+        if (MOUNT_GREP(packageName)().exitCode != 0) {
+            throw NotMountedException()
+        }


This is to fail if user tries to unmount a pkg that's not mounted to begin with. IMHO indicates a user error that should be marked as such.

As a downside makes the unmount command non-idempotent. Still has my vote though.

I think, if the user tries to uninstall an app that isn't installed, it's better to do it in good terms than in bad. An error is a bad term. Uninstallation finishing without doing anything, because nothing is mounted is a good term.

laur89 · 2025-04-20T15:46:18Z

src/commonMain/kotlin/app/revanced/library/installation/installer/RootInstaller.kt

@@ -71,13 +68,17 @@ abstract class RootInstaller internal constructor(
    }

    override suspend fun uninstall(packageName: String): RootInstallerResult {
+        packageName.assertInstalled()


Attempting to unmount a pkg that's not even installed should surely throw.

This line would need to be present in all implementations of Installer. This doesn't sound correctly abstracted

oSumAtrIX · 2025-04-20T18:18:37Z

src/commonMain/kotlin/app/revanced/library/installation/installer/Constants.kt

    const val MOUNT_APK =
-        "base_path=\"$MOUNTED_APK_PATH\" && " +
+        "base_path='$MOUNTED_APK_PATH' && " +
            "mv $TMP_FILE_PATH \$base_path && " +
            "chmod 644 \$base_path && " +
            "chown system:system \$base_path && " +
            "chcon u:object_r:apk_data_file:s0  \$base_path"


Could use multiline string here and in other constants so that && can be removed, the line length reduced and readability improved. Also escaping " wont be needed anymore. .trimIndent() should be used then.

Already did that change and then reverted it. Think current code is readable enough and &&-chaining has the benefit of failing fast and returning the failure code.

Single quotes for base_path var are ok here, as everything is expanded on java side, no need for shell-side expansion anyway.

and &&-chaining has the benefit of failing fast and returning the failure code.

What do you mean? Wouldn't running the commands in a new line also return the failure code

Short multiple lines rather than a single long one is more readable, ideally this is applied here.

Wouldn't running the commands in a new line also return the failure code

By default, without the set -e (which as discussed somewhere else I don't know whether shell on android supports), no. E.g.

command1 command2 command3

would return 0 if only command2 failed.

Why not if command 1 or 3 failed?

If only command1 and/or command2 failed, then the script would succeed as its exit code would be that of command3's - meaning successful.

Doesn't command1 failing exit the script? From what I know if a line fails the lines below arent run.

Not familiar with android, but in dash or bash, with default settings - no. That's what the set -e is for.

src/commonMain/kotlin/app/revanced/library/installation/installer/Constants.kt

oSumAtrIX · 2025-04-20T18:22:41Z

src/commonMain/kotlin/app/revanced/library/installation/installer/RootInstaller.kt

        val packageName = apk.packageName?.also { it.assertInstalled() } ?: throw PackageNameRequiredException()
+        logger.info("Installing ${apk.packageName} by mounting")


The throw should happen after the log as it would indicate that the attempt of the installation failed. This way this log would never occur when the throw would happen.

I'd rather not see that log message prior to exception, as that can leave user confused as to whether any installation took place or not.

I'd rather not see that log message prior to exception, as that can leave user confused as to whether any installation took place or not.

The exception is raised. If not caught without logging anything, after the installation log, the exception will be shown. Otherwise the exception will be shown prior to letting the user know it occurred while installation.

oSumAtrIX · 2025-04-20T18:23:52Z

src/commonMain/kotlin/app/revanced/library/installation/command/AdbShellCommandRunner.kt

@@ -41,10 +41,14 @@ class AdbShellCommandRunner : ShellCommandRunner {
            override fun waitFor() {
                process.waitFor()
            }
+
+            override fun ensureSuccess() {
+                if (exitCode != 0) throw ShellCmdFailure()


There is already a exitCode field. This function is not necessary in this class. You can add this to the invoke extension if necessary.

Let's continue discussion about it here

src/commonMain/kotlin/app/revanced/library/installation/command/AdbShellCommandRunner.kt

oSumAtrIX · 2025-04-20T18:28:56Z

src/commonMain/kotlin/app/revanced/library/installation/command/AdbShellCommandRunner.kt

@@ -56,4 +60,6 @@ class AdbShellCommandRunner : ShellCommandRunner {
     * @param targetFilePath The target file path.
     */
    override fun move(file: File, targetFilePath: String) = device.push(file, RemoteFile(targetFilePath))
+
+    internal class ShellCmdFailure internal constructor() : Exception("Shell command execution failure")


I don't believe the command runner classes should raise exceptions. A shell returning 0 and 1 is both controlled and expected behaviour. The classes using this such as the installers can treat 1 and 0 exits accordingly and handle it by raising an exception if according to requirement.

This exception and ensureSuccess() were created for convenience.

I suppose instead of executing commands such as

UMOUNT(packageName)().ensureSuccess()

perhaps we should change invoke extension to enforce every command to return w/ exit code 0?

Then again, workflows such as

if (MOUNT_GREP(packageName)().exitCode != 0) { throw NotMountedException() }

would then become impossible.

So in short, my vote is still on ensureSuccess() in its current form. Open to suggestions.

src/commonMain/kotlin/app/revanced/library/installation/installer/Constants.kt

oSumAtrIX · 2025-04-23T11:48:11Z

src/commonMain/kotlin/app/revanced/library/installation/installer/RootInstaller.kt

        val packageName = apk.packageName?.also { it.assertInstalled() } ?: throw PackageNameRequiredException()
+        logger.info("Installing ${apk.packageName} by mounting")


I'd rather not see that log message prior to exception, as that can leave user confused as to whether any installation took place or not.

The exception is raised. If not caught without logging anything, after the installation log, the exception will be shown. Otherwise the exception will be shown prior to letting the user know it occurred while installation.

oSumAtrIX · 2025-04-23T11:50:37Z

src/commonMain/kotlin/app/revanced/library/installation/installer/RootInstaller.kt

@@ -71,13 +68,17 @@ abstract class RootInstaller internal constructor(
    }

    override suspend fun uninstall(packageName: String): RootInstallerResult {
+        packageName.assertInstalled()


This line would need to be present in all implementations of Installer. This doesn't sound correctly abstracted

oSumAtrIX · 2025-04-23T11:52:58Z

src/commonMain/kotlin/app/revanced/library/installation/installer/RootInstaller.kt

+        if (MOUNT_GREP(packageName)().exitCode != 0) {
+            throw NotMountedException()
+        }


I think, if the user tries to uninstall an app that isn't installed, it's better to do it in good terms than in bad. An error is a bad term. Uninstallation finishing without doing anything, because nothing is mounted is a good term.

oSumAtrIX · 2025-04-23T11:55:16Z

src/commonMain/kotlin/app/revanced/library/installation/command/RunResult.kt

+     *
+     * @throws ShellCommandRunnerException if given [RunResult] exited unsuccessfully.
+     */
+    fun ensureSuccess() {}


I think we don't need this as well as the exception because this looks like a high level API that isn't specific to commands or command runs. The installers can check for the exit code themselves or you can add a helper function to them to raise an exception.

laur89 · 2025-04-23T12:07:58Z

Sounds like a dud, closing.

oSumAtrIX · 2025-04-23T14:24:04Z

Some changes looked fine, may be worth it to keep the PR open with those changes

laur89 commented Apr 20, 2025

View reviewed changes

oSumAtrIX changed the base branch from main to dev April 20, 2025 16:51