Skip to content

Simplify finding jps #45677

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.

Sign up for GitHub

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

Merged
jasontedor merged 1 commit into from
Aug 16, 2019
Merged

Simplify finding jps #45677

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 4 additions & 14 deletions buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ package org.elasticsearch.gradle.test
import org.apache.tools.ant.DefaultLogger
import org.apache.tools.ant.taskdefs.condition.Os
import org.elasticsearch.gradle.BuildPlugin
import org.elasticsearch.gradle.BwcVersions
import org.elasticsearch.gradle.LoggedExec
import org.elasticsearch.gradle.Version
import org.elasticsearch.gradle.BwcVersions
import org.elasticsearch.gradle.VersionProperties
import org.elasticsearch.gradle.plugin.PluginBuildPlugin
import org.elasticsearch.gradle.plugin.PluginPropertiesExtension
Expand All @@ -39,11 +39,13 @@ import org.gradle.api.logging.Logger
import org.gradle.api.tasks.Copy
import org.gradle.api.tasks.Delete
import org.gradle.api.tasks.Exec
import org.gradle.internal.jvm.Jvm

import java.nio.charset.StandardCharsets
import java.nio.file.Paths
import java.util.concurrent.TimeUnit
import java.util.stream.Collectors

/**
* A helper for creating tasks to build a cluster that is used by a task, and tear down the cluster when the task is finished.
*/
Expand Down Expand Up @@ -887,15 +889,7 @@ class ClusterFormationTasks {
onlyIf { node.pidFile.exists() }
// the pid file won't actually be read until execution time, since the read is wrapped within an inner closure of the GString
ext.pid = "${ -> node.pidFile.getText('UTF-8').trim()}"
File jps
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
jps = getJpsExecutableByName(project, "jps.exe")
} else {
jps = getJpsExecutableByName(project, "jps")
}
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')
Copy link
Contributor

@mark-vieira mark-vieira Aug 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jasontedor jasontedor Aug 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So project.runtimeJavaHome may 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.

Copy link
Contributor

@mark-vieira mark-vieira Aug 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jasontedor jasontedor Aug 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precisely.

Copy link
Contributor

@alpar-t alpar-t Aug 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jasontedor jasontedor Aug 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mark-vieira mark-vieira Aug 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

commandLine jps, '-l'
standardOutput = new ByteArrayOutputStream()
doLast {
Expand All @@ -914,10 +908,6 @@ class ClusterFormationTasks {
}
}

private static File getJpsExecutableByName(Project project, String jpsExecutableName) {
return Paths.get(project.runtimeJavaHome.toString(), "bin/" + jpsExecutableName).toFile()
}

/** Adds a task to kill an elasticsearch node with the given pidfile */
static Task configureStopTask(String name, Project project, Object depends, NodeInfo node) {
return project.tasks.create(name: name, type: LoggedExec, dependsOn: depends) {
Expand Down