Differing classloaders can cause distribution plugins to miss dependency recommendations from within the repository

[AlexLandau]

What happened?

We have seen some cases where a distribution plugin was not discovering recommended dependencies from elsewhere in the same repo. In particular, someone was writing a plugin wrapping the asset distribution plugin and this behavior was not working in their Gradle TestKit test, so they added a task dependency on the upstream jar task, which was causing everything to compile when running ./gradlew --write-locks. I thought that was terrible and decided to investigate, and here we are.

It turns out that this failure happens when the classloaders for the respective plugins in the two projects are different. Here's where we currently set up the necessary task dependencies to make this discovery work correctly:

sls-packaging/gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/tasks/CreateManifestTask.java

Lines 135 to 136 in c96d7ef

	return Stream.of(
	dependencyProject.getTasks().withType(ConfigureProductDependenciesTask.class));

When the ConfigureProductDependenciesTask.class in that code comes from a different classloader than the ConfigureProductDependenciesTask that is the class for the Task we want, the task will be filtered out and not used as a dependency.

I first saw this in a case involving Gradle TestKit. There's an internal repository named sls-packaging-multiproject-test with a simpler repro of this, and a link to a related ticket.

What did you want to happen?

Instead of identifying the appropriate tasks by their class, we could use project.getPluginManager().hasPlugin("com.palantir.sls-recommended-dependencies") to check that that plugin is in use, and if so, depend on the configureProductDependencies task by name. That would avoid any classloader issues.

[iamdanfox]

@AlexLandau on foundry infra we've written a lot of gradle plugins, and we do have a few cases where a plugin depends on another plugin, but it has definitely multiplied the complexity. I think we've avoided hitting the problem you describe by always using the old-style gradle syntax (buildscript+classpath+apply plugin) rather than the new style (plugins { id 'whatever' }) as the old one jams everything onto one classloader, and the new one has isolated classloaders.

I think if we were diving into this chunk of the codebase, I'd want to rejig this so that it uses my new favourite hammer (gradle attributes and artifact transformers, a la this thing).

Lastly, I don't know what the context was for making a plugin on top of the asset plugin BUT would you consider just upstreaming the functionality into this repo??

[iamdanfox]

But I'd also happily accept a PR to do the stringy thing if you want a short-term fix 🤷