Skip to content

chore: add validations to RootInstaller #66

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.

Sign up for GitHub

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 3 commits into
base: dev
Choose a base branch
from
Open

chore: add validations to RootInstaller #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c1ec6f2
chore: add validations to RootInstaller
laur89 Apr 20, 2025
3cd3ade
[WIP] address review
laur89 Apr 20, 2025
62b99e9
[WIP] address review
laur89 Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ captures
.cxx
local.properties
xcuserdata
node_modules/
node_modules/
2 changes: 2 additions & 0 deletions api/android/revanced-library.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,15 @@ public final class app/revanced/library/installation/command/LocalShellCommandRu
}

public abstract interface class app/revanced/library/installation/command/RunResult {
public abstract fun ensureSuccess ()V
public abstract fun getError ()Ljava/lang/String;
public abstract fun getExitCode ()I
public abstract fun getOutput ()Ljava/lang/String;
public abstract fun waitFor ()V
}

public final class app/revanced/library/installation/command/RunResult$DefaultImpls {
public static fun ensureSuccess (Lapp/revanced/library/installation/command/RunResult;)V
public static fun waitFor (Lapp/revanced/library/installation/command/RunResult;)V
}

Expand Down
2 changes: 2 additions & 0 deletions api/jvm/revanced-library.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,15 @@ public final class app/revanced/library/installation/command/AdbShellCommandRunn
}

public abstract interface class app/revanced/library/installation/command/RunResult {
public abstract fun ensureSuccess ()V
public abstract fun getError ()Ljava/lang/String;
public abstract fun getExitCode ()I
public abstract fun getOutput ()Ljava/lang/String;
public abstract fun waitFor ()V
}

public final class app/revanced/library/installation/command/RunResult$DefaultImpls {
public static fun ensureSuccess (Lapp/revanced/library/installation/command/RunResult;)V
public static fun waitFor (Lapp/revanced/library/installation/command/RunResult;)V
}

Expand Down
6 changes: 5 additions & 1 deletion src/commonMain/kotlin/app/revanced/library/installation/command/AdbShellCommandRunner.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,14 @@ class AdbShellCommandRunner : ShellCommandRunner {
override fun waitFor() {
process.waitFor()
}

override fun ensureSuccess() {
if (exitCode != 0) throw ShellCommandRunnerException()
}
}
}

override fun hasRootPermission(): Boolean = invoke("whoami").exitCode == 0
override fun hasRootPermission(): Boolean = invoke("exit").exitCode == 0

override fun write(content: InputStream, targetFilePath: String) =
device.push(content, System.currentTimeMillis(), 644, RemoteFile(targetFilePath))
Expand Down
9 changes: 9 additions & 0 deletions src/commonMain/kotlin/app/revanced/library/installation/command/RunResult.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package app.revanced.library.installation.command

import app.revanced.library.installation.command.ShellCommandRunner.ShellCommandRunnerException

/**
* The result of a command execution.
*/
Expand All @@ -23,4 +25,11 @@ interface RunResult {
* Waits for the command to finish.
*/
fun waitFor() {}

/**
* Verifies whether the command exits with code 0.
*
* @throws ShellCommandRunnerException if given [RunResult] exited unsuccessfully.
*/
fun ensureSuccess() {}
Copy link
Member

Choose a reason for hiding this comment

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

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.

}
2 changes: 2 additions & 0 deletions src/commonMain/kotlin/app/revanced/library/installation/command/ShellCommandRunner.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,6 @@ abstract class ShellCommandRunner internal constructor() {
internal operator fun invoke(
command: String,
) = runCommand("su -c \'$command\'")

internal class ShellCommandRunnerException internal constructor() : Exception("Shell command execution failed")
}
1 change: 0 additions & 1 deletion src/commonMain/kotlin/app/revanced/library/installation/installer/AdbInstaller.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package app.revanced.library.installation.installer

import app.revanced.library.installation.command.AdbShellCommandRunner
import app.revanced.library.installation.installer.Constants.INSTALLED_APK_PATH
import app.revanced.library.installation.installer.Installer.Apk
import se.vidstige.jadb.JadbException
import se.vidstige.jadb.managers.Package
import se.vidstige.jadb.managers.PackageManager
Expand Down
2 changes: 0 additions & 2 deletions src/commonMain/kotlin/app/revanced/library/installation/installer/AdbRootInstaller.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package app.revanced.library.installation.installer

import app.revanced.library.installation.command.AdbShellCommandRunner
import app.revanced.library.installation.installer.Installer.Apk
import app.revanced.library.installation.installer.RootInstaller.NoRootPermissionException

/**
* [AdbRootInstaller] for installing and uninstalling [Apk] files with using ADB root permissions by mounting.
Expand Down
6 changes: 3 additions & 3 deletions src/commonMain/kotlin/app/revanced/library/installation/installer/Constants.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ internal object Constants {
const val CREATE_INSTALLATION_PATH = "$CREATE_DIR $MOUNT_PATH"

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"
Comment on lines 21 to 26
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Why not if command 1 or 3 failed?

Copy link
Author

@laur89 laur89 Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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


const val UMOUNT =
"grep $PLACEHOLDER /proc/mounts | " +
"$MOUNT_GREP | " +
"while read -r line; do echo \$line | " +
"cut -d ' ' -f 2 | " +
"sed 's/apk.*/apk/' | " +
Expand Down Expand Up @@ -56,7 +56,7 @@ internal object Constants {

# Use Magisk mirror, if possible.
if command -v magisk &> /dev/null; then
MIRROR="${'$'}(magisk --path)/.magisk/mirror"
MIRROR="$(magisk --path)/.magisk/mirror"
fi

mount -o bind ${'$'}MIRROR${'$'}base_path ${'$'}stock_path
Expand Down
1 change: 0 additions & 1 deletion src/commonMain/kotlin/app/revanced/library/installation/installer/Installer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package app.revanced.library.installation.installer

import app.revanced.library.installation.installer.Installer.Apk
import java.io.File
import java.util.logging.Logger

Expand Down
18 changes: 10 additions & 8 deletions src/commonMain/kotlin/app/revanced/library/installation/installer/RootInstaller.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ import app.revanced.library.installation.installer.Constants.RESTART
import app.revanced.library.installation.installer.Constants.TMP_FILE_PATH
import app.revanced.library.installation.installer.Constants.UMOUNT
import app.revanced.library.installation.installer.Constants.invoke
import app.revanced.library.installation.installer.Installer.Apk
import app.revanced.library.installation.installer.RootInstaller.NoRootPermissionException
import java.io.File

/**
Expand Down Expand Up @@ -50,9 +48,8 @@ abstract class RootInstaller internal constructor(
* @throws PackageNameRequiredException If the [Apk] does not have a package name.
*/
override suspend fun install(apk: Apk): RootInstallerResult {
logger.info("Installing ${apk.packageName} by mounting")

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

@laur89 laur89 Apr 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


// Setup files.
apk.file.move(TMP_FILE_PATH)
Expand All @@ -61,8 +58,8 @@ abstract class RootInstaller internal constructor(

// Install and run.
TMP_FILE_PATH.write(MOUNT_SCRIPT(packageName))
INSTALL_MOUNT_SCRIPT(packageName)().waitFor()
MOUNT_SCRIPT_PATH(packageName)().waitFor()
INSTALL_MOUNT_SCRIPT(packageName)().ensureSuccess()
MOUNT_SCRIPT_PATH(packageName)().ensureSuccess()
RESTART(packageName)()

DELETE(TMP_FILE_PATH)()
Expand All @@ -71,13 +68,17 @@ abstract class RootInstaller internal constructor(
}

override suspend fun uninstall(packageName: String): RootInstallerResult {
packageName.assertInstalled()
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

if (MOUNT_GREP(packageName)().exitCode != 0) {
throw NotMountedException()
}
Comment on lines +72 to +74
Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

logger.info("Uninstalling $packageName by unmounting")

UMOUNT(packageName)()
UMOUNT(packageName)().ensureSuccess()

DELETE(MOUNTED_APK_PATH)(packageName)()
DELETE(MOUNT_SCRIPT_PATH)(packageName)()
DELETE(TMP_FILE_PATH)() // Remove residual.
DELETE(TMP_FILE_PATH)()

KILL(packageName)()

Expand Down Expand Up @@ -131,4 +132,5 @@ abstract class RootInstaller internal constructor(

internal class PackageNameRequiredException internal constructor() : Exception("Package name is required")
internal class NoRootPermissionException internal constructor() : Exception("No root permission")
internal class NotMountedException internal constructor() : Exception("Package not mounted")
}