Bundle java in distributions#38013
Conversation
Finding java on the path is sometimes confusing for users and unexpected, as well as leading to a different java being used than a user expects. This commit adds warning messages when starting elasticsearch (or any tools like the plugin cli) and using java found on the PATH instead of via JAVA_HOME.
Setting up a jdk is currently a required external step when installing elasticsearch. This is particularly problematic for the rpm/deb packages as installing a jdk in the same package installation command does not guarantee any order, so must be done in separate steps. Additionally, JAVA_HOME must be set and often causes problems in selecting a correct jdk when, for example, the system java is an older unsupported version. This commit bundles platform specific openjdks into each distribution. In addition to eliminating the issues above, it also presents future possible improvements like using jlink to build jdk images only containing modules that elasticsearch uses. closes elastic#31845
|
Pinging @elastic/es-core-infra |
|
I am still working on getting packaging tests right, but I opened this so reviewers can start looking at the general approach. |
alpar-t
left a comment
There was a problem hiding this comment.
Look good overall, left some comments.
| static Task configureExecTask(String name, Project project, Task setup, NodeInfo node, Object[] execArgs) { | ||
| return project.tasks.create(name: name, type: LoggedExec, dependsOn: setup) { Exec exec -> | ||
| exec.workingDir node.cwd | ||
| exec.environment 'JAVA_HOME', node.getJavaHome() |
There was a problem hiding this comment.
is there reason to keep node.javaHome around ?
Maybe at least print a warning for now to make it obvious it's not being picked up.
On second taught, I'm not sure we want to use the bundled java here. We are rotating runtime java version in CI for this AFAIK and I think we should keep that, so all the bundled vs non bundled Java
testing would be done in packaging tests only and rest tests will continue to use a java version as provided.
There was a problem hiding this comment.
I don't think there is a need to keep java home here. IMO the integration tests should be as true to the expected default setup as they can be. We will have packaging tests to ensure the override behavior works.
There was a problem hiding this comment.
My understanding is that with this change setting ES_RUNTIME_JAVA will no longer work, as getJavaHome() returns project.runtimeJavaHome when nothing specific is configured. So our CI tests that are supposed to test different runtime java versions like 8 and 12 will just use the bundled one.
There was a problem hiding this comment.
Note on this that I reversed my previous thinking and added support for RUNTIME_JAVA_HOME. This is because I have experienced a bug with jdk 11.0.2 on macos which prevents Elasticsearch from starting. I haven't been able to get anyone else to replicate this issue, but the support of RUNTIME_JAVA_HOME is the only thing that allows me to run integ tests on my laptop at all with this change.
| String jdkBuild = bundledJdkVersion[3] | ||
| repositories { | ||
| ivy { | ||
| url "https://download.java.net" |
There was a problem hiding this comment.
I'm worried about this in CI.
I assume this being a repository it does benefit from caching, and the artifacts will be pulled down by running the resolveAllDependencies task.
As long as we make sure that this is true I'm fine with the change.
There was a problem hiding this comment.
Yes, resolveAllDependencies is triggering the download:
:distribution:resolveAllDependencies > Resolve dependencies of :distribution:jdk_darwin > openjdk-11.0.1_osx-x64_bin.xml
| * Note the "patch" version is not yet handled here, as it has not yet | ||
| * been used by java. | ||
| */ | ||
| public static List<String> getBundledJdk() { |
There was a problem hiding this comment.
Too bad that Gradles JavaVersion doesn't have all this information.
Given how broadly this class could be used it might be better to wrap the list in a class, or move the parsing logic to the build script that uses it ?
There was a problem hiding this comment.
I don't think it should be used broadly? I'm fine moving it to distribution/build.gradle if you insist, but I think it makes sense here since VersionProperties is all about the versions.properties file where this setting lives.
There was a problem hiding this comment.
I'm ok to return it as a string from here and do the parsing in distribution/build.gradle since it's only needed there.
There was a problem hiding this comment.
We have the version of the bundled JDK in build-tools.
Rather than assuming that it will match compilerJavaHome I think it would make more sense to have this conditional based on that. If the runtimeJavaVersion matches the bundled JDK version that use that instead.
There was a problem hiding this comment.
The purpose of this conditional is to allow testing with Java home set. This is just checking whether runtime Java home was set, since if it wasn't it would be the same as the compiler Java. If we compared to the bundled version, we would likely never test the bundled jdk as devs aren't always on the same version on their machines as we are bundling.
There was a problem hiding this comment.
I wasn't thinking about matching the version exactly, just the major, since that's what our matrix CI jobs also care about and what's included in the reproduction lines.
I don't think the conditional would behave differently, but it would convey intent better without relying on an implementation detail in the build plugin.
There was a problem hiding this comment.
The intent is to test the default/common behavior by default. That is, to test the bundled jdk. If runtime java home is set, this tests whatever is there, whether is is an external 11 or 8. That seems straightforward to me.
There was a problem hiding this comment.
I wouldn't have a problem with that, but we are not testing directly if ES_RUNTIME_JAVA is passed in, but rely on knowledge about how this is passed along instead. I think that these are worth avoiding because it makes the build harder to maintain. In other words looking at just this line and ignoring everything else you know about the build would make you wonder why the java used for compilation would matter in weather we do or do not use the bounded JDK ?
There was a problem hiding this comment.
@atorok Tests are passing now. Do you have a concrete suggestion here? I do not want to compare anything about runtimeJavaHome to the version being bundled. Just because they are the same major version of java does not mean they are the same.
There was a problem hiding this comment.
I added an extension property to indicate whether runtime java home is set in
29ec298
|
@elasticmachine run elasticsearch-ci/1 |
|
@elasticmachine |
|
@elasticmachine |
* Bundle java in distributions Setting up a jdk is currently a required external step when installing elasticsearch. This is particularly problematic for the rpm/deb packages as installing a jdk in the same package installation command does not guarantee any order, so must be done in separate steps. Additionally, JAVA_HOME must be set and often causes problems in selecting a correct jdk when, for example, the system java is an older unsupported version. This commit bundles platform specific openjdks into each distribution. In addition to eliminating the issues above, it also presents future possible improvements like using jlink to build jdk images only containing modules that elasticsearch uses. closes elastic#31845
* Bundle java in distributions Setting up a jdk is currently a required external step when installing elasticsearch. This is particularly problematic for the rpm/deb packages as installing a jdk in the same package installation command does not guarantee any order, so must be done in separate steps. Additionally, JAVA_HOME must be set and often causes problems in selecting a correct jdk when, for example, the system java is an older unsupported version. This commit bundles platform specific openjdks into each distribution. In addition to eliminating the issues above, it also presents future possible improvements like using jlink to build jdk images only containing modules that elasticsearch uses. closes elastic#31845
* Bundle java in distributions Setting up a jdk is currently a required external step when installing elasticsearch. This is particularly problematic for the rpm/deb packages as installing a jdk in the same package installation command does not guarantee any order, so must be done in separate steps. Additionally, JAVA_HOME must be set and often causes problems in selecting a correct jdk when, for example, the system java is an older unsupported version. This commit bundles platform specific openjdks into each distribution. In addition to eliminating the issues above, it also presents future possible improvements like using jlink to build jdk images only containing modules that elasticsearch uses. closes #31845
* Bundle java in distributions Setting up a jdk is currently a required external step when installing elasticsearch. This is particularly problematic for the rpm/deb packages as installing a jdk in the same package installation command does not guarantee any order, so must be done in separate steps. Additionally, JAVA_HOME must be set and often causes problems in selecting a correct jdk when, for example, the system java is an older unsupported version. This commit bundles platform specific openjdks into each distribution. In addition to eliminating the issues above, it also presents future possible improvements like using jlink to build jdk images only containing modules that elasticsearch uses. closes #31845
Setting up a jdk is currently a required external step when installing
elasticsearch. This is particularly problematic for the rpm/deb packages
as installing a jdk in the same package installation command does not
guarantee any order, so must be done in separate steps. Additionally,
JAVA_HOME must be set and often causes problems in selecting a correct
jdk when, for example, the system java is an older unsupported version.
This commit bundles platform specific openjdks into each distribution.
In addition to eliminating the issues above, it also presents future
possible improvements like using jlink to build jdk images only
containing modules that elasticsearch uses.
closes #31845