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

add support for Java toolchains #16

Merged
merged 4 commits into from
Apr 3, 2023
Merged

Conversation

madisp
Copy link
Contributor

@madisp madisp commented Mar 28, 2023

Add support for specifying the Java toolchain to use for the R8 invoke.
The JAVA_HOME of the specified toolchain will be passed in to the R8 invoke as library jars. If no toolchain is specified then the default one from Java plugin extension is used.

NOTE: requires bumping Gradle api to 6.7, potentially breaking change for downstream consumers?

@martinbonnin
Copy link
Member

Very cool, thanks 👍 !

In the current state, usage should look like this, right?

gr8 {
  create("relocated") {
    toolchain(java.toolchain {
      languageVersion.set(JavaLanguageVersion.of(11))
    })
  }
}

Would it be possible to use a more java-like way to set it like so?

gr8 {
  create("relocated") {
    toolchain {
      languageVersion.set(JavaLanguageVersion.of(11))
    }
  }
}

requires bumping Gradle api to 6.7, potentially breaking change for downstream consumers?

I think this is fine. Users can stay on older version if really needed but I would expect most Gradle plugin developers to use more recent versions of Gradle.

@madisp
Copy link
Contributor Author

madisp commented Mar 29, 2023

Very cool, thanks 👍 !

In the current state, usage should look like this, right?

gr8 {
  create("relocated") {
    toolchain(java.toolchain {
      languageVersion.set(JavaLanguageVersion.of(11))
    })
  }
}

Would it be possible to use a more java-like way to set it like so?

gr8 {
  create("relocated") {
    toolchain {
      languageVersion.set(JavaLanguageVersion.of(11))
    }
  }
}

hm, I checked the docs and a better way is actually to ask for Launcher (or in this case Compiler) specifically:

https://docs.gradle.org/current/userguide/toolchains.html#toolchains_for_tasks

invoking java.toolchain {} would also modify your build toolchain. So perhaps something like this:

gr8 {
  create("relocated") {
    toolchain(javaToolchains.compilerFor {
        languageVersion.set(JavaLanguageVersion.of(11))
    })
  }
}

and a convenience method that takes the Action<JavaToolchainSpec> like so:

gr8 {
  create("relocated") {
    toolchain {
      languageVersion.set(JavaLanguageVersion.of(11))
    }
  }
}

Looking at your comment we could easily hide the former and expose the latter.

one thing that slightly bugs me here, the name toolchain in that case isn't very great and compiler wouldn't be straightforward as well. Would be ok to name it something like systemClassesToolchain instead? A bit longer but much more descriptive.

@martinbonnin
Copy link
Member

invoking java.toolchain {} would also modify your build toolchain

Right, makes sense 👍

Would be ok to name it something like systemClassesToolchain instead?

Agreed, this is better 👍

@martinbonnin
Copy link
Member

@madisp any objection to merging this as is? The plugin is still pre-1.0 so we can always change later and this is a nice change to ship

@madisp
Copy link
Contributor Author

madisp commented Apr 3, 2023

@martinbonnin no objections now :D

I updated to use the JavaCompiler instead of JavaLauncher, changed the configuration block to accept compiler directly and added a convenience method taking an Action<JavaToolchainSpec>.

Added a note at the end of the README.md as well.

@madisp madisp marked this pull request as ready for review April 3, 2023 18:59
@madisp madisp requested a review from martinbonnin April 3, 2023 18:59
@martinbonnin
Copy link
Member

Thanks for the quick turnaround !

@martinbonnin martinbonnin merged commit cb007cd into GradleUp:main Apr 3, 2023
@madisp madisp deleted the java-toolchain branch April 3, 2023 19:15
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.

None yet

2 participants