Simplify finding jps #45677
Simplify finding jps #45677
Conversation
This commit simplifies how we find jps to instead use built-in Gradle functionality for the same.
jasontedor
added
>non-issue
:Delivery/Build
|
Pinging @elastic/es-core-infra |
mark-vieira
left a comment
mark-vieira
left a comment
There was a problem hiding this comment.
One comment, otherwise LGTM.
| if (!jps.exists()) { | ||
| throw new GradleException("jps executable not found; ensure that you're running Gradle with the JDK rather than the JRE") | ||
| } | ||
| final File jps = Jvm.forHome(project.runtimeJavaHome).getExecutable('jps') |
There was a problem hiding this comment.
So project.runtimeJavaHome may or may not be Java home used to run the build. Since jps in this case is a build dependency, it seems to make sense to use the Java runtime that Gradle is using via Jvm.current(). Realistically, we should be able to set RUNTIME_JAVA_HOME to a JRE, since it's just used for execution bits, not anything build related.
There was a problem hiding this comment.
So
project.runtimeJavaHomemay or may not be Java home used to run the build.
Yup. The way I’ve always thought of it is:
- we have the JVM running Gradle
- we have the JVM compiling code (compiler Java home)
- we have the JVM running tests and all other executions that require a JVM (runtime Java home)
It’s not perfect (I think third-party audit runs in the Gradle JVM, although we want to move it out). I’d rather not break this mental model more though.
There was a problem hiding this comment.
Ah, giving this another think this makes sense. We should use the same version of jps that's used to launch any ES processes, even though it's actually the build that's executing jps here.
There was a problem hiding this comment.
ThirdPartyAuditTask does have a javaHome property which we set to runtimeJavaHome.
CheckForbiddenApis runs in the same jvm ( does not work ) and has a target compatibility specific
to runtimeJavaVersion
There was a problem hiding this comment.
Thanks for the correction @atorok, I knew there was a precommit task that runs in the Gradle JVM that we ideally would want running in the runtime Java home JVM, I couldn't remember which though.
There was a problem hiding this comment.
We actually are slowly migrating to the opposite @jasontedor. There's no reason for most precommit checks to run in the runtime JVM when javac, checkstyle and most other build-time verification runs using the Gradle JVM. These are "build" operations, not runtime ones so realistically they should run the the Gradle JVM. Also, forking them has a significant build time penalty as we are creating and throwing away lots of JVMs for each of these tasks.
This is a separate issue though and I think the usage of runtimeJavaHome is appropriate in this case. For others, we'll address them case-by-case but we are definitely misusing it in several places.
This commit simplifies how we find jps to instead use built-in Gradle functionality for the same.
As of elastic/elasticsearch#45677 build tools now uses a function that requires JavaHome to be a File type. This commit updates the JavaHome property to be a File type. This commit also bumps the latest supported version of build-tools (8.0.0-SNAPSHOT is not supported yet).
As of elastic/elasticsearch#45677 build tools now uses a function that requires JavaHome to be a File type. This commit updates the JavaHome property to be a File type.
5 participants
This commit simplifies how we find jps to instead use built-in Gradle functionality for the same.
Relates #45666