Skip to content
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

KAFKA-14680: Upgrade gradle version from 7.6 to 8.0.1 #13205

Merged
merged 3 commits into from Feb 24, 2023

Conversation

dejan2609
Copy link
Contributor

@dejan2609 dejan2609 commented Feb 6, 2023

Details:

  • gradle upgrade: 7.6 -> 8.0.1
  • spotbugs plugin upgrade: 5.0.9 -> 5.0.13
  • tweaked the mechanics for -release/-source/-target to workaround idiosyncrasies in Gradle 8.0.1 and newer Scala 2.13 versions.
  • streams-scala test task no longer triggers the spotless task since a newer version is required for Gradle 8 support, but the newer version requires Java 11.
    Note: relates to MINOR: Add spotlessScalaCheck dependency to streams-scala test task #5479

Gradle upgrade highlights:

Full release notes: https://docs.gradle.org/8.0/release-notes.html

@dejan2609
Copy link
Contributor Author

dejan2609 commented Feb 6, 2023

Related to #13199

Thanx for heads up @ijuma

On my local box build ends up in green, not sure why it fails on Jenkins; will investigate (and wait for plugin changes to be merged).

18:46:36  + ./retry_zinc ./gradlew -PscalaVersion=2.13 clean compileJava compileScala compileTestJava compileTestScala spotlessScalaCheck checkstyleMain checkstyleTest spotbugsMain rat --profile --continue -PxmlSpotBugsReport=true -PkeepAliveMode=session
18:46:36  Error: Could not find or load main class org.gradle.wrapper.GradleWrapperMain
18:46:36  Caused by: java.lang.ClassNotFoundException: org.gradle.wrapper.GradleWrapperMain

@ijuma
Copy link
Contributor

ijuma commented Feb 6, 2023

@dejan2609 We'll wait for the final release, but it's fine to start working through the issues. Looks like the build failed.

@dejan2609
Copy link
Contributor Author

@ijuma yes, somehow build on Jenkins fails to start, I will rebase and then test on my local box (with Windows/Linux).

@ijuma
Copy link
Contributor

ijuma commented Feb 15, 2023

Gradle 8 has been released. Have you been able to figure out why the build is failing?

@dejan2609
Copy link
Contributor Author

@ijuma some findigs: build fails due to issues with gradle wrapper bootstrapping.

I will post more details today.

@dejan2609
Copy link
Contributor Author

Update: gradle wrapper bootstrapping is ok now, but spotless Scala checks are failing...
Searching for a solution.

@dejan2609
Copy link
Contributor Author

It seems that Spotless Gradle plugin needs to be alligned with Gradle 8.0 (I filed a ticket here: diffplug/spotless#1572)

Thing is that they dropped support for direct Java 8 builds: https://github.com/diffplug/spotless/blob/main/plugin-gradle/CHANGES.md#6140---2023-01-26 (their suggestion for other teams is to use Java cross compilation: https://docs.gradle.org/8.0/userguide/building_java_projects.html#sec:java_cross_compilation).

Kafka obviously still needs to build artifacts against Java 8, so maybe it would be a good idea to use Spotless team suggestion.

All-in-all, herewith a plan for a Gradle 7 -->> 8 upgrade:

  • spotless team will release Gradle 8.0 compatible version (most probably they will not backport solution into spotless gradle 6.13.x line)
  • in parallel I can try to drop JDK 8 usage (note: Java 8 compatible artifacts will still be generated)
  • when we make sure that Java cross compilation works we can come back to this PR and bump Gradle (and Spotless plugin version to 6.15+).

@ijuma If it is ok with you I can start working towards this solution.

@ijuma
Copy link
Contributor

ijuma commented Feb 15, 2023

An alternative would be to drop spotless until we drop support for Java 8 (Apache Kafka 4.0). What actually uses spotless today?

@dejan2609
Copy link
Contributor Author

At the moment spotless is used via Jenkins CI server ➡️ Jenkinsfile 'spotlessScalaCheck' task execution: https://github.com/apache/kafka/blob/3.4.0/Jenkinsfile#L23

My suggestion is to:

Note: this PR already removes task dependency.

@dejan2609 dejan2609 marked this pull request as ready for review February 16, 2023 11:13
@dejan2609
Copy link
Contributor Author

🤔 Interesting, altough 'spotlessScalaCheck' is removed out of Jenkinsfile that task is still executed on Jenkins:

image

@ijuma
Copy link
Contributor

ijuma commented Feb 16, 2023

Yeah, that's a security feature. I suggest filing a JIRA about disabling spotlessScalaCheck with the rationale and have a separate PR for that change.

@dejan2609
Copy link
Contributor Author

Update: there are some new issues with Gradle 8.0.1; I will try to investigate that also.

@dejan2609
Copy link
Contributor Author

dejan2609 commented Feb 21, 2023

@ijuma Good news, it seems that I managed to provide a workaround for Gradle 8.0.1 (and across all JDK/Scala versions that Apache Kafka uses and the moment).
I still need to test few more time and after that will move this PR from draft to ready for review.

Big shoutout for @ljacomet from Gradle team, he went over and above during investigation 🎉

@ijuma
Copy link
Contributor

ijuma commented Feb 21, 2023

Thanks @dejan2609. I still prefer the path I described here:

gradle/gradle#23962 (comment)

Do you know if that works?

@inglor
Copy link

inglor commented Feb 22, 2023

Thanks @dejan2609. I still prefer the path I described here:

gradle/gradle#23962 (comment)

Do you know if that works?

Hi @ijuma ,

just to pitch in that setting -release on scala 2.13 is producing same error
[Error] : -target is deprecated: Use -release instead to compile against the correct platform API.

diff --git a/build.gradle b/build.gradle
--- a/build.gradle	(revision 2e1947d240607d53f071f61c875cfffc3fec47fe)
+++ b/build.gradle	(date 1677025069098)
@@ -684,7 +684,7 @@
     }
 
     // Scalac's `-release` requires Java 9 or higher
-    if (JavaVersion.current().isJava9Compatible())
+    if (versions.baseScala == '2.13')
       scalaCompileOptions.additionalParameters += ["-release", minJavaVersion]
 
     configure(scalaCompileOptions.forkOptions) {

PS: Encountered this error while trying to package kafka for Arch Linux which has latest gradle version 8.0.1

@ijuma
Copy link
Contributor

ijuma commented Feb 22, 2023

@inglor Thanks. The following comment explains that the way -release must be set is a bit specific:

gradle/gradle#23962 (comment)

@dejan2609
Copy link
Contributor Author

dejan2609 commented Feb 22, 2023

@inglor thanx for stopping by, any insight or help is highly appreciated !

@ijuma I scraped fast solution that, suprisingly enough, works (at least compilation is succesful, that is).

I tested locally across entire matrix: JDK 8/11/17 x Scala 2.12/13 (with ./gradlew -PscalaVersion=2.1X clean jar) and all builds are fine.

Still need to spend some time with this to doublecheck and maybe even to compare compilation console output (trunk vs. this PR).

@inglor
Copy link

inglor commented Feb 22, 2023

@ijuma Thanks for the pointer - I have missed that.
@dejan2609 I can confirm your cdbb59f works fine (as in it compiles the scala code) for

Gradle version 8.0.1:

Scala 2.12 Scala 2.13
JDK 8
JDK 11
JDK 17

@dejan2609
Copy link
Contributor Author

@dejan2609 I added a few cleanups here: ijuma@9c6ae57

If you agree with them, please integrate into your PR.

@ijuma Scala 2.13 builds are fine, but something is still missing for Scala 2.12 (all ./gradlew -PscalaVersion=2.12 clean jar builds are failling):

  • JDK 17
> Task :core:compileScala
'-release' does not accept multiple arguments
bad option: '-release:8'

> Task :core:compileScala FAILED

> Task :streams:streams-scala:compileScala FAILED
'strict-unsealed-patmat' is not a valid choice for '-Xlint'
'-release' does not accept multiple arguments
bad option: '-release:8'

FAILURE: Build completed with 2 failures.
  • JDK 11
> Task :core:compileScala FAILED

> Task :streams:streams-scala:compileScala
'strict-unsealed-patmat' is not a valid choice for '-Xlint'
'-release' does not accept multiple arguments
bad option: '-release:8'

> Task :streams:streams-scala:compileScala FAILED

FAILURE: Build completed with 2 failures.
  • JDK 8
> Task :core:compileScala FAILED
'-release' does not accept multiple arguments
bad option: '-release:8'

FAILURE: Build failed with an exception.

@ijuma
Copy link
Contributor

ijuma commented Feb 23, 2023

@dejan2609 Good catch. I fixed it in the same branch. And tested with Scala 2.12 and 2.13. See ijuma@31aec82

Gradle 8 related links:
* https://github.com/gradle/gradle/releases/tag/v8.0.0
* https://github.com/gradle/gradle/releases/tag/v8.0.1
* https://docs.gradle.org/8.0.1/userguide/upgrading_version_7.html#changes_8.0

notes:
* Javac and Scalac options are reshuffled (workaround for Gradle 8.0.1 bug: gradle/gradle#23962 (comment))
* spotless plugin task reference is removed (newer plugin versions require Java 11, so we can't use them until Kafka 4.0); plugin configuration is kept
* jacoco version is bumped: 0.8.7 -->> 0.8.8 https://docs.gradle.org/8.0.1/userguide/jacoco_plugin.html#sec:configuring_the_jacoco_plugin
@dejan2609 dejan2609 marked this pull request as ready for review February 23, 2023 06:25
@dejan2609
Copy link
Contributor Author

@ijuma changes are integrated (please remember that #13263 needs to be merged prior to this one).

ijuma pushed a commit that referenced this pull request Feb 23, 2023
Blocks #13205.

Rationale:
- build works fine in trunk with Gradle 7.6 and spotless gradle plugin 6.13.0 for all currently used JDK versions (that is: JDK 8 / JDK 11 / JDK 17)
- however, recent spotless gradle plugin versions (6.14.+) support only JDK 11+ versions: https://github.com/diffplug/spotless/blob/main/plugin-gradle/CHANGES.md#6140---2023-01-26
- given a fact that Kafka still needs to support JDK 8 builds (until Kafka version 4.0) it is reasonable to simply remove spotless checks out of Jenkinsfile (and re-introduce them when the time comes).

For even more details see GitHub discussion here: #13205 (comment)

Reviewers: Ismael Juma <ismael@juma.me.uk>
@ijuma ijuma changed the title KAFKA-14680: gradle version upgrade 7 -->> 8 KAFKA-14680: Upgrade gradle version from 7.6 to 8.0.1 Feb 23, 2023
@ijuma
Copy link
Contributor

ijuma commented Feb 23, 2023

@dejan2609 I merged #13263 and updated the PR description. Please take a look.

@ijuma
Copy link
Contributor

ijuma commented Feb 23, 2023

@dejan2609 One issue is that IntelliJ still doesn't handle this correctly. Before this PR, it would add the following to .idea/scala_compiler.xml:

<parameter value="-target:jvm-1.8" />

Now it adds:

<parameter value="-target:jvm-17" />

Both cause the build to break with the latest Scala 2.13 version since that deprecated -target and we compile with fatal warnings.

gradlew Outdated Show resolved Hide resolved
@ijuma
Copy link
Contributor

ijuma commented Feb 23, 2023

I pushed a commit that reverts part of my previous commit. We need to set sourceCompatibility and targetCompatibility to workaround an issue affecting the Scala plugin for IntelliJ. We're back to with this change:

<parameter value="-target:jvm-1.8" />

This is still a problem since this is deprecated in newer Scala versions, but this PR doesn't make it worse.

@dejan2609 would you be willing to submit a ticket for the Scala plugin for IntelliJ so it doesn't set -target if -release is set?

Copy link
Contributor

@ijuma ijuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks. I'll wait for the CI to complete before merging. The last CI build looked good, so it should be fine.

@dejan2609
Copy link
Contributor Author

@dejan2609 would you be willing to submit a ticket for the Scala plugin for IntelliJ so it doesn't set -target if -release is set?
@ijuma Sure, will do that.

@ijuma
Copy link
Contributor

ijuma commented Feb 24, 2023

JDK 8 build passed, the other two had unrelated failures:

Build / JDK 17 and Scala 2.13 / org.apache.kafka.streams.processor.internals.DefaultStateUpdaterTest.shouldPauseActiveTaskAndTransitToUpdateStandby()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.integration.ConnectorRestartApiIntegrationTest.testMultiWorkerRestartOnlyConnector
Build / JDK 11 and Scala 2.13 / org.apache.kafka.tools.MetadataQuorumCommandTest.[5] Type=Raft-CoReside, Name=testDescribeQuorumStatusSuccessful, MetadataVersion=3.5-IV0, Security=PLAINTEXT

@ijuma ijuma merged commit 72dd401 into apache:trunk Feb 24, 2023
@chia7712
Copy link
Contributor

Should we disable spotlessScala temporarily? otherwise, the Gradle task related to spotlessScala can't work. For example, the common command ./gradlew clean build -x test.

@dejan2609
Copy link
Contributor Author

@chia7712 Correct, ./gradlew build executed against trunk does fail.

For the last few days we (@ijuma and me):

  • merged into a trunk Jenkinsfile change that removes 'spotlessScalaCheck': 97efdc6
  • merged this PR also (that was tested with ./gradlew jar on our local machines and then on Jenkins)

However, ./gradlew build depends on spotless tasks (tree is generated using this Gradle plugin: https://plugins.gradle.org/plugin/org.barfuin.gradle.taskinfo)

./gradlew build tiTree

> Task :tiTree
:build                                                         (org.gradle.api.DefaultTask)
+--- :assemble                                                 (org.gradle.api.DefaultTask)
|    `--- :jar                                                 (org.gradle.api.tasks.bundling.Jar)
|         +--- :classes                                        (org.gradle.api.DefaultTask)
|         |    +--- :compileJava                               (org.gradle.api.tasks.compile.JavaCompile)
|         |    `--- :processResources                          (org.gradle.language.jvm.tasks.ProcessResources)
|         `--- :compileJava                                    (org.gradle.api.tasks.compile.JavaCompile)
`--- :check                                                    (org.gradle.api.DefaultTask)
     +--- :rat                                                 (org.nosphere.apache.rat.RatTask)
     +--- :spotlessCheck                                       (org.gradle.api.DefaultTask)
     |    `--- :spotlessScalaCheck                             (com.diffplug.gradle.spotless.SpotlessCheck)
     |         `--- :spotlessScala                             (com.diffplug.gradle.spotless.SpotlessTaskImpl)
     |              `--- :spotlessInternalRegisterDependencies (com.diffplug.gradle.spotless.RegisterDependenciesTask)
     `--- :test                                                (org.gradle.api.tasks.testing.Test)
          +--- :classes                                        (org.gradle.api.DefaultTask)
          |    +--- :compileJava                               (org.gradle.api.tasks.compile.JavaCompile)
          |    `--- :processResources                          (org.gradle.language.jvm.tasks.ProcessResources)
          +--- :compileJava                                    (org.gradle.api.tasks.compile.JavaCompile)
          +--- :compileTestJava                                (org.gradle.api.tasks.compile.JavaCompile)
          |    +--- :classes                                   (org.gradle.api.DefaultTask)
          |    |    +--- :compileJava                          (org.gradle.api.tasks.compile.JavaCompile)
          |    |    `--- :processResources                     (org.gradle.language.jvm.tasks.ProcessResources)
          |    `--- :compileJava                               (org.gradle.api.tasks.compile.JavaCompile)
          `--- :testClasses                                    (org.gradle.api.DefaultTask)
               +--- :compileTestJava                           (org.gradle.api.tasks.compile.JavaCompile)
               |    +--- :classes                              (org.gradle.api.DefaultTask)
               |    |    +--- :compileJava                     (org.gradle.api.tasks.compile.JavaCompile)
               |    |    `--- :processResources                (org.gradle.language.jvm.tasks.ProcessResources)
               |    `--- :compileJava                          (org.gradle.api.tasks.compile.JavaCompile)
               `--- :processTestResources                      (org.gradle.language.jvm.tasks.ProcessResources)

@chia7712
Copy link
Contributor

@dejan2609 thanks for nice explanation!

That is why I suggested to disable spotlessScala from build.gradle :)

@dejan2609
Copy link
Contributor Author

@chia7712 I just pushed PR that (hopefully) solves this regression, feel free to check it.

@dejan2609
Copy link
Contributor Author

@dejan2609 would you be willing to submit a ticket for the Scala plugin for IntelliJ so it doesn't set -target if -release is set?

@ijuma ticket is created here: https://youtrack.jetbrains.com/issue/SCL-21039

@dejan2609
Copy link
Contributor Author

@ijuma please review Scala plugin for Intellij ticket (link above); I need some help to provide reproducer.

@ijuma
Copy link
Contributor

ijuma commented Feb 27, 2023

The spotlessScala issue was fixed via #13311.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants