-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Improvement usage of gradle task avoidance api #56627
Improvement usage of gradle task avoidance api #56627
Conversation
fee92e5
to
dd497ec
Compare
@elasticmachine test this please |
4cedb32
to
3b61b94
Compare
@elasticmachine test this please |
a8d1c06
to
30d4542
Compare
@elasticmachine test this please |
5e6334c
to
d3112a4
Compare
19d2457
to
85ee4b9
Compare
@@ -1,5 +1,5 @@ | |||
jar { | |||
archiveName = "${project.name}.jar" | |||
archiveFileName = "${project.name}.jar" |
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.
fix deprecation warning on the way
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
@elasticmachine update branch |
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.
Couple of comments but otherwise LGTM.
Got any numbers of config time improvement, if any?
@@ -485,7 +485,7 @@ allprojects { | |||
if (realTask == null) { | |||
return | |||
} | |||
project.tasks.create(taskName) { | |||
project.tasks.register(taskName) { |
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.
Is this strictly beneficial since this task rule should only run when it finds a task matching this naming convention, right? Granted, it doesn't hurt, just wondering.
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.
In general I think we just should use register over create where possible. In a later step we need to revisit the actual behaviour and when they got materialised anyhow, but more on a per case fashion I think as thinks get complex there.
There are use cases where tasks in a rule would not be created but registered. You can depend on tasks that are created by a rule, e.g. you could have `myTask.dependsOn tasks.named("cleanCompileJava"). The rule would kick in so the task would be registered, but it doesn't need to be created.
stop.doLast { | ||
project.delete(fixture.pidFile) | ||
TaskProvider<Exec> stop = project.tasks.register("${name}#stop", LoggedExec) | ||
stop.configure{ |
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.
nit: missing space stop.configure {
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.
fixed
|
||
// TODO: The task configuration block has side effects that require it currently to be always executed. | ||
// Otherwise tests start failing. Therefore we enforce the task creation for now. | ||
tsk.get() |
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.
Should we just use create()
then? This seems even more confusing.
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.
keeping create()
might look like an oversight. I think this way makes it more explicit that there is something wrong with the task setup.
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.
Cool, let's create an issue to look into what the task configuration block is doing that's required for test setup.
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.
done here: #56948
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.
Changes LGTM 👍
Use gradle task avoidance api wherever it is possible as a drop in replacement in the es build
Use gradle task avoidance api wherever it is possible as a drop in replacement in the es build
Use gradle task avoidance api wherever it is possible as a drop in replacement in the es build
back ported |
Use gradle task avoidance everywhere we generate a task.
partially addresses Gradle task avoidance api should be used consistently in the build #56610
Moving from
task.create()
totasks.register()
means that the linked configuration block is not always executed. That leads to side effects in some parts of the elasticsearch build that require more rework of how certain tasks and plugins are cutted. (e.g. how Rest testing is setup). This is out of scope of this PR.also not in scope: minimizing task realisation. We should optimise for that once we finished using the api consistently.