Shade third-party dependencies in the plugin classpath

[tadfisher]

Fixes #1091

At least one other Android-related plugin also puts Google API client libraries on the classpath with conflicting versions; see the linked issue.

• Apply the shadow plugin to android-publisher and play-publisher.
• Add a shadowImplementation configuration, for which dependency classes will be relocated to a package under com.github.triplet.gradle.shaded. Move all non-project dependencies from implementation to shadowImplementation.
• Publishing the shaded jar is configured automatically by com.gradle.plugin-publish. As android-publisher is not a plugin itself and doesn't apply that plugin in its build, apply PublishTaskShadowAction as plugin-publish does.
• testapp: Bump compileSdk and targetSdk to 34 so it builds with AGP 8.2.0.

Tested locally by publishing to mavenLocal and using it in another project, by running check tasks, and by running testapp:publishBundle (which fails due to an auth token issue, but otherwise applies the plugin fine).

[SUPERCILEX]

Can you send me a PR with all the non shadow stuff? So plugins.withType and the testapp sdk.

I don't like this because it defeats the purpose of using a dependency manager. For example, before this change people could include versions of libraries we use in their builds as a way to update us without having to get a new version of GPP. Similarly, I don't know if this is actually how the shadow plugin works, but I'm guessing it won't show any of our dependencies in the POM which means security scanning tools also won't work. We're basically opting out of the ecosystem. Maybe that's fine? I need to think about it a bit more.

Have you filed a ticket with Firebase support? They really should be the ones to upgrade.

[tadfisher]

Can you send me a PR with all the non shadow stuff? So plugins.withType and the testapp sdk.

Will do.

I don't like this because it defeats the purpose of using a dependency manager. For example, before this change people could include versions of libraries we use in their builds as a way to update us without having to get a new version of GPP.

I completely agree. The problem is on the Gradle side, though; plugins run without classpath isolation, so shading dependencies is the recommended approach by upstream (and why they built support for the Shadow plugin in plugin-publish). Otherwise you're destined to get ABI conflicts with any third-party dependency declared in the POM, because Gradle will resolve the highest version to include in the global, non-isolated plugin classpath.

There is another approach you could take, which is done by the Android Lint plugin: split the plugin into API and implementation artifacts, and have the API code construct a semi-isolated "embedded" classpath loader which loads runtime dependencies, making them available via reflection. This is less simple than shading, and arguably should be performed by Gradle itself, but at least it preserves the dependency information in the POM. You're already halfway there, having a separate android-publisher artifact with the majority of third-party dependencies.

Reading up on this: as you're using the Worker API, we could configure workers to use classLoaderIsolation and include Google API dependencies in a separate dependency configuration. This might be the cleanest way to avoid classpath conflicts, and it seems at least to be the most supported method.

Similarly, I don't know if this is actually how the shadow plugin works, but I'm guessing it won't show any of our dependencies in the POM which means security scanning tools also won't work. We're basically opting out of the ecosystem.

Yes, as-is this PR will remove the dependency information from the POM/Gradle-module metadata.

Have you filed a ticket with Firebase support? They really should be the ones to upgrade.

Ticket was filed in July; no response just yet, though it was assigned.

I think I will investigate Worker classpath isolation, unless you have objections. There is probably a bit of API leakage into the plugin currently.

[SUPERCILEX]

Will do.

See below.

so shading dependencies is the recommended approach by upstream

Alright, that works for me! I want this to be an isolated change so I can easily revert it if the pitchforks show up on the other side of this debate, but separate commits will suffice. So just pull out the unrelated changes in a separate commit and we're good.

Ticket was filed in July; no response just yet, though it was assigned.

Classic google lol.

I think I will investigate Worker classpath isolation, unless you have objections.

If you'd like to, go for it! It does sound like the right thing to do. That said, it also sounds like a lot of work 😅, so I'm ok going with the shadow approach until someone complains about it.

[tadfisher]

I investigated using classLoaderIsolation() for the worker tasks, similar to what SQLDelight is doing here: https://github.com/cashapp/sqldelight/blob/a188d6afb52afce9a736047206ed08405bd8cc3d/sqldelight-gradle-plugin/src/main/kotlin/app/cash/sqldelight/gradle/SqlDelightWorkerTask.kt#L25

It is sort of hacky, because PlayApiService is shared among all workers, but that's the thing we want to use classpath isolation; basically, the service loads a PlayPublisher implementation using the ClassLoader of the first worker thread that instantiates the lazy publisher instance, which doesn't make a whole lot of sense. I tried using a private classloader within PlayApiService, but Gradle doesn't let you do this outside of a worker thread (they detect this during configuration resolution).

So I cleaned up this change instead.

[SUPERCILEX]

because PlayApiService is shared among all workers

Yeah, trying to accumulate changes to an edit in parallel and then commit the edit at the end has been the bane of this project's existence lol.

[Goooler]

Suggested change

	tasks.withType<ShadowJar> {
	tasks.withType<ShadowJar>().configureEach {

[Goooler]

Suggested change

	tasks.withType<AbstractArchiveTask> {
	tasks.withType<AbstractArchiveTask>().configureEach {

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity.
It will be closed if no further activity occurs. Thank you for your contributions.

[SUPERCILEX]

Feel free to reopen once you're ready.