-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Update build to use gradle wrapper 6.8 #65596
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.
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
Conversation
8a7687d
to
c26b3cb
Compare
Raised gradle/gradle#15392 at gradle to handle the windows issues we see |
@elasticmachine test this please |
b4a4e3b
to
52a70d8
Compare
c0dcdc3
to
c654b22
Compare
project.getTasks().findByName("generateNotice") | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test class is not working in a unit test context anymore. The only non ignored test checked for the existence of the registered extension. Thats not enough for me to have this ported to an integ tests at the moment so I removed the unit test.
456edfd
to
295be18
Compare
Pinging @elastic/es-delivery (Team:Delivery) |
That has been fixed in Gradle 6.8-rc-3 |
@elasticmachine test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments, otherwise LGTM.
* We resolve all available java versions using auto detected by gradles tool chain | ||
* To make transition more reliable we only take env var provided installations into account for now | ||
*/ | ||
private List<JavaHome> getAvailableJavaVersions() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bit we lose here is that we no longer enorce that the environment variable name matches the JDK version it points at. I guess this would later fail w/ us not being able to resovle a compatible JDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. is that something you think is worth to invest in? I guess those issues are rare, but when they occur they suck to be debugged I can imagine. Maybe we want to look into this safety net in a follow up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think realistically what we want to do is remove the reliance on these environment variables and use Gradle's toolchain capabilities to just download a compatible JDK when necessary. These variables are only used for build time purposes, where we intend to run tests with a particular JDK implementation, that's what RUNTIME_JAVA_HOME is for.
So I think we can punt here and instead focus on ditching all usages of these things vs resolving JDKs dynamically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look into this in a separate PR. I didn't want to make too many changes in this PR. So RUNTIME_JAVA_HOME
is basically used to configure the jdk on CI to run all tests against?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I see at the moment is that the compatibility choices (which vendor is available etc.) is limited in gradle at the moment and might not be enough. If you say that's less of an issue for build time
then the toolchain capabilities in gradle might be enough at the moment to pick a proper one
@@ -60,6 +63,10 @@ abstract class AbstractGradleFuncTest extends Specification { | |||
true | |||
} | |||
|
|||
def assertOutputContainsNot(String givenOutput, String expected) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertOutputDoesNotContain
or assertOutputMissing
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. assertOutputMissing
it is
@@ -105,7 +105,9 @@ private void runInsecureArtifactRepositoryTest(final String name, final String u | |||
.withProjectDir(tmpDir.getRoot()) | |||
.withArguments("clean", "hello", "-s", "-i", "--warning-mode=all", "--scan") | |||
.withPluginClasspath() | |||
.forwardOutput() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep this here or was this only for testing? I'm ok with keeping this around across the board BTW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it.
@@ -32,6 +32,9 @@ | |||
public TaskProvider<? extends Task> createTask(Project project) { | |||
Configuration jarHellConfig = project.getConfigurations().create("jarHell"); | |||
if (BuildParams.isInternal() && project.getPath().equals(":libs:elasticsearch-core") == false) { | |||
// ideally we would configure this as a default dependency. But Default dependencies do not work correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine anyway as we don't expect this to be replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other question though. How does this work for external builds? Where does the jarhell implementation come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would need to add a dependency like `jarHell 'org.elasticsearch:elasticsearch-core:7.10' at the moment. In general I think we need to put some more dedicated effort into making these external available plugins work nicely for contributors. By that I mean having proper test coverage of third party plugin author use cases etc. I remember we talked about that very early on when having our first talks about the elastic build already.
We're moving in that direction with having internal plugins nowadays but there's more work required here
- resolve available gradle versions using built-in toolchain support - fixes deprecated usage of JavaInstallationRegistry
- with gradle 6.8 artifact transforms output is kept in build cache - therefore there is no skipping info seen in our tests
f717ed4
to
77dbe62
Compare
- Updates our build to use the latest Gradle 6.8 release which is the last release before the major 7.0 release. - Resolve available gradle versions using built-in toolchain support - Fixes deprecated usage of JavaInstallationRegistry - We replace jdk handling in our build to rely on jvm detection provided by the gradle build tool itself. As we rely on environment variables pointing to jdks we wire this into the gradle jdk detection mechanism
Uh oh!
There was an error while loading. Please reload this page.