Simplify java home verification #55635
Conversation
At one time, all uses of java home were found through the getJavaHome utility method on BuildPlugin. However, that was changed many refactorings ago, but the complex support for registering a java home version needed that fails at configuration time still exists. The only remaining use of grabbing java home is within bwc tests, and must be at runtime since that is when we have the checkout and know what version is needed. This commit consolidates the java home finding method into a utility unassociated with BuildPlugin.
rjernst
added
:Delivery/Build
|
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
Doesn't this now mean that if you try to run one of these BWC tasks and don't have the correct environment variable set you won't get an error until task execution time? Before we'd error at graph calculation time, giving folks and opportunity to fix the issue before the build kicked off and failed late. |
We did long ago, but we dont' anymore. Notice that the method is called from within a doFirst. |
|
Note that this changed when we moved from hardcoding the java version to use inside bwc/build.gradle to extracting the version from the bwc checkout. |
mark-vieira
left a comment
mark-vieira
left a comment
There was a problem hiding this comment.
Couple minor comments, otherwise LGTM. Thanks for moving this out of BuildPlugin. We will soon wrangle this beast.
| if (java.isEmpty()) { | ||
| throw new GradleException("JAVA" + version + "_HOME required to run task " + task.getPath()); | ||
| } | ||
| return java.get().getJavaHome().get().getAbsolutePath(); |
There was a problem hiding this comment.
This could be simplified to java.orElseThrow(() -> new GradleException("message"))
| public class JavaUtil { | ||
|
|
||
| /** A convenience method for getting java home for a version of java and requiring that version for the given task to execute */ | ||
| static String getJavaHome(final Task task, final int version) { |
There was a problem hiding this comment.
Give we are ditching all the task graph stuff I think we can even just avoid the Task parameter here. If this blows up at runtime in a task action the exception cause chain will include the failing task so no need to have it here just for the purpose of including it in the exception message.
* Simplify java home verification At one time, all uses of java home were found through the getJavaHome utility method on BuildPlugin. However, that was changed many refactorings ago, but the complex support for registering a java home version needed that fails at configuration time still exists. The only remaining use of grabbing java home is within bwc tests, and must be at runtime since that is when we have the checkout and know what version is needed. This commit consolidates the java home finding method into a utility unassociated with BuildPlugin. * fix checkstyle * address feedback
* Simplify java home verification At one time, all uses of java home were found through the getJavaHome utility method on BuildPlugin. However, that was changed many refactorings ago, but the complex support for registering a java home version needed that fails at configuration time still exists. The only remaining use of grabbing java home is within bwc tests, and must be at runtime since that is when we have the checkout and know what version is needed. This commit consolidates the java home finding method into a utility unassociated with BuildPlugin. * fix checkstyle * address feedback
4 participants
At one time, all uses of java home were found through the getJavaHome
utility method on BuildPlugin. However, that was changed many
refactorings ago, but the complex support for registering a java home
version needed that fails at configuration time still exists. The only
remaining use of grabbing java home is within bwc tests, and must be at
runtime since that is when we have the checkout and know what version is
needed.
This commit consolidates the java home finding method into a utility
unassociated with BuildPlugin.