Implement possibility to set custom java home for Gradle tasks

[dreis2211]

Hi,

as discussed in #20147 this PR should give us a first insight into what might be broken with JDK 14.

The new functionality can be used like this:

./gradlew -PcustomJavaHome=/Users/christoph.dreis/.sdkman/candidates/java/14-ea build

I specifically implemented a check for emptiness of the new property in preparation for an eventual CI. By that we could simply set it for JDK 14 builds and leave it empty for the others. That should keep the build-project.sh relatively simple.

Let me know what you think.
Cheers,
Christoph

[wilkinsona]

Thanks very much, @dreis2211.

I don't think we need to generate the javadoc with JDK 14 (we don't really need to generate it with anything other than JDK 8). If we ignore Javadoc tasks here, the changes would then be focused on Test tasks alone. That would mean we could give the property a more descriptive name. Something like testTaskJavaHome, perhaps.

[snicoll]

the changes would then be focused on Test tasks alone

Don't we want to compile with JDK14 as well?

[wilkinsona]

Don't we want to compile with JDK14 as well?

Oops. I'd completely overlooked that part of the PR.

I'm not sure that we do. The binaries that we ship will be built with Java 8 so our CI will be closer to what users will be running if we compile with Java 8 and test with JDK 14. This applies to our JDK 11 and 13 builds too, I just hadn't thought about it before.

[dreis2211]

Anything I can do to help? I'm a bit confused what you want me to do now ;-)

[dreis2211]

This applies to our JDK 11 and 13 builds too

I have trouble to actually see where the JDK 11+ builds do compile with JDK 8. Or do you mean that they do exactly the same as proposed here? And you simply wonder if that's the right approach?

Anyhow, I do see some value in compiling with the newer JDK versions as that will show deprecation warnings etc. E.g. compiling with JDK 11 shows warnings about using Class.newInstance() instead of Class.getDeclaredConstructor().newInstance().

For the javadocs, this overlaps with #17259 if you ask me. Again, I see some value in generating them with the respective versions as that will show eventual changes in behaviour better. Similar to what @snicoll mentioned there already.

In summary, being aware of changes on the JDK level - let it be on compiler, javadoc or test side - is imho beneficial. But I'm also fine with removing the javadoc & compile part if you really want me to.

[wilkinsona]

Sorry for the confusion, @dreis2211.

do you mean that they do exactly the same as proposed here? And you simply wonder if that's the right approach?

Yes, that's what I meant. I was trying to acknowledge that the problem I described with what's proposed here also already exists in our JDK 11 and 13 builds.

Anyhow, I do see some value in compiling with the newer JDK versions as that will show deprecation warnings etc. E.g. compiling with JDK 11 shows warnings about using Class.newInstance() instead of Class.getDeclaredConstructor().newInstance().

There's definitely some value. However, if we're going to pick one approach over the other, I think that testing Java 8-compiled code against later JDKs is of greater value as that's what our users will be doing. Feeling this way is also influenced by the fact that there's often nothing we can do about the deprecation warnings while we continue to support Java 8, although that's not the case with the example above.

A downside of compiling with JDK 8 and testing with a later JDK is that it complicates our CI set up. We currently have everything in place to produce Docker images with a single JDK in a well-known location. Adopting the approach I've described above would require our JDK 11, 13, and 14 images to have two JDKs in place. That's unavoidable for JDK 14 with Gradle's current capabilities no matter which approach we take so maybe it's not so bad.

[dreis2211]

That's unavoidable for JDK 14 with Gradle's current capabilities no matter which approach we take so maybe it's not so bad.

Exactly. It feels like we're discussing the bigger picture (which I enjoy) while this PR seems needed anyway for the time being and is only a tiny piece of the complete JDK 14 CI process...

[wilkinsona]

@dres2211 Thanks again for your work on this one. We discussed it and the broader topic today and concluded that we'd like two properties:

1. buildJavaHome for controlling the Java home that's used for the whole build. This should affect compilation, javadoc, and tests
2. testJavaHome for controlling the Java home that's used for tests.

We'll use testJavaHome on CI so that we can test Java 8-compiled byte code with later JDKs. This will cover what users are doing with our published binaries. We'll then use buildJavaHome on our own machines while exploring compatibility with JDKs that Gradle itself cannot yet run on.

This PR pretty much covers 1, other than renaming customJavaHome to buildJavaHome. If you have time to make that tweak, that'd be much appreciated, otherwise we can handle it as part of merging. We'll then add the other property as a separate task.

[dreis2211]

Sure...will do so in a couple of minutes.

[dreis2211]

Pushed

[wilkinsona]

Thanks for the updates, @dreis2211. I've polished this a bit (wilkinsona@563083b) and in doing so, I think I've noticed that CompileJava tasks aren't using the custom Java home or javac executable. I can run a build with -PbuildJavaHome=/whatever and test and javadoc tasks will fail due to a non-existent Java home/executable, but CompileJava tasks do not. I don't think I've introduced the problem while polishing, but I need to dig a bit more to be certain.

[wilkinsona]

And almost immediately after writing the above, I realised that we need to call compile.getOptions().setFork(true). As expected, failures now occur when the configured Java home does not exist:

Execution failed for task ':spring-boot-project:spring-boot-tools:spring-boot-loader-tools:compileJava'.
> Supplied javaHome must be a valid directory. You supplied: /whatever

[dreis2211]

@wilkinsona As I saw test failures I didn't question that the custom Java home was (or wasn't) used. Good catch! Sorry that I didn't catch it. :/

[wilkinsona]

Thanks once again, @dreis2211. The proposed changes are now on the master branch.