Skip to content

feat: Increase installation logging granularity and set process exit code properly #365

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

feat: Increase installation logging granularity and set process exit code properly #365

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
b065ef4
feat: improve (un)install stdout logging
laur89 Apr 19, 2025
ee0d8df
feat: provide (un)installer with failure exit code
laur89 Apr 20, 2025
830bd97
[WIP] address review
laur89 Apr 20, 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
26 changes: 21 additions & 5 deletions src/main/kotlin/app/revanced/cli/command/utility/InstallCommand.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import kotlinx.coroutines.runBlocking
import picocli.CommandLine.*
import java.io.File
import java.util.logging.Logger
import kotlin.system.exitProcess

@Command(
name = "install",
Expand Down Expand Up @@ -44,20 +45,35 @@ internal object InstallCommand : Runnable {
}.install(Installer.Apk(apk, packageName))
} catch (e: Exception) {
logger.severe(e.toString())
throw e
Copy link
Member

Choose a reason for hiding this comment

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

Why are we logging when throwing the exception which will be logged again?

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.

which will be logged again?

It won't be logged again. The err is rethrown only for the sake of control flow.

Copy link
Member

Choose a reason for hiding this comment

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

Since this is a binary situation, you can use return logic.

Copy link
Author

Choose a reason for hiding this comment

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

Think we should continue this discussion under the return code chain

}

when (result) {
RootInstallerResult.FAILURE ->
RootInstallerResult.SUCCESS ->
logger.info("Mounted the APK file")
Copy link
Member

Choose a reason for hiding this comment

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

"Mounting" is an installation method. I think we don't need more granularity than "install" for both cases. Mentioning that it was mounted is not necessary too for the reason that the CLI input already mentions --mount as an installation method.

Copy link
Author

Choose a reason for hiding this comment

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

'ite, both {RootInstallerResult.SUCCESS, AdbInstallerResult.Success} will log

Installed the APK file

Copy link
Author

Choose a reason for hiding this comment

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

Happy w/

RootInstallerResult.FAILURE -> {
    logger.severe("Failed to mount the APK file")

or want it changed?

RootInstallerResult.FAILURE -> {
logger.severe("Failed to mount the APK file")
is AdbInstallerResult.Failure ->
logger.severe(result.exception.toString())
else ->
throw Exception()
Copy link
Member

Choose a reason for hiding this comment

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

raising an empty exception doesn't look right to me.

Copy link
Author

Choose a reason for hiding this comment

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

Same as explained above - it's for control flow only.

Copy link
Member

Choose a reason for hiding this comment

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

In this case, you can probably just return?

Copy link
Author

Choose a reason for hiding this comment

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

Like above, we should continue this discussion under the return code chain

}
AdbInstallerResult.Success ->
logger.info("Installed the APK file")
is AdbInstallerResult.Failure -> {
logger.severe("Failed to install the APK file: ${result.exception}")
throw Exception()
}
else -> {
logger.severe("Unknown installation result")
throw Exception()
}
}
}

runBlocking {
deviceSerials?.map { async { install(it) } }?.awaitAll() ?: install()
try {
Copy link
Member

Choose a reason for hiding this comment

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

I generally find exceptions ugly/messy code. They introduce nesting and unnecessary blocks of code. Using return values seems simpler for "binary" cases. Exceptions have particular use case for raising messages or multiple kinds of cases not just true or false.

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.

If you pefer not to use exceptions for this control flow, then what do you propose?
Perhaps change InstallCommand/UninstallCommand to implement Callable instead of Runnable and return code (i.e. Int) from install() / uninstall()?

Copy link
Member

Choose a reason for hiding this comment

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

Is this the expected way of returning an exit code for PicoCLI? If so, every subcommand should be subject to this change.

Copy link
Author

@laur89 laur89 Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Honestly no idea what the common there pattern is.

deviceSerials?.map { async { install(it) } }?.awaitAll() ?: install()
} catch (_: Exception) {
exitProcess(1)
}
}
}
}
25 changes: 21 additions & 4 deletions src/main/kotlin/app/revanced/cli/command/utility/UninstallCommand.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import kotlinx.coroutines.runBlocking
import picocli.CommandLine.*
import picocli.CommandLine.Help.Visibility.ALWAYS
import java.util.logging.Logger
import kotlin.system.exitProcess

@Command(
name = "uninstall",
Expand Down Expand Up @@ -48,19 +49,35 @@ internal object UninstallCommand : Runnable {
}.uninstall(packageName)
} catch (e: Exception) {
logger.severe(e.toString())
throw e
}

when (result) {
RootInstallerResult.FAILURE ->
RootInstallerResult.SUCCESS ->
Copy link
Member

Choose a reason for hiding this comment

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

I am just noticing code duplication here and in the patch subcommand, possibly extracting into a common function makes sense.

logger.info("Unmounted the patched APK file")
RootInstallerResult.FAILURE -> {
logger.severe("Failed to unmount the patched APK file")
is AdbInstallerResult.Failure ->
throw Exception()
}
AdbInstallerResult.Success ->
logger.info("Uninstalled the patched APK file")
is AdbInstallerResult.Failure -> {
logger.severe(result.exception.toString())
else -> logger.info("Uninstalled the patched APK file")
throw Exception()
}
else -> {
logger.severe("Unknown uninstallation result")
throw Exception()
}
}
}

runBlocking {
deviceSerials?.map { async { uninstall(it) } }?.awaitAll() ?: uninstall()
try {
deviceSerials?.map { async { uninstall(it) } }?.awaitAll() ?: uninstall()
} catch (_: Exception) {
exitProcess(1)
}
}
}
}