Add gradle plugin for downloading jdk #41461

rjernst · 2019-04-23T22:24:59Z

We currently download 3 variants of the same version of the jdk for
bundling into the distributions. Additionally, the vagrant images do
their own downloading. This commit moves the jdk downloading into a
utility gradle plugin. This will be used in a future PR by the packaging
tests.

The new plugin exposes a "usesJdk" method on tasks. Once the jdk version
and platform are set for a task, a "jdkHome" property exists which may
be used to get the extracted jdk directory at runtime.

We currently download 3 variants of the same version of the jdk for bundling into the distributions. Additionally, the vagrant images do their own downloading. This commit moves the jdk downloading into a utility gradle plugin. This will be used in a future PR by the packaging tests. The new plugin exposes a "usesJdk" method on tasks. Once the jdk version and platform are set for a task, a "jdkHome" property exists which may be used to get the extracted jdk directory at runtime.

elasticmachine · 2019-04-23T22:25:01Z

Pinging @elastic/es-core-infra

alpar-t · 2019-04-24T16:12:17Z

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java

+                    if (ALLOWED_PLATFORMS.contains(platform) == false) {
+                        throw new IllegalArgumentException("platform must be one of " + ALLOWED_PLATFORMS);
+                    }
+                    ExtraPropertiesExtension extraProperties = task.getExtensions().findByType(ExtraPropertiesExtension.class);


nit: could be moved up so it's repeated on line 60

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java

alpar-t · 2019-04-24T16:14:59Z

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java

+                    }
+
+                    // ensure a root level jdk download task exists
+                    setupRootJdkDownload(project.getRootProject(), platform, version);


Could we set up a single task to download all as we do for test clusters rather than one for each version ? Having fewer tasks will make configuration faster and I find it easier to follow what's going on.
It could also be a lazily created task that's always set up and then have the tasks that require it depend on it

I might be missing it, but is task made dependent on this ?

I do not think we should have a single configuration for all jdks we download. That means resolving the configuration triggers download of all of them. Additionally, the way this works is to be able to resolve a configuration to a single file.

is task made dependent on this ?

The root project does the extraction, and makes the extracted dir available as an artifact of the root project. The tasks in subprojects add a normal dependency on that "artifact", through a separate configuration in the subproject, and the task then depends on that local subproject configuration.

I wasn't referring to having a single configuration, we actually need those to be separate to be able to have different versions, but we can extract all in the same task, having multiple tasks for it doesn't buy us anything.

Having a single task would mean the task depends on all those configurations, thus still resolving all the artifacts at the same time. We need a 1-1 mapping to ensure one extraction is done.

Is that all that bad? The configurations are cached, and the task has up to date checks . For most things this all the JDKs will be needed anyway due to the number of tests that depend on the JDK.

For most things this all the JDKs will be needed anyway

I don't think that is true. When I run a packaging test for a single distro, I need one jdk, not many, and certainly not the ones from non linux.

alpar-t · 2019-04-24T16:27:29Z

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java

+                ivyRepo.setName(repoName);
+                ivyRepo.setUrl("https://download.java.net");
+                ivyRepo.patternLayout(layout -> {
+                    layout.artifact("java/GA/jdk" + jdkMajor + "/" + jdkBuild + "/GPL/openjdk-[revision]_[module]-x64_bin.[ext]");


we could add something like:

spec.content(c-> c.includeGroup("org.elasticsearch.jdk-downloader"));

And use "org.elasticsearch.jdk-downloader" as a group when downloading the dependency.
It's a good practice for repositories confirmed by plugins.

Sorry I don't follow. The group is completely arbitrary and internal since this is a fake ivy repo.

But the repository is not, so all dependencies could potentially be checked against it.
I just suggested org.elasticsearch.jdk-downloader to make it more specific, or make it "ours" since it's not really used in the pattern layout. It would work the same way with "jdk".

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java

rjernst · 2019-04-25T17:58:34Z

@atorok I wasn't happy with usesJdk on every task, so I rethought this and have pushed an alternative approach. The new code has a jdks container extension on the project where each jdk used has a name. Then depending on the jdk in a task is as simple as dependsOn jdks.myjdk, and using it eg for cli args is args jdks.myjdk.homeDir and to copy the files from jdks.myjdk.files.

rjernst · 2019-04-25T17:59:09Z

Note that I need to update the tests with the new approach, but wanted to get this up here for you to look at if you have time today.

rjernst · 2019-04-26T15:44:21Z

@elasticmachine run elasticsearch-ci/1

mark-vieira

Some misc comments, otherwise this looks great.

mark-vieira · 2019-04-30T22:19:13Z

buildSrc/src/main/java/org/elasticsearch/gradle/Jdk.java

+import java.util.List;
+import java.util.regex.Pattern;
+
+public class Jdk implements Buildable {


To be more explicit we could also implement Named here.

I'm happy to add this. Can you explain what benefits it would give us here?

It's just a bit more explicit (getName() will no be an @Override method) and I believe I recall some Gradle APIs that handle a Named explicitly.

mark-vieira · 2019-04-30T22:22:53Z

buildSrc/src/main/java/org/elasticsearch/gradle/Jdk.java

+public class Jdk implements Buildable {
+
+    static final Pattern VERSION_PATTERN = Pattern.compile("(\\d+)(\\.\\d+\\.\\d+)?\\+(\\d+)");
+    private static final List<String> ALLOWED_PLATFORMS = Collections.unmodifiableList(Arrays.asList("linux", "windows", "darwin"));


Why not use an enum then?

We have something similar: org.elasticsearch.gradle.OS

I don't think an enum has any advantages here. In fact, it would require more unnecessary code to convert between the capitalized enum entities and the lowercase platform names we need here.

mark-vieira · 2019-04-30T22:23:39Z

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadExtension.java

+import java.io.File;
+import java.util.function.Supplier;
+
+public class JdkDownloadExtension {


This looks to be unused. Seems we went the domain objection container route instead.

buildSrc/src/main/java/org/elasticsearch/gradle/Jdk.java

mark-vieira · 2019-04-30T22:24:56Z

buildSrc/src/main/java/org/elasticsearch/gradle/Jdk.java

+        return configuration;
+    }
+
+    public FileCollection getFiles() {


I'd rename this getFileCollection(). as FileCollection itself already has a method called getFiles() which returns a Set<File>.

mark-vieira · 2019-04-30T22:53:09Z

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java

+        if (rootProject.getRepositories().findByName(repoName) == null) {
+            rootProject.getRepositories().ivy(ivyRepo -> {
+                ivyRepo.setName(repoName);
+                ivyRepo.setUrl("https://download.java.net");


Why don't we use jvm-catalog.elastic.co instead? It has a much simpler URL pattern that would allow us to avoid having to create a repo per version as well. If we use a number of versions we are going to make a lot of HEAD requests that return a 404 with this approach.

This would add a considerable load to our infrastructure. We may want to still consider it, but I think it should be separate from this PR, which is just to consolidate existing code, which already uses this url.

I still have a bit of a concern with this approach of registering so many repositories. Is there anyway perhaps that we could construct the dependency notation so that we can avoid creating a repo per version? I'm not sure this is possible, or if what I see here even works. For example, the download link I see here includes the artifact hash in the download URL.

I don't think it is possible here. The reason we embed this information in the pattern is we only have so many elements we can substitute via the maven/ivy coordinates.

The hash included in the download link is something that changed very recently. There are 2 versions registered here because versions before 12.0.1 still use the old pattern. I could definitely decide which one to register based on the version, but in this PR I'm only trying to move the logic we already have in the build for jdk downloading. We can certainly look at improving this either through reducing repos or moving to using the jvm catalog, but I think these should be discussed separately as followups to this PR.

Since we're only registering all these repos to the root project it's probably ok, as it won't affect dependency solution of subprojects. Let's make sure we create an issue to follow up here. I assume if the recent version download links have changed this is going to be a problem for fetching newer JDK versions, no?

We register 2 ivy repositories, one with the older pattern, and one with the hash. Are you sure you are looking at the latest commit?

I definitely was not. It seems GitHub was filtering out the latest commits for me. Thanks GH!

Given that we know the version when registering the repo can we be smart about it and avoid registering both for all versions?

Or simpler yet, if the artifact version notation includes a hash, use the hash repo, otherwise use the legacy one.

Sure, I will do that in a followup.

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java

mark-vieira · 2019-04-30T23:09:58Z

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java

+        int rootNdx = platform.equals("darwin") ? 2 : 1;
+        Action<CopySpec> removeRootDir = copy -> {
+            // remove extra unnecessary directory levels
+            copy.eachFile((FileCopyDetails details) -> {


Is this cast to FileCopyDetails necessary?

alpar-t · 2019-05-02T13:39:56Z

buildSrc/src/testKit/jdk-download/subproj/build.gradle

+
+String fakeJdkVersion = Objects.requireNonNull(System.getProperty('tests.jdk_version'))
+jdks {
+    linux_jdk {


nit: could we remove _jdk from the name to avoid redundancy ? And keep that as the naming format in general.

This is just an example in a test, but I removed it.

alpar-t · 2019-05-02T13:48:44Z

@atorok I wasn't happy with usesJdk on every task, so I rethought this and have pushed an alternative approach. The new code has a jdks container extension on the project where each jdk used has a name. Then depending on the jdk in a task is as simple as dependsOn jdks.myjdk, and using it eg for cli args is args jdks.myjdk.homeDir and to copy the files from jdks.myjdk.files.

@rjernst ++ I like the approach very much !

You might be able to add some additional magic with Object#asType(Class).
I'm not 100% sure if that helps or just creates more confusion, but you would be able to always pass just jdks.myjdk.

rjernst · 2019-05-06T19:24:07Z

You might be able to add some additional magic with Object#asType(Class).

Thanks for the idea! While I didn't use asType, I was able to eliminate homeDir and fileCollection by making Jdk implement toString and Iterable.

rjernst · 2019-05-07T01:14:57Z

@atorok @mark-vieira I believe I have addressed all your feedback.

alpar-t

LGTM

We might still need an asType(File) to be able to pass a JDK to an API that expects a File, but we can add that whenever we need it.

mark-vieira · 2019-05-07T15:59:54Z

Yeah, I'm on the fence on using asType(). It still would require you to do something like myJdk as File which I'm not certain is significantly better than myJdk.file. I'm generally in favor of using the least amount of Groovy magic as possible unless it makes the DSL significantly cleaner.

* Update TLS ciphers and protocols for JDK 11 (elastic#41385) This commit updates the default ciphers and TLS protocols that are used after the minimum supported JDK is JDK 11. The conditionals around TLSv1.3 and 256-bit cipher support have been removed. JDK 11 no longer requires an unlimited JCE policy file for 256 bit cipher support and TLSv1.3 is supported in JDK 11+. New cipher support has been introduced in the newer JDK versions as well. The ciphers are ordered with PFS ciphers being most preferred, then AEAD ciphers, and finally those with mainstream hardware support. * Fixes for TLSv1.3 on JDK11 * fix for JDK-8212885

…tic#41893) The class was called PatternSeenEventExcpectation. This commit is a straight class rename to correct the spelling.

rjernst · 2019-05-07T18:05:03Z

@elasticmachine run elasticsearch-ci/1

rjernst · 2019-05-07T19:44:35Z

@elasticmachine run elasticsearch-ci/packaging

rjernst · 2019-05-08T01:25:20Z

@elasticmachine run elasticsearch-ci/1

We currently download 3 variants of the same version of the jdk for bundling into the distributions. Additionally, the vagrant images do their own downloading. This commit moves the jdk downloading into a utility gradle plugin. This will be used in a future PR by the packaging tests. The new plugin exposes a "jdks" project extension which allows creating named jdks. Once the jdk version and platform are set for a named jdk, the jdk object may be used as a lazy String for the jdk home path, or a file collection for copying.

rjernst added >enhancement :Delivery/Build Build or test infrastructure :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v8.0.0 v7.2.0 labels Apr 23, 2019

simplify

4f1b693

rjernst requested a review from alpar-t April 24, 2019 15:02

alpar-t reviewed Apr 24, 2019

View reviewed changes

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java Outdated Show resolved Hide resolved

alpar-t reviewed Apr 24, 2019

View reviewed changes

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java Show resolved Hide resolved

alpar-t reviewed Apr 24, 2019

View reviewed changes

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java Show resolved Hide resolved

alpar-t reviewed Apr 24, 2019

View reviewed changes

buildSrc/src/main/java/org/elasticsearch/gradle/JdkDownloadPlugin.java Outdated Show resolved Hide resolved

rjernst added 5 commits April 24, 2019 10:25

review feedback

8b5c7d9

back to ext props

e57df7e

use an "extension", but not a real extension

486fa22

fix tests

cd24b92

alternative

5bb4142

rjernst added 2 commits April 25, 2019 14:47

tests

c000b33

fix checkstyle

7242a05

mark-vieira requested changes Apr 30, 2019

View reviewed changes

alpar-t reviewed May 2, 2019

View reviewed changes

Merge branch 'master' into jdk_download

1b0e7cd

rjernst added 3 commits May 6, 2019 10:41

address feedback

13ac874

fix oops

127b3b7

make jdk useable as project.files and lazy string

97885a9

mark-vieira approved these changes May 7, 2019

View reviewed changes

alpar-t approved these changes May 7, 2019

View reviewed changes

jaymode and others added 3 commits May 7, 2019 10:24

Correct spelling of MockLogAppender.PatternSeenEventExpectation (elas…

502b395

…tic#41893) The class was called PatternSeenEventExcpectation. This commit is a straight class rename to correct the spelling.

Merge branch 'master' into jdk_download

f985df5

jasontedor force-pushed the master branch from 1b80e0d to 8af01df Compare May 7, 2019 23:35

rjernst added 2 commits May 7, 2019 17:15

Merge branch 'master' into jdk_download

0d3e0f0

remove bad files

caf3246

Merge branch 'master' into jdk_download

42cf91c

rjernst merged commit c7db902 into elastic:master May 8, 2019

rjernst deleted the jdk_download branch May 8, 2019 14:16

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Add gradle plugin for downloading jdk #41461

Add gradle plugin for downloading jdk #41461

Conversation

rjernst commented Apr 23, 2019

elasticmachine commented Apr 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Apr 25, 2019

rjernst commented Apr 25, 2019

rjernst commented Apr 26, 2019

mark-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira May 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented May 2, 2019

rjernst commented May 6, 2019

rjernst commented May 7, 2019

alpar-t left a comment

Choose a reason for hiding this comment

mark-vieira commented May 7, 2019

rjernst commented May 7, 2019

rjernst commented May 7, 2019

rjernst commented May 8, 2019

alpar-t Apr 24, 2019 •

edited

Loading

mark-vieira May 6, 2019 •

edited

Loading