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

Make project compilable against jdk8 #245

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ksumit
Copy link

@ksumit ksumit commented Nov 17, 2023

What is the purpose of the pull request

Fixes #244

Brief change log

  • Uses JDK8 compatible source and target configurations for Maven
  • Uses syntax that is compatible to compile on JDK8 and beyond.

Verify this pull request

This pull request is a trivial rework / code cleanup without any test coverage.

  • Verified by running $ mvn clean package -DskipTests locally.

Copy link
Contributor

@the-other-tim-brown the-other-tim-brown left a comment

Choose a reason for hiding this comment

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

run mvn spotless:apply to pass the style checker

@ksumit
Copy link
Author

ksumit commented Nov 17, 2023

Looks like i've opened a can full of worms :-)

$ mvn spotless:apply
....
...
[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.27.2:apply (default-cli) on project onetable: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:2.27.2:apply failed: You are running Spotless on JVM 8, which limits you to google-java-format 1.7.
[ERROR] Upgrade your JVM or try google-java-format alternatives:
[ERROR] - Version 1.7 requires JVM 8+
[ERROR] - Version 1.15.0 requires JVM 11+

@the-other-tim-brown
Copy link
Contributor

Looks like i've opened a can full of worms :-)

$ mvn spotless:apply
....
...
[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.27.2:apply (default-cli) on project onetable: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:2.27.2:apply failed: You are running Spotless on JVM 8, which limits you to google-java-format 1.7.
[ERROR] Upgrade your JVM or try google-java-format alternatives:
[ERROR] - Version 1.7 requires JVM 8+
[ERROR] - Version 1.15.0 requires JVM 11+

you can downgrade the plugin version locally to get it to run

@charlesy6
Copy link

Agree, we known that hudi, iceberg, paimon still support JDK8.

          I chatted about this with @ashvina this morning. The production reality is that JDK8 is still the most in use (at least within bigdata ecosystem). We should compile the project against JDK8 to ensure we don't end up taking a dependency on a library that is only compatible with higher versions of JDK. Let's discuss this offline if we can find a middle ground where developers feel empowered to use a higher version of JDK for development but we are still guaranteeing a JDK8 compatible build output along with runtime dependencies being compatible with JDK8.

Originally posted by @ksumit in #244 (comment)

@the-other-tim-brown
Copy link
Contributor

@charlesy6 the jar produced currently is java8 compatible. We use the build target for that.

@charlesy6
Copy link

@the-other-tim-brown, but this is very unfriendly to developers.

@the-other-tim-brown
Copy link
Contributor

@charlesy6 forcing people to downgrade would be just as unfriendly so if there is a way to make the build tooling work with both versions then we should pursue that. It looks like Iceberg and Paimon support multiple versions in their setup so let's take some inspiration from them.

@charlesy6
Copy link

@charlesy6 forcing people to downgrade would be just as unfriendly so if there is a way to make the build tooling work with both versions then we should pursue that. It looks like Iceberg and Paimon support multiple versions in their setup so let's take some inspiration from them.

emm, I think ksumit's reasons in the issues are already very clear.

@the-other-tim-brown
Copy link
Contributor

the-other-tim-brown commented Jun 6, 2024

@charlesy6 forcing people to downgrade would be just as unfriendly so if there is a way to make the build tooling work with both versions then we should pursue that. It looks like Iceberg and Paimon support multiple versions in their setup so let's take some inspiration from them.

emm, I think ksumit's reasons in the issues are already very clear.

@charlesy6 care to elaborate? There is discussion in the issue about allowing users to develop with a more modern java version while producing java packages that are capable of running in a java 8 environment. That is what we are doing today. I am suggesting that we allow java 11 and java 8 for developers to build locally instead of just java 11 by using similar setup.

@ghost
Copy link

ghost commented Jun 6, 2024

CI report:

Bot commands @xtable-bot supports the following commands:
  • @xtable-bot run azure re-run the last Azure build

@charlesy6
Copy link

@ksumit can you rebase the main branch? This branch has conflicts that must be resolved.

@ksumit
Copy link
Author

ksumit commented Jul 25, 2024

I can rebase it but we don't have agreement on the next steps.

Argument 1: Developers want to use new jdk features and new syntaxes and we are building "jdk8 compatible jars" anyways.
Argument 2: We are running under risk that "jdk8 compatible jars" don't actually work on jre8 for some reason (a bug in jdk's compatibility mode compilation)

After talking to @ashvina and @the-other-tim-brown offline, they felt strongly about not taking away developer's ability to develop against latest JDK features and I think that's needed to attract more contributors. On the other hand, I don't have data to support Argument 2 as of now and so backed off.

@ksumit
Copy link
Author

ksumit commented Jul 25, 2024

I missed to highlight that it's never going to be possible to build java 11 or higher syntax against jdk8. I'm inclined to close this PR without any change.

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

Successfully merging this pull request may close these issues.

Make project compile against jdk8
3 participants