Add individual precommit task plugins #56926

rjernst · 2020-05-18T22:04:50Z

Each precommit task is currently registered inside the shared
PrecommitTasks class. Having a single class with all precommit tasks
makes individualizing which precommit tasks are needed based on type of
project difficult, where we often just disable somet tasks eg for all qa
projects. This commit creates plugins for each precommit task, and moves
the configuration of the task into each plugin. PrecommitTasks remains
for now, and just delegates to each plugin, but will be removed in a
future change.

Each precommit task is currently registered inside the shared PrecommitTasks class. Having a single class with all precommit tasks makes individualizing which precommit tasks are needed based on type of project difficult, where we often just disable somet tasks eg for all qa projects. This commit creates plugins for each precommit task, and moves the configuration of the task into each plugin. PrecommitTasks remains for now, and just delegates to each plugin, but will be removed in a future change.

elasticmachine · 2020-05-18T22:05:04Z

Pinging @elastic/es-core-infra (:Core/Infra/Build)

breskeby

This is great and definitely the right way to go something is dying in me every time I see a task.enabled = false :)

I wonder if we should rethink the term precommit here as we implies this is ONLY executed pre commit. But all those things should and do also run on CI. In other builds I often saw the term sanityCheck. I'm not saying we need to fix this here as part of that PR but just food for thought / discussion.

breskeby · 2020-05-19T10:32:35Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/PrecommitPlugin.java

+/**
+ * Base plugin for adding a precommit task.
+ */
+public abstract class PrecommitPlugin implements Plugin<Project> {


I think it would be better to not extend this Plugin in other PrecommitPlugins but use composition here. So we would have a PreCommit plugin that is applied by all other specific PreCommitPlugins (e.g. the CheckstylePreCommitPlugin). The PreCommitPlugin would only create a 'precommit' task that depends on all tasks of a certain marker type like tasks.withType(PreCommitTask.class). The other specific precommit plugins would only need to ensure that the tasks they create are of type PreCommitTask which can be just common interface.
Also this maybeRegister method is relative expensive.

Thinking more about it an alternative would be if the specific PreCommit plugins are just responsible to wire things against the precommit task. That way you could skip the specific common task type. so these plugins would look always like

... apply(PreCommitPlugin) // register tasks I want to wire tasks.named("precommit").configure { dependsOn 'specificTask; } ...

I think it would be better to not extend this Plugin in other PrecommitPlugins but use composition here

Using inheritance allows us ensure the correct wirings happen. With composition, we can forget to hook up the new task to precommit, but with inheritance we enforce this because the class and createTask methods are abstract. Additionally, it wouldn't make sense to have precommit alone, it only makes sense in the context of having a concrete task like forbiddenapis.

The other specific precommit plugins would only need to ensure that the tasks they create are of type PreCommitTask

We can't guarantee this since some tasks are external, like forbidden apis or checkstyle.

I see. I would still split out the generation of the precommit task to a small (internal) plugin (e.g. PrecommitBase Plugin. By internal I mean we would not need to provide a public id for the plugin or even make it package protected. And here we would just apply this base plugin. This way we

a) ensure this task registration is done in a gradle idiomatic way
b) ensure its only applied by the specific PreCommit Plugins.

this is a common pattern in gradle where a plugin just provides a lifecycle task and other plugins hook into this lifecycle.

I think a separate plugin that creates the precommit task might be the way to go as well. Primary reason being that maybeRegister does exception-based flow control, which is incredibly expensive and basically negates all benefits of lazy tasks. In fact, I'd recommend we completely remove that method so folks don't use it. We are throwing hundreds of exceptions here during configuration time. Filling all those stack frames has a big penalty.

👍 small general thought on findByName. findByName often is an indicator of weak modelling that can be improved. I think in the build there's a lot of logic that is based on tasks that are around which should be more about what type of project (which plugins are applied) are we dealing with.

findByName often is an indicator of weak modelling that can be improved
That's definitely true. If you have to check if a task exists that indicates a code smell.

After chatting with Ryan I think I'm fine with plugin class extension. I'm less concerned about extending plugins when the superclass is abstract, that at least indicates this is purposeful. Extending non-abstract plugins is a no-no, and composition should be used instead as you say.

We can then have the abstract plugin apply a plugin which creates the precommit task that way Gradle handles idempotency for us rather than using maybeRegister.

We can then have the abstract plugin apply a plugin which creates the precommit task that way Gradle handles idempotency for us rather than using maybeRegister.

👍

I pushed b5dff57 which no longer uses maybeCreate. Instead we have an inner plugin that creates the task, but we still use inheritance to ensure we don't forget to add a task to it.

rjernst · 2020-05-19T13:14:47Z

I wonder if we should rethink the term precommit

I often saw the term sanityCheck

You are correct that precommit is a sanity check. The terminology comes from the Lucene build (we have quite a few Lucene developers who work on both projects). For that reason, I don't think we should change the name.

breskeby · 2020-05-19T14:16:52Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/PrecommitPlugin.java

+        });
+        precommit.configure(t -> t.dependsOn(task));
+
+        project.getPluginManager().withPlugin("java", p -> {


In my opinion we should extract that into a JavaPreCommit plugin. this also ensure this code is not executed more than once per project.

It's not doing anything extra, there is no java precommit task. This is only ensuring when we have java projects with precommit, we wire up the precommit task in the desired order. I can move it so it is only applied once.

breskeby · 2020-05-19T14:18:53Z

I wonder if we should rethink the term precommit

I often saw the term sanityCheck

You are correct that precommit is a sanity check. The terminology comes from the Lucene build (we have quite a few Lucene developers who work on both projects). For that reason, I don't think we should change the name.

got it.

breskeby

lgtm 👍

Each precommit task is currently registered inside the shared PrecommitTasks class. Having a single class with all precommit tasks makes individualizing which precommit tasks are needed based on type of project difficult, where we often just disable somet tasks eg for all qa projects. This commit creates plugins for each precommit task, and moves the configuration of the task into each plugin. PrecommitTasks remains for now, and just delegates to each plugin, but will be removed in a future change.

rjernst added v8.0.0 v7.7.1 v7.8.1 v7.9.0 :Delivery/Build Build or test infrastructure >refactoring labels May 18, 2020

rjernst requested a review from mark-vieira May 18, 2020 22:05

elasticmachine added the Team:Core/Infra Meta label for core/infra team label May 18, 2020

rjernst mentioned this pull request May 18, 2020

Create plugin for yamlTest task #56841

Merged

iter

1b4afbf

breskeby reviewed May 19, 2020

View reviewed changes

breskeby requested changes May 19, 2020

View reviewed changes

rjernst added 4 commits May 19, 2020 07:47

use actual Closure

097877d

Merge branch 'master' into buildsplit7

778d71a

only create precommit once

b5dff57

split java specific to not be repeated

ff9c462

breskeby approved these changes May 19, 2020

View reviewed changes

rjernst added 4 commits May 19, 2020 09:08

fix eql licenses

cd68d73

package protected internal plugin

89c170d

remove debug

aa5fb8a

actually add license files

68c93e9

mark-vieira assigned rjernst May 19, 2020

rjernst added 3 commits May 19, 2020 09:36

overwrite existing checkstyle config

8cc4c28

Merge branch 'master' into buildsplit7

e825db5

spotless

32ef971

rjernst mentioned this pull request May 20, 2020

Gradle deprecation warning of compileOnly not being directly resolved is removed #56998

Closed

rjernst added 4 commits May 20, 2020 09:17

add back substractions of compileOnly

661d527

revert eql change

a59ee84

remove licenses dir

f7919c8

Merge branch 'master' into buildsplit7

6d11107

rjernst merged commit c98d548 into elastic:master May 20, 2020

rjernst deleted the buildsplit7 branch May 20, 2020 20:12

mark-vieira added Team:Delivery Meta label for Delivery team and removed Team:Core/Infra Meta label for core/infra team labels Nov 11, 2020

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

Add individual precommit task plugins #56926

Add individual precommit task plugins #56926

Uh oh!

Conversation

rjernst commented May 18, 2020

Uh oh!

elasticmachine commented May 18, 2020

Uh oh!

breskeby left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breskeby May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breskeby May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breskeby commented May 19, 2020

Uh oh!

breskeby left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

breskeby left a comment •

edited

Loading

breskeby May 19, 2020 •

edited

Loading

breskeby May 19, 2020 •

edited

Loading

rjernst commented May 19, 2020 •

edited

Loading