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

chore: Update projects that use Arrow 18 to require Java 11 #6417

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

niloc132
Copy link
Member

These projects depend directly or indirectly on Arrow v18 jars, which are compiled to require Java 11. Gradle correctly fails to build if a project depending on one of these still tries to use Java 8.

Follow-up #6347

@niloc132 niloc132 added build NoDocumentationNeeded dependencies Pull requests that update a dependency file ReleaseNotesNeeded Release notes are needed labels Nov 22, 2024
@niloc132 niloc132 added this to the 0.37.0 milestone Nov 22, 2024
Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

I'm surprised that gradle doesn't fail to build these JARS that target --release 8; I would have thought / hoped that was the case? Does it implicitly upgrade them to --release 11 based on their dependencies?

I don't think we need / want to bump java-client-session / java-client-session-dagger to 11 since it should not depend on Arrow.

@niloc132
Copy link
Member Author

I think Gradle can only fail in that way if we could have gotten the org.gradle.jvm.version=11 attribute on the arrow jars - but as they are published by maven, they don't include gradle-specific metadata. I did spend a little time to see if we could annotate our libs.versions.toml or maybe reintroduce a Classpath.groovy or bom project that would add annotations to our dependencies, but didn't find an obvious way. Likewise, javac doesn't seem to care that --release=8 was passed with a classpath that includes Java 11 bytecode - which makes some sense, since that would actually require each JDK is shipped with the full bytecode of all APIs before it, rather than some metadata of what changed.

java-client-session explicitly depends on proto-backplane-grpc, which as of this patch is now Java 11, because in turn it depends on flight. Granted, that is a compileOnly dependency, but we do have classes that use it. If you restore languageLevel=8 to java-client-session, the build will begin to fail since proto-backplane-grpc still is 11 - and in turn if we restore languageLevel=8 to proto-backplane-grpc then while it won't fail, that's only because your above "why doesnt gradle fail with --release 8" doesn't work as we expected - fixing that in our build may yet be possible, and I don't think we should try to make it easier to run Java 8 when we're actively trying to reduce support there.

@devinrsmith
Copy link
Member

I would have hoped that the bytecode level of the JAR would be enough information to suss this out; if not via javac, then through gradle :/

Although, I guess technically, I can strongarm an argument for why it is the way it is: "Yes, I'm creating a Jar that only uses Java 8 features, and builds a Java 8 compatible Jar; and even though I depend directly/transitively on Java 11+ Jar, that's a concern for runtime, and shouldn't effect my Jar in isolation".

Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

I think we should file an issue that languageLevel doesn't provide as strong as guarantees as I would have thought / hoped.

@niloc132 niloc132 merged commit baa9870 into deephaven:main Nov 22, 2024
26 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build dependencies Pull requests that update a dependency file NoDocumentationNeeded ReleaseNotesNeeded Release notes are needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants