Use task configuration avoidance in gradle initialization #162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gradle task creation can be heavyweight when hundreds or thousands of instances of tasks exist. This is especially true in large multi project builds. Forbidden apis currently always creates a task per source set, as well as the alias `forbiddenApis` task. Gradle provides a way to lazily construct the task, and only configure when the task is actually needed for a given task graph. This commit switches the construction of the forbidden apis tasks to use task registration, when it is available (based on gradle version).
|
Hi, looks great. Did you test it already with Elasticsearch to ensure that it works as expected? |
|
I just merged it, but the "ant test-gradle" failed with Gradle 5.6: So something is not working correctly. |
|
Maybe the project.afterEvaluate has to be also moved outside? |
|
Hrm, i can't fix this. Should I revert the pull request. Sorry it does not work and I mreged it already. Not sure how to handle this I will let Jenkins also test with latest Gradle, bt at least Gradle 5.6 does not work (the one I have installed locally). |
|
I remeber that I tried the same like you did and failed for the same reason. So I gave up. I just have to find my branch where I tried the same. |
|
The reason for that problem is that you can't add a project.afterEvaluate, if the task is generated in a delayed way. Problem here is: During the lately created task's configuration, Gradle did not yet apply all settings, so the classesDirs property is still the one from the convention. If the build script has changed it, the change is not yet applied. I digged around: The only workaround is to hardly depend on sourceSet's output. So if task avoidance is enabled, it will always depend on the sourceSet's output, although the task may be configured to looks somewhere else for classes. IMHO this is also the correct thing to do. I leave the legacy behaviour for older Gradle versions: if (TASK_AVOIDANCE_AVAILABLE) {
// we have to setup the dependcy, as we don't know what the user does afterwards:
task.dependsOn(sourceSet.output);
} else {
// add dependency to compile task after evaluation, if the classesDirs collection has overlaps with our SourceSet:
project.afterEvaluate{
def sourceSetDirs = getSourceSetClassesDirs().files;
if (classesDirs.any{ sourceSetDirs.contains(it) }) {
task.dependsOn(sourceSet.output);
}
}
}Maybe as we are on version 3, let's simply remove the coolness. If somebody wants to inspect some other files outside of the sourceset, heshe should create the task from scratch. The code should look like this - much easier to understand and no magic: task.dependsOn(sourceSet.output);Opinions? |
uschindler
added a commit
that referenced
this pull request
Apr 9, 2020
…) which was way too clever. Just always depend on sourceSet's output unconditionally.
|
I removed the cleverness completely. Much easier to understand. |
|
As a followup, I refactored a bit of the plugin initialization and also made the Gradle test project that caused the issue above a little bit more self-contained. It now also compiles its sources before checking, so its more real-world like. See this commit: 3242ec4 |
|
Thanks @uschindler! I was in the middle of testing Elasticsearch; I had only created the PR once |
|
I tested it with 3.2 (minimum), 4.0 (changes regarding classesDir) and finally with 5.6 (latest on my laptop). Jenkins ran latest 6 release and also failed in same way: https://jenkins.thetaphi.de/job/Forbidden-APIs-testonly-GradleLatest-Linux/38/console Could you test latest master branch with Elasticsearch? To me it looks fine now, plugin startup code is also looking better now. The snapshot builds are also on sonatype, but easiest to build with ant from master and once it's installed in local maven repo you can use with Elasticsearch with that special Gradle My plan is to use gradle wrapper to go the testing with 3 versions (3.2. 4.0 and 4.9) and maybe latest. Which version did you use? Gradle support is unfortunately very fragile. |
Tested, and it works now. I finally found how to publish to maven local with
We are currently on gradle 6.3. |
|
Did you see a speed improvement by the task configuration avoidance? I think to see it, you need all plugins support it, right? |
|
Yes, anything that is explicitly configuring forbidden apis needs to switch to using TaskProviders, which means instead of: one does this: I did this in Elasticsearch along with using a 3.0 snapshot. There are 800+ forbidden apis tasks. On master, it takes about 0.25 seconds to configure those. On my branch it is shaved to 0.06 seconds. So yes, we do see the much of the speed improvement we were hoping for. |
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Apr 9, 2020
Currently forbidden apis accounts for 800+ tasks in the build. These tasks are aggressively created by the plugin. In forbidden apis 3.0, we will get task avoidance (policeman-tools/forbidden-apis#162), but we need to ourselves use the same task avoidance mechanisms to not trigger these task creations. This commit does that for our foribdden apis usages, in preparation for upgrading to 3.0 when it is released.
rjernst
added a commit
to elastic/elasticsearch
that referenced
this pull request
Apr 15, 2020
Currently forbidden apis accounts for 800+ tasks in the build. These tasks are aggressively created by the plugin. In forbidden apis 3.0, we will get task avoidance (policeman-tools/forbidden-apis#162), but we need to ourselves use the same task avoidance mechanisms to not trigger these task creations. This commit does that for our foribdden apis usages, in preparation for upgrading to 3.0 when it is released.
rjernst
added a commit
to elastic/elasticsearch
that referenced
this pull request
Apr 15, 2020
Currently forbidden apis accounts for 800+ tasks in the build. These tasks are aggressively created by the plugin. In forbidden apis 3.0, we will get task avoidance (policeman-tools/forbidden-apis#162), but we need to ourselves use the same task avoidance mechanisms to not trigger these task creations. This commit does that for our foribdden apis usages, in preparation for upgrading to 3.0 when it is released.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Gradle task creation can be heavyweight when hundreds or thousands of
instances of tasks exist. This is especially true in large multi project
builds. Forbidden apis currently always creates a task per source set,
as well as the alias
forbiddenApistask. Gradle provides a way tolazily construct the task, and only configure when the task is actually
needed for a given task graph.
This commit switches the construction of the forbidden apis tasks to use
task registration, when it is available (based on gradle version).
closes #145