feat!: switch to arsclib #165
Conversation
There was a problem hiding this comment.
- Please finish the missing documentation.
- Some classes and implementations feel like they don't belong in ReVanced Patcher, mainly because they only serve the purpose of handling resources. The package
app.revanced.patcher.arsc, for example, mainly hosts code to read and write resources. In contrast, only a fraction is necessary for ReVanced Patcher being the classReVancedApkArchiveat max.
src/test/kotlin/app/revanced/patcher/usage/resource/patch/ExampleResourcePatch.kt
Outdated
Show resolved
Hide resolved
| classFile.realName | ||
| .let { it.substring(0, it.length - (CLASS_FILE_EXTENSION_LENGTH + 1)) } // remove file extension | ||
| .replace('/', '.') // file path to package name |
There was a problem hiding this comment.
Currently, this is a hack. Can a specification be relied on to filter a JAR archive for class files?
There was a problem hiding this comment.
I don't know of any specifications for this. I have changed it so it is closer to the way it was before.
| fun readText() = String(contents) | ||
| fun writeText(string: String) { | ||
| contents = string.toByteArray() | ||
| } |
There was a problem hiding this comment.
Oh right these make more sense as extension functions in the patches repo. I will move them.
Patches used the read/writeText extension functions of java.io.File to perform string manipulation on XML files. That is why they exist
There was a problem hiding this comment.
A resource file is not yet an XML file. Justifying the existence of these methods by saying resource files are XML files wouldn't be really correct. Classes such as String() should not be visible at patch level as they are low level classes for resources. For that reason moving the function readText to patches should be reconsidered.
There was a problem hiding this comment.
There can be other text files such as json in an apk too, and those are also text. The current implementation just transparently decodes/encodes android binary xml to human readable xml, just like aapt does. Do you think there is there a problem with that?
There was a problem hiding this comment.
I am highlighting your intention to move the methods to the patches, therefore exposing the String() class publicly to the patches, whereas it should not be visible at the patch level as they are low-level classes for resources. This is why moving the function readText to patches should be reconsidered
There was a problem hiding this comment.
I am not entirely sure what you mean. readText is just there to read the entire resource file as a normal kotlin string. String is not a custom class or anything.
| } | ||
| } | ||
|
|
||
| return if (resPath.endsWith(".xml")) ResourceFileImpl.XML( |
There was a problem hiding this comment.
This is loose. Resource files could also end without an extension.
There was a problem hiding this comment.
It now reads existing files in order to see if they are AXML or not and only uses file extension when creating new files. Is this sufficient?
|
Shouldn't this branch merge into the |
Split branch is out of date and this PR supersedes it. #113 should be closed if this PR gets merged |
| * The result of a patch. | ||
| * | ||
| * @param apk The patched [Apk] file. | ||
| */ | ||
| sealed class Patch(val apk: Apk) { |
There was a problem hiding this comment.
Yeah, that doesn't make sense. This is not the result of a single Patch, this is the finalized APK "bundle" the patcher came up with. Please adjust this naming accordingly, such as PatchedApk.
There was a problem hiding this comment.
That naming suggests that PatchedApk is Apk which isn't correct, please suggest another name
There was a problem hiding this comment.
@Sculas do you have a better name?
| } | ||
| } | ||
|
|
||
| inner class Resources(val tableBlock: TableBlock?) { |
There was a problem hiding this comment.
How about ResourceContainer?
| inner class Resources(val tableBlock: TableBlock?) { | |
| inner class ResourceContainer(val tableBlock: TableBlock?) { |
There was a problem hiding this comment.
See my earlier comment about PatchException: #165 (comment)
src/main/kotlin/app/revanced/patcher/util/proxy/mutableTypes/MutableMethod.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/app/revanced/patcher/util/xml/LazyXMLInputSource.kt
Outdated
Show resolved
Hide resolved
|
@oSumAtrIX This one too. |
|
PR should be renamed to a breaking change, btw. |
|
I have added documentation and moved some code around. Almost all the other reviews are marked as resolved, which is why I am requesting another one. |
| { file.close() }) | ||
|
|
||
| /** | ||
| * Closes the editor. Write backs and decreases the lock count. |
There was a problem hiding this comment.
| * Closes the editor. Write backs and decreases the lock count. | |
| * Closes the editor, writes back the file, and decreases the lock count. |
There was a problem hiding this comment.
Is lock count still up to date here.
| /** | ||
| * Wrapper for a file that can be edited as a dom document. | ||
| * | ||
| * This constructor does not check for locks to the file when writing. |
There was a problem hiding this comment.
I don't see any related code to locking
There was a problem hiding this comment.
Looks like I forgot to remove the documentation related to locking after I remove the lock itself.
I removed those docs and simplified the save code because we don't have to do any OutputStream workarounds unlike with real files.
| SplitApkResult(it, file, exception) | ||
| } | ||
|
|
||
| inner class GlobalResources { |
There was a problem hiding this comment.
Why is this class part of an ApkBundle and what is it used for? Please document it.
| } | ||
|
|
||
| /** | ||
| * The global resource container. |
There was a problem hiding this comment.
Please clarify what a global resource container for ApkBundle means.
|
@oSumAtrIX the commits you pushed look good to me other than the |
BREAKING CHANGE: This removes the previously available `Path` option
arsclib-utils/build.gradle.kts
Outdated
| publishing { | ||
| repositories { | ||
| if (System.getenv("GITHUB_ACTOR") != null) | ||
| maven { | ||
| name = "GitHubPackages" | ||
| url = uri("https://maven.pkg.github.com/revanced/revanced-patcher") | ||
| credentials { | ||
| username = System.getenv("GITHUB_ACTOR") | ||
| password = System.getenv("GITHUB_TOKEN") | ||
| } | ||
| } | ||
| else | ||
| mavenLocal() | ||
| } | ||
| publications { | ||
| register<MavenPublication>("gpr") { | ||
| from(components["java"]) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why does the arsclib module depend on patcher?
oSumAtrIX
force-pushed
the
insan
branch
2 times, most recently
from
1fce0c8 to
1b543b0
Compare
|
If possible, introduce some tests to the |
There was a problem hiding this comment.
This file should be in the root folder
| * @param handle The [Handle] associated with this file. | ||
| * @param archive The [Archive] that the file resides in. | ||
| */ | ||
| class ResourceFile private constructor( |
There was a problem hiding this comment.
This file should be removed in favor of the new class ArchiveResource. You use dependency inversion to inject the archive this resource belongs to and write back to it. This can be implemented in ArchiveResource too. Is it necessary to invert the dependency to Handle? Also, ArchiveResource is abstracted in a way that does not need to read all bytes to a byte array. Before my changes, you were reading all bytes from a parsed XmlDocument file, then later on parsing it again in DomFIleEditor as an w3c XML file. w3c XML files should be removed, and XmlDocument from ARSCLib should be used instead. This way, we stop depending on w3c XML as well as don't have to do all the unnecessary writing and parsing
| @@ -0,0 +1,7 @@ | |||
| package app.revanced.arsc.logging | |||
There was a problem hiding this comment.
Maybe relying on a logging facade is better than having these interfaces in patcher and arsclib-utils.
| pluginManagement { | ||
| repositories { | ||
| mavenCentral() | ||
| maven { | ||
| url = uri("https://maven.pkg.github.com/revanced/multidexlib2") | ||
| credentials { | ||
| username = providers.gradleProperty("gpr.user").orNull ?: System.getenv("GITHUB_ACTOR") | ||
| password = providers.gradleProperty("gpr.key").orNull ?: System.getenv("GITHUB_TOKEN") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| dependencyResolutionManagement { | ||
| repositoriesMode.set(RepositoriesMode.FAIL_ON_PROJECT_REPOS) | ||
| repositories { | ||
| mavenCentral() | ||
| maven { | ||
| url = uri("https://maven.pkg.github.com/revanced/multidexlib2") | ||
| credentials { | ||
| username = providers.gradleProperty("gpr.user").orNull ?: System.getenv("GITHUB_ACTOR") | ||
| password = providers.gradleProperty("gpr.key").orNull ?: System.getenv("GITHUB_TOKEN") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not entirely sure. I just used compose manager as a reference when setting up the gradle monorepo because android projects are always setup like that.
| /** | ||
| * A set of all architectures supported by android. | ||
| */ | ||
| val architectures = setOf("armeabi_v7a", "arm64_v8a", "x86", "x86_64") |
There was a problem hiding this comment.
Currently, the architectures are hard coded. What do you think of propagating the selection of the correct Apk types? This means the patcher would not automatically know the library APK architecture and instead require the library user to input the correct one. The benefit of that would be that the architecture wouldn't be hardcoded anymore.
There was a problem hiding this comment.
That just contains a set containing all architectures supported by android (not the current/target device). I don't think that list is likely to change any time soon so hardcoding probably isn't really an issue. It is used for determining if the split configuration is valid or not, so we cannot rely on the list of architectures supported by the current device (SUPPORTED_ABIS on android).
Adding the ability for frontends to filter which apks are used might make sense though.
| val module = manifestElement.searchAttributeByName("configForSplit") | ||
| ?.let { Module.DynamicFeature(it.valueAsString) } ?: Module.Main | ||
|
|
||
| // Examples: | ||
| // config.xhdpi | ||
| // df_my_feature.config.en | ||
| val config = this.split.split(".").last() |
There was a problem hiding this comment.
Are those conventions written in stone or can a language APKModule not end with a two-letter country code?
There was a problem hiding this comment.
Android uses ISO 639-1:2002 language codes and those are always two letters long.
Language variants (en_US, en_GB) are just stored in one apk (en).
| @@ -0,0 +1,42 @@ | |||
| .gradle | |||
There was a problem hiding this comment.
This file should be in the root of the repository
oSumAtrIX
force-pushed
the
dev
branch
2 times, most recently
from
b00a1b0 to
1bca84e
Compare
This comment was marked as spam.
This comment was marked as spam.
|
Iirc it's considered 20% done, there's further refactoring and things necessary |
|
I am going to close this for two reasons:
Anyone who wants to give this a shot can use my branch as a reference. |
Fixes #73
Motivation
aapt2is slow, and has several skill issues such as the inability to run properly on most armv7 devices and sometimes fails to decode properly as seen here.ARSCLib is faster, and does not experience these issues.