-
-
Notifications
You must be signed in to change notification settings - Fork 4
fix: Add shadow plugin #6
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.
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
base: dev
Are you sure you want to change the base?
Conversation
|
||
// JarTask includes the javadoc and sourcesjar. Be sure to only add the "-thin" filename suffix, if there isn't already one set | ||
if (it.archiveClassifier.orNull == null) { | ||
it.archiveClassifier.set("thin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shaded jar has the classifier set to "null" (see below). Therefore it has the filename "patches-5.30.0.rvp". This could conflict with the jar task. I therefore set the archiveClassifier to "thin", which results in the filename "patches-5.30.0-thin.rvp" if somebody runs "./gradlew jar" (the non-shaded jar, it is not run by default). I first check if it is null, if not it is the "javadocJar" (patches-5.30.0-javadoc.rvp) and "sourcesJar" (patches-5.3.0.0-sources.rvp) which should not be affected by this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it has the "all" classifier by default. I changed that and removed it, so "revanced-patches-5.30.0.rvp" is the shadowJar and "revanced-patches-5.30.0-thin.rvp" is the normal jar (which is not expected to be built by default).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the JAR task and shadowJar task should produce the same artifact name, even if overwriting each other. It is the "expected" output. "thin" would be an arbitrary change from our end. As long as the build task does not run the jar task it should be fine. If the user wants they can run the jar task and it would overwrite the shadow jar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the JAR task and shadowJar task should produce the same artifact name
I don't think it has an advantage. It doesn't hurt that there is a clear difference between the two files. It could create confusion if it has the same filename (and maybe even more issues / bug reports etc., although I think that shouldn't be noticable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shadowJar should IMO just be a configuration of jar e.g. jar.shadow = true.
Not producing the same artifact causes issues like having to configure the API regex, naming decisions and confusion in artifacts generated, their differences and the likes. If you generate a shadow you don't need the thin jar or vice versa. Overriding doesn't change anything else in other systems and spares said issues on its way.
configurations.getByName("compileClasspath").extendsFrom(shade) | ||
configurations.getByName("runtimeClasspath").extendsFrom(shade) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain the purpose of these lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created the "shade" configuration (I explained why below). By default, it does nothing.
Meaning, it isn't present in the compileClasspath (present at compile time, self explanatory I guess), and not present in runtimeClasspath (present at runtime, for a javaExec task for example).
By adding it to the compileClasspath and runtimeClasspath, "shade" acts the same way as "implementation"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would "shade" be needed in runtime or compiletime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it wouldn't be in the compileClasspath:
e: patches/all/misc/spoof/SignatureSpoofPatch.kt:6:20 Unresolved reference 'apksig'.
e: patches/all/misc/spoof/SignatureSpoofPatch.kt:7:20 Unresolved reference 'apksig'.
e: patches/all/misc/spoof/SignatureSpoofPatch.kt:76:24 Unresolved reference 'ApkVerifier'.
e: patches/all/misc/spoof/SignatureSpoofPatch.kt:81:71 Unresolved reference 'certificate'.
e: patches/all/misc/spoof/SignatureSpoofPatch.kt:83:71 Unresolved reference 'certificate'.
e: patches/all/misc/spoof/SignatureSpoofPatch.kt:85:71 Unresolved reference 'certificate'.
e: patches/all/misc/spoof/SignatureSpoofPatch.kt:91:17 Unresolved reference 'ApkFormatException'.
You can ask the same question with any other dependency used.
For example, why is "revanced-patcher" inside the compileClasspath (via "implementation).
Answer: So Java/Kotlin knows it exists and can compile it.
For the runtimeClasspath:
This is technically only relevant if "JavaExec" is used. Here are all dependencies that are expected to be present at runtime (= when the patches are executed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created the "shade" configuration (I explained why below)
I couldn't find the explanation below. Why did you create a separate configuration?
The plugin already adds a "shade()" API as seen here: https://gradleup.com/shadow/configuration/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Dependencies added to the shadow configuration are not bundled into the output JAR." Thats blacklisting.
The "shade" configuration contains the ones that are getting bundled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Dependencies added to the shadow configuration are not bundled into the output JAR." Thats blacklisting.
The "shade" configuration contains the ones that are getting bundled.
But I could also just use the "shadow" configuration to blacklist "revanced-patcher" and "smali" and everything else with "implementation" would get shaded in? If you prefer this way, let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using the default APIs is better. As the plugin configures the patches project, the plugin can use the shadow APIs to exclude the APIs such as patcher, library or co.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is also "guava" inside the revanced-patches build.gradle.kts? Is there any reason for it being there and not hardcoded in the plugin? Would also need to be excluded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kotlin plugin automatically adds dependencies, which shouldn't be shaded in, which makes excluding harder.
it.configurations = listOf(shade) | ||
it.archiveExtension.set("rvp") | ||
it.archiveClassifier.set(null as String?) | ||
|
||
it.minimize() | ||
it.isEnableRelocation = true | ||
it.relocationPrefix = "app.revanced.libs" | ||
} | ||
tasks.named("assemble") { | ||
it.setDependsOn(listOf(tasks.named("shadowJar"), tasks.named("javadocJar"), tasks.named("sourcesJar"))) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we set configurations, archiveextnsion and archiveClassifier?
What does "minimize", and relocation do
"app.revanced.libs" seems arbitrary.
Why does the assemble task depend on shadowJar. I thought shadowJar depends on assemble. What about javadoc and sources? Those should not be ran when assemble is ran
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the assemble task depend on shadowJar. I thought shadowJar depends on assemble
So when you run ./gradlew build, it runs the build task. The build task depends on "assemble" and "test".
The assemble task itself doesn't do anything, it just depends on the required tasks iirc. By default with the java plugin on "jar", but I replaced that with shadowJar (although I could just added it instead of replaced, but there is no use for the unshaded patches jar/rvp )
What about javadoc and sources? Those should not be ran when assemble is ran
Its the default behaviour of gradle. The jar task depends on the javadoc and sourcesjar task (if java.withSourcesJar() and java.withJavadocJar() is called, which is the case). Because the jar task isn't run anymore (by default), we have to add the tasks manually to not lose the behaviour.
What does "minimize", and relocation do
"app.revanced.libs" seems arbitrary.
relocation is that the libraries don't conflict. The revanced-cli for example, uses apksig without obfuscation. If they use different versions if could get into a conflict. What relocation does it, it "relocates" the dependencies with a package prefix. For example "com.android.apksig" becomes "app.revanced.libs.com.android.apksig". Shadow automatically updates the imports in the shaded jar.
Minimize just means, it removes everything that isn't used. For example, I only use apksig to obtain signatures using verification. I do not sign apk files. Shadow should automatically remove any dead code.
Why do we set configurations, archiveextnsion and archiveClassifier?
By default, the output jar would have this filename "patches-5.30.0-all.jar".
I set the archiveextension to "rvp" so it is "patches-5.30.0-all.rvp".
Then I set the archiveClassifier to null, to remove the "-all", so its "patches-5.30.0.rvp".
The configurations are set, because by default the runtimeClasspath is used by shadow. In the runtimeClasspath is everything that should be present at runtime. Everything from "runtimeOnly" or "implementation" or "api" etc. is inside the runtimeClasspath. This includes things like "revanced-patcher" which is present in runtime, which shouldn't be shaded in. I explicitly add a new configuration called "shade", which is the exact same as "implementation", but with the difference that shadow shades every dependency from it into the shaded jar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assemble task itself doesn't do anything, it just depends on the required tasks iirc. By default with the java plugin on "jar", but I replaced that with shadowJar (although I could just added it instead of replaced, but there is no use for the unshaded patches jar/rvp )
I can't understand this. Please explain why the assemble task depends on shadowJar. The project should be built merely from running "gradlew build". Currently buildAndroid exists to build a .dex file but that's not ideal for the reason of it to work from running the build task.
Because the jar task isn't run anymore (by default), we have to add the tasks manually to not lose the behaviour.
shadowJar depends on the JAR task.
Shadow automatically updates the imports in the shaded jar.
This could break reflection. I don't think relocation should be enabled by default. Patches projects should individually decide on that. Without relocation and obfuscation, would it cause problems when CLI and patches have apkzlib in classpath?
Then I set the archiveClassifier to null, to remove the "-all", so its "patches-5.30.0.rvp".
You can set it to an empty string "" to avoid the cast.
If you set the extension to rvp and the classifier to "", would it override the thin JAR? I would like this behaviour. Also, does this work with the signing plugin for Gradle? WIll the shadowJar artifact be signed correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could break reflection.
No, shadow also replaces strings of classes.
Without relocation and obfuscation, would it cause problems when CLI and patches have apkzlib in classpath?
It could. Because of the patches, there is a seperate classloader. If the version does mismatch, for example:
-
revanced-patcher uses apksig 8.11.0
-
revanced-patches uses apksig 8.5.2 (via revanced-library)
-
revanced-patcher loads class ApkSigner and its dependent shared classes (because the revanced-patcher code lys in the AppClassLoader, it will try to load the 8.5.2 class of it)
-
revanced-patches loads class ApkVerifier (because revanced-patches has its own classloader, it will attempt to load the 8.11.0 version of it)
-
Now there is ApkSigner and shared classes (with version 8.5.2) in memory and ApkVerifier (with version 8.11.0) in memory. Although there are multiple versions of it, there can only be one classname loaded at a time (doesn't matter the classloader, jvm limitation).
-
If there is an internal api breakage, ApkVerifier could throw an exception, because it uses some classes also used by ApkSigner, which is an older version. For example, it could try to access a method that doesn't exist anymore.
You can set it to an empty string "" to avoid the cast.
If you set the extension to rvp and the classifier to "", would it override the thin JAR? I would like this behaviour. Also, does this work with the signing plugin for Gradle? WIll the shadowJar artifact be signed correctly?
I will look into this (although I don't like the behaviour of overwriting a jar). Like I already wrote once, it could confuse people (same filename, different files).
I can't understand this. Please explain why the assemble task depends on shadowJar. The project should be built merely from running "gradlew build". Currently buildAndroid exists to build a .dex file but that's not ideal for the reason of it to work from running the build task.
The shadowJar task isn't run by default. "./gradlew build" runs the jar task by default, not shadowJar. So I overwrote the dependency of "jar" in the assemble task with "shadowJar".
Also in my tests shadow did not depend on "jar", steps to reproduce:
build.gradle.kts
plugins {
id("java")
id("com.github.johnrengelman.shadow") version "8.1.1"
}
tasks.named("assemble") {
setDependsOn(listOf(tasks.named("shadowJar")))
}
gradle build --dry-run
or gradle assemble --dry-run
Notice: no "jar" task there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, shadow also replaces strings of classes.
That assumes the strings contain the full class. Something like "cla" + "ss" + "name" wont work.
It could. Because of the patches, there is a seperate classloader. If the version does mismatch, for example:
Different API versions can not really be solved however, wont a duplicate class be thrown at runtime?
The shadowJar task isn't run by default. "./gradlew build" runs the jar task by default, not shadowJar. So I overwrote the dependency of "jar" in the assemble task with "shadowJar".
You should make jar dependon shadowJar. This way we don't have to change anything anywhere and jar "turns" into "shadowJar". Perhaps it is possible to cancel jar task so it doesn't run unnecessarily but things like javadoc continue to work. Otherwise the shadowJar task should overwrite the jar just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That assumes the strings contain the full class. Something like "cla" + "ss" + "name" wont work.
Bad example, java is smart enough to optimize the "cla" + "ss" + "name" statement into "classname".
See
public class Test {
public static void main(String[] args) {
System.out.println("cla" + "ss" + "name");
}
}
$ javac Test.java
$ javap -c Test.class
(Notice the string "classname" being compiled in)
Same with:
public class Test {
private static final String PREFIX = "class";
public static void main(String[] args) {
System.out.println(PREFIX + "name");
}
}
But, yes, something like String.valueOf(new char[] {'c', 'l', 'a', 's', 's', 'n', 'a', 'm', 'e'})
obviously won't work, but why should any java library do something like this?
Different API versions can not really be solved however, wont a duplicate class be thrown at runtime?
Yes, the issue is though that different classes with different versions of the same library could be loaded at the same time. If you wish, I could write you a quick example in java (with a git repo).
You should make jar dependon shadowJar. This way we don't have to change anything anywhere and jar "turns" into "shadowJar". Perhaps it is possible to cancel jar task so it doesn't run unnecessarily but things like javadoc continue to work. Otherwise the shadowJar task should overwrite the jar just fine.
Yes, I can do that (with a few changes).
@@ -115,12 +125,16 @@ abstract class PatchesPlugin : Plugin<Project> { | |||
task.description = "Builds the project for Android by compiling to DEX and adding it to the patches file." | |||
task.group = "build" | |||
|
|||
task.dependsOn(tasks["jar"]) | |||
task.dependsOn(tasks["shadowJar"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about keeping everything the same around here and just making jar depend on shadow jar task? Looks simpler to me, as JAR already handles everything. Only the classifier and extension needs to be set and signing or publication configured
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thin (not shaded) jar would be additionally built then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run shadowJar in revanced-cli it already builds the thin jar. So what is different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk. Here it doesn't.
To be honest, I don't know myself why it isn't here the case here, but in revanced-cli it is.
Feel free to test it yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe because of the "application" plugin:
explorer_AIRg4slSFh.mp4
Status? |
This adds support for shading dependencies into the patches rvp file.
Needed for a fixup in ReVanced/revanced-patches#5158
I also added some entries to the .gitignore file, which IntelliJ created automatically.