search

apache / kyuubi

Apache Kyuubi is a distributed and multi-tenant gateway to provide serverless SQL on data warehouses and lakehouses.
https://kyuubi.apache.org/
Apache License 2.0
2.11k stars 914 forks source link

[BUILD] pin Maven compiler target version to Java 8 and remove explict compiler release version #6766

Open bowenliang123 opened 1 month ago

bowenliang123 commented 1 month ago

:mag: Description

Issue References 🔗

This pull request fixes #

Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Types of changes :bookmark:

Test Plan 🧪

Behavior Without This Pull Request :coffin:

Behavior With This Pull Request :tada:

Related Unit Tests

Checklist 📝

Be nice. Be informative.

codecov-commenter commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 0.00%. Comparing base (c391d16) to head (c374494).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #6766 +/- ## ====================================== Coverage 0.00% 0.00% ====================================== Files 687 687 Lines 42442 42442 Branches 5793 5793 ====================================== Misses 42442 42442 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚨 Try these New Features:

pan3793 commented 1 month ago

since we set parent pom to org.apache:apache:33, we don't need to define those properties again, just follow https://github.com/apache/maven-apache-parent/commit/ded34a863110021191af2db5b772b665a2789394 to override maven.compiler.target then everything is fine.

bowenliang123 commented 1 month ago

since we set parent pom to org.apache:apache:33, we don't need to define those properties again, just follow apache/maven-apache-parent@ded34a8 to override maven.compiler.target then everything is fine.

SGTM. Applied a similar patch to our pom. And it could be removed when apache parent pom 34 released in the future.

bowenliang123 commented 1 month ago

Builds failed for missing class/package sun.misc.Signal with error logs: https://github.com/apache/kyuubi/actions/runs/11452487137/job/31863464278?pr=6766#step:9:197

Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:26: not found: object sun
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:36: not found: type Signal
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:54: not found: type Signal
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:54: not found: type SignalHandler
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:56: not found: type SignalHandler
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:56: not found: value Signal
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:58: not found: type Signal
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:59: not found: value Signal
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:65: not found: value Signal
Error: [ERROR] 9 errors found

cc @pan3793 @wForget

pan3793 commented 1 month ago

cc @LuciferYang, could you please give some advice if you have time? thanks in advance.

LuciferYang commented 1 month ago

Builds failed for missing class/package sun.misc.Signal with error logs: https://github.com/apache/kyuubi/actions/runs/11452487137/job/31863464278?pr=6766#step:9:197

Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:26: not found: object sun
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:36: not found: type Signal
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:54: not found: type Signal
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:54: not found: type SignalHandler
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:56: not found: type SignalHandler
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:56: not found: value Signal
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:58: not found: type Signal
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:59: not found: value Signal
Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignalRegister.scala:65: not found: value Signal
Error: [ERROR] 9 errors found

cc @pan3793 @wForget

~After version 4.7.1, the scala-maven-plugin will, by default, add the -release compilation option for newer~ ~Scala versions, such as 2.13.9+ and possibly some newer versions of 2.12.x (I can't recall the specifics).~

-release can lead to compilation errors when crossing Java versions, such as using Java APIs that are not opened(SignalHandler in Java 8 belongs to the internal API, If the target version is 9+ using SignalHandler is also visible during cross compilation). ~If it's not possible to avoid using them,we can try downgrading the~ ~scala-maven-plugin to version 4.7.1 to avoid the forced addition of the -release compilation option~

Ignoring the above comments, it seems that it's not the plugin that adds -release, but rather the apache-33.pom that activates the jdk9+ profile, which then uses maven.compiler.release.

 <profile>
      <id>jdk9+</id>
      <activation>
        <!-- this does not support toolchains, https://issues.apache.org/jira/browse/MNG-6943 -->
        <jdk>[9,)</jdk>
      </activation>
      <properties>
        <!-- https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-release.html, affects m-compiler-p and m-javadoc-p -->
        <maven.compiler.release>${maven.compiler.target}</maven.compiler.release>
      </properties>
    </profile>

So, without modifying the code, when building with Java 17/21, the -release option might only be able to use a higher version, such as 11. I haven't thought of any better solutions at the moment. @pan3793