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

Configure target java version alongside gradle toolchain #5676

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

tom-seqera
Copy link
Contributor

Configure the target Java version inside the gradle java block next to the toolchain info, rather than on a per-task basis. This propogates the information to all the variants, in particular the test fixtures.

Without this, a project depending on the test fixtures (such as a nextflow plugin) can, in some situations, fail to build with the following error:

- Variant 'testFixturesApiElements' declares a library for use during compile-time, packaged as a jar, and its dependencies declared externally:
    - Incompatible because this component declares a component, compatible with Java 21 and the consumer needed a component, compatible with Java 17

Before this change

Using command gradle :nextflow:outgoingVariants to show the variants, the 'main' variants correctly declare that they require Java 17:
image

but the 'testFixtures' variants requires Java 21:
image

After this change

All the Java variants require Java 17:
image
image

All the compiled bytecode still has class file format version 61 (Java 17).

Copy link

netlify bot commented Jan 17, 2025

Deploy Preview for nextflow-docs-staging canceled.

Name Link
🔨 Latest commit f35da0c
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/6790bf9a6bb0340008343a7a

@pditommaso
Copy link
Member

All green, switch to Ready to review and merge?

@tom-seqera tom-seqera marked this pull request as ready for review January 17, 2025 13:57
@tom-seqera tom-seqera force-pushed the gradle-target-java-version branch from 467d81e to 7b9be79 Compare January 17, 2025 15:55
@pditommaso
Copy link
Member

Solved by 6dd92e3. Feel free to re-open if it still does not solve your problem.

@pditommaso pditommaso closed this Jan 19, 2025
@tom-seqera
Copy link
Contributor Author

That change doesn't seem to have solved it: the testFixtures variant jars are still marked as requireing Java 21.

I did some digging and there's a few things going on here.

The relevant javac compiler options are:

  • --source - this controls which Java language features can be used
  • --target - this determines what class file format version the compiled bytecode will have
  • --release - this is equivalent to setting --source and --target, but also controls which Java API classes/methods can be used

Source/target

in this version of Gradle, the intended way to set --source and --target is in the java block, which should be respected by all JVM-based tasks (including Groovy, Kotlin, etc):

java {
    toolchain {
        languageVersion = JavaLanguageVersion.of(21)
    }
    sourceCompatibility = 17
    targetCompatibility = 17
}

I was able to verify that this does seem to work - all the .class files (including those generated from both .groovy and .java files) have the correct version (61). Unlike setting this on the compile tasks explicitly, this also correctly sets the required Java version on the variant jars (like test fixtures).

This change would fix the issue described in this PR.

Release

While not needed for this fix, the --release compiler option is a bit more interesting. Gradle currently doesn't have a nice syntax for this. Before the changes in this PR, it is configured like so:

compileJava {
    options.release.set(17)
}

This is slightly incorrect because it doesn't apply to the tests, so an improvement would be:

tasks.withType(JavaCompile).configureEach {
    options.release = 17
}

Except of course when compiling with groovyc in 'joint' mode, neither approach has any effect (ie the existing config on master isn't doing anything either). We'd need to specify it on the Groovy compile tasks for it to be applied. Using groovyc directly, it would look like this:

groovyc -j -J-release=17 ...

Unfortunately, as far as I can tell, there doesn't seem to be a way to persuade Gradle to correctly pass this through in joint compilation mode - options.forkOptions.jvmArgs would seem logical, but I wasn't able to find a way to make it work.

In any case, since this already seems not to be working on master, it's probably a separate issue to the change needed for test fixtures.

@tom-seqera tom-seqera reopened this Jan 20, 2025
@pditommaso pditommaso force-pushed the master branch 2 times, most recently from 1f834a0 to 6454605 Compare January 20, 2025 15:13
@pditommaso
Copy link
Member

That change doesn't seem to have solved it: the testFixtures variant jars are still marked as requireing Java 21.

Have you tried using testCompileJava similarly to compileJava

  testCompileJava {
        options.release.set(17)
    }

@tom-seqera tom-seqera force-pushed the gradle-target-java-version branch from 7b9be79 to 48a5b12 Compare January 21, 2025 15:04
@tom-seqera
Copy link
Contributor Author

Have you tried using testCompileJava similarly to compileJava

I think the release option has become a distraction and it's not related to the test fixtures problem, so I've updated the PR to contain only the change needed to fix the test fixtures.

@pditommaso
Copy link
Member

pditommaso commented Jan 21, 2025

Even better then! please add a comment to highlight sourceCompatibility and targetCompatibility apply also to the groovy compiler and there's no need for groovy specific setting

This applies it to all gradle variants (including test fixtures)

Signed-off-by: Tom Sellman <tom.sellman@seqera.io>
@tom-seqera tom-seqera force-pushed the gradle-target-java-version branch from 48a5b12 to f35da0c Compare January 22, 2025 09:51
@pditommaso pditommaso merged commit 2153abb into master Jan 22, 2025
20 checks passed
@pditommaso pditommaso deleted the gradle-target-java-version branch January 22, 2025 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants