KAFKA-16803: Update ShadowJavaPlugin

[Nancy-ksolves]

Upgrade to a different ShadowJavaPlugin to remove deprecated dependency 'org.gradle.util.ConfigureUtil'.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[gharris1727]

I verified that this upgrade includes a commit to remove the deprecated ConfigureUtil: Goooler/shadow@b1d87c2 and the Gradle build scan no longer shows the deprecation warning.

It also contains a fix for the blocker mentioned in the previous comment: Goooler/shadow#80 but I have not verified this fix as I am unfamiliar with the original problem.

@ijuma and @divijvaidya What do you think about changing forks for this plugin? The existing fork appears to have an issue that blocks our upgrade, isn't compatible with Gradle 9.0, and doesn't appear to have any recent releases. The new fork is from a contributor to the original fork, and has the exact same license.

[gharris1727]

I think this can be removed now.

[Nancy-ksolves]

The Comment has been removed as suggested

[divijvaidya]

I am not a big fan of taking a dependency on a project maintained by a single developer as it's a huge supply chain risk (cue: https://xkcd.com/2347/)

Our dependency on zstd-jni falls in the same category which makes me nervous. Nevertheless, coming back to the topic, I agree that releases on https://github.com/johnrengelman/shadow seems to have stalled and hence, it makes sense to use an alternative in the short term. Using https://github.com/Goooler/shadow sounds good to me.

Alternatively, can we get rid of usage of the shadow plugin? I observe that the use case for this plugin is to create a fat jar for jmh-benchmarks. I haven't explored this option, but from a quick look, it looks like we could use a few lines of vanilla gradle to achieve the same result (instead of relying on a dependency)? (see https://www.baeldung.com/gradle-fat-jar)

[Nancy-ksolves]

So maybe we can consider this as a temporary solution and then think about removing that altogether later. Thoughts?

[gharris1727]

I quickly looked into replacing the shadow jar plugin with a vanilla Jar task.
It lacks the relocate method that we use to perform real shading (not just fat-jar) for the kafka-clients jar.
I don't think that replacing this dependency will be trivial, so i'm fine moving forward with just swapping out the upstream for now.

[Nancy-ksolves]

Thanks, @gharris1727. Hope this can be merged now.

[gharris1727]

I checked the upstream diff and didn't see anything out of the ordinary: GradleUp/shadow@8.1.0...Goooler:shadow:v8.1.7

I verified locally that the kafka-clients jar has the correct filename and contains the proper shaded classes. I also verified that the JMH benchmarks run as-expected.

[gharris1727]

Test failures are unrelated.

[gharris1727]

Thank you @Nancy-ksolves for fixing this!

[omkreddy]

@Nancy-ksolves @gharris1727 It looks like io.github.goooler.shadow:8.1.7 is not honouring archiveClassifier added in https://github.com/apache/kafka/blob/trunk/build.gradle#L1569. Due to this clients jar is getting published with '-all' classifier. (kafka-clients-3.9.0-SNAPSHOT-all.jar). Its working with 8.1.3 version.

We may want to fix the issue or downgrade the shadow plugin to 8.1.3.

[gharris1727]

@omkreddy How are you seeing that classifier? This is what i'm seeing:

# ./gradlew clean clients:shadowJar
# ls clients/build/libs/
kafka-clients-3.9.0-SNAPSHOT.jar

[gharris1727]

Okay I see that the publish results are different:

ls ~/.m2/repository/org/apache/kafka/kafka-clients/3.9.0-SNAPSHOT/
kafka-clients-3.9.0-SNAPSHOT-all.jar
...

[gharris1727]

@omkreddy I confirmed that 8.1.3 prevents the -all classifier from being emitted. I did not expect that, because 8.1.3 lacks the classifier "fix" that I thought was necessary for us. After investigating further, I realized that we have an evaluation order problem.

The statement project.shadow.component(mavenJava) is executed before archiveClassifier = null, so the classifier change affects the filesystem but not the publishing build. I think this is a slight mistake in the upstream patch, as the archiveClassifier is eagerly evaluated rather than lazily evaluated.

We can immediately downgrade to 8.1.3 since that appears to work. I'll follow up with the upstream repo to see if we can change to lazy evaluation, or re-structure our build to accommodate the eager evaluation order.

Thanks again for finding this problem, it's unfortunate that I didn't find it in my pre-merge validation.

[gharris1727]

@omkreddy Here's the downgrade PR: #16489

[divijvaidya]

@gharris1727 do you have any suggestion on how could we have caught this regression?

One naive option could be to add a validation step in our build logic? In the validation we can start with a simple validation on the names of the files and add more complicated validations later on.