Conversation
laur89
commented
Apr 20, 2025
laur89
commented
- 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;
- install():
- 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
Outdated
Show resolved
Hide resolved
| if (MOUNT_GREP(packageName)().exitCode != 0) { | ||
| throw NotMountedException() | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| override suspend fun uninstall(packageName: String): RootInstallerResult { | ||
| packageName.assertInstalled() |
There was a problem hiding this comment.
Attempting to unmount a pkg that's not even installed should surely throw.
There was a problem hiding this comment.
This line would need to be present in all implementations of Installer. This doesn't sound correctly abstracted
| 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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
command3would return 0 if only command2 failed.
There was a problem hiding this comment.
Why not if command 1 or 3 failed?
There was a problem hiding this comment.
If only command1 and/or command2 failed, then the script would succeed as its exit code would be that of command3's - meaning successful.
There was a problem hiding this comment.
Doesn't command1 failing exit the script? From what I know if a line fails the lines below arent run.
There was a problem hiding this comment.
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
Show resolved
Hide resolved
src/commonMain/kotlin/app/revanced/library/installation/installer/Constants.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/app/revanced/library/installation/installer/Constants.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/app/revanced/library/installation/installer/Constants.kt
Outdated
Show resolved
Hide resolved
| logger.info("Installing ${apk.packageName} by mounting") | ||
|
|
||
| val packageName = apk.packageName?.also { it.assertInstalled() } ?: throw PackageNameRequiredException() | ||
| logger.info("Installing ${apk.packageName} by mounting") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| override fun ensureSuccess() { | ||
| if (exitCode != 0) throw ShellCmdFailure() |
There was a problem hiding this comment.
There is already a exitCode field. This function is not necessary in this class. You can add this to the invoke extension if necessary.
src/commonMain/kotlin/app/revanced/library/installation/command/AdbShellCommandRunner.kt
Show resolved
Hide resolved
src/commonMain/kotlin/app/revanced/library/installation/command/AdbShellCommandRunner.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/app/revanced/library/installation/command/AdbShellCommandRunner.kt
Outdated
Show resolved
Hide resolved
| */ | ||
| override fun move(file: File, targetFilePath: String) = device.push(file, RemoteFile(targetFilePath)) | ||
|
|
||
| internal class ShellCmdFailure internal constructor() : Exception("Shell command execution failure") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
laur89
force-pushed
the
rootinstaller-add-validation
branch
from
39c78ef to
62b99e9
Compare
src/commonMain/kotlin/app/revanced/library/installation/installer/Constants.kt
Outdated
Show resolved
Hide resolved
| logger.info("Installing ${apk.packageName} by mounting") | ||
|
|
||
| val packageName = apk.packageName?.also { it.assertInstalled() } ?: throw PackageNameRequiredException() | ||
| logger.info("Installing ${apk.packageName} by mounting") |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| override suspend fun uninstall(packageName: String): RootInstallerResult { | ||
| packageName.assertInstalled() |
There was a problem hiding this comment.
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() | ||
| } |
There was a problem hiding this comment.
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.
| * | ||
| * @throws ShellCommandRunnerException if given [RunResult] exited unsuccessfully. | ||
| */ | ||
| fun ensureSuccess() {} |
There was a problem hiding this comment.
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.
|
Sounds like a dud, closing. |
|
Some changes looked fine, may be worth it to keep the PR open with those changes |
2 participants
