Create gradle plugin for ES stable plugins by pgomulka · Pull Request #90355 · elastic/elasticsearch

pgomulka · 2022-09-26T13:59:41Z

New ES stable plugins when built should have a stable-plugin-descriptor.properties file instead of plugin-descriptor.properties.
New plugins also do not use classname property in the plugin descriptor
new plugin will also scan classes and libraries for @NamedComponents and will create named_components.json file. That file contains a map of Extensible interface (like TokenizerFactory) to a map of "component name" to "className"

This commit extracts common logic from PluginBuildPlugin into BasePluginBuildPLugin so that it can also be used by StableBuildPlugin
the differences are:
classaname - used in old plugin, but not in the new one (stable)
the plugin descriptor file name - the new one has stable-plugin-descriptor.properties
dependencies - the new plugin does not need elasticsearch as a dependency.
We might want to consider if we want to add test framework dependency in the future.

relates #88980

New ES stable plugins when built should have a stable-plugin-descriptor.properties file instead of plugin-descriptor.properties. New plugins also do not use classname property in the plugin descriptor relates elastic#88980

pgomulka · 2022-09-26T14:00:32Z

a next step will be to generate named_components.json file where information about annotated plugin components which will be stored

elasticsearchmachine · 2022-09-26T14:07:36Z

Pinging @elastic/es-delivery (Team:Delivery)

breskeby

Hey there I left some comments we should address IMO. Also we should have that full StablePluginBuildPlugin functionality in this PR otherwise adding just the skeleton doesnt add any value IMO and each PR should have a purpose. Alternatively we just contain the refactoring here without the new Plugin

breskeby · 2022-09-28T08:18:18Z

build-tools/src/main/resources/plugin-descriptor.properties

 #
 # 'classname': the name of the class to load, fully-qualified. Only applies to
 # "isolated" plugins
+<% if (classname) { %>


We should put the if block around the description too IMO

breskeby · 2022-09-28T08:19:16Z

build-tools/src/main/java/org/elasticsearch/gradle/plugin/GeneratePluginPropertiesTask.java

    public GeneratePluginPropertiesTask(ProjectLayout projectLayout) {
-        setDescription("Generate " + PROPERTIES_FILENAME);
-        getOutputFile().convention(projectLayout.getBuildDirectory().file("generated-descriptor/" + PROPERTIES_FILENAME));
+        // TODO I cannot tell if this is stable or not..


To be honest I would just change it to a generic description like. "Generates Elasticsearch Plugin descriptor files"

breskeby · 2022-09-28T08:24:35Z

build-tools/build.gradle

            implementationClass = 'org.elasticsearch.gradle.plugin.PluginBuildPlugin'
        }
+        stableEsPlugin {
+          id = 'elasticsearch.stable_esplugin'


our given convention is to use - instead of _ so it should be elasticsearch.stable-esplugin

breskeby · 2022-09-28T08:25:35Z

build-tools/src/main/java/org/elasticsearch/gradle/plugin/StableBuildPlugin.java

+import org.gradle.api.file.RegularFile;
+import org.gradle.api.provider.Provider;
+
+public class StableBuildPlugin implements Plugin<Project> {


It should be StableBuildPluginPlugin really as we have also a Build Plugin and that would be confusing

breskeby · 2022-09-28T08:27:17Z

...-tools/src/integTest/groovy/org/elasticsearch/gradle/plugin/PluginBuildPluginFuncTest.groovy

    }

    Map<String, String> getPluginProperties() {
-        Path propsFile = file("build/generated-descriptor/plugin-descriptor.properties").toPath();


That seems not required anymore as we split the tests of Stable Plugin Build Plugin and and Build Plugin.

breskeby · 2022-09-28T08:28:39Z

...-tools/src/integTest/groovy/org/elasticsearch/gradle/plugin/PluginBuildPluginFuncTest.groovy

@@ -75,7 +75,7 @@ class PluginBuildPluginFuncTest extends AbstractGradleFuncTest {
            }


In general the test that test functionality that moved into the BasePlugin should be moved into a BasePluginBuildPluginFunc test

but I cannot really test basePluginBuildPlugin without applying 'elasticsearch.esplugin' or elasticsearch.stable-esplugin`.

you can apply the BasePluginBuildPlugin just by using the classname

plugins.apply(org.elasticsearch.gradle.plugin. BasePluginBuildPlugin)

We do something similar to test internal plugins. see AbstractGradleInternalPluginFuncTest

pgomulka · 2022-10-04T07:17:35Z

...tools/src/testFixtures/java/org/elasticsearch/gradle/internal/test/InMemoryJavaCompiler.java

+ *     Map<String, byte[]> result = compile(sources);
+ * }</pre>
+ */
+public class InMemoryJavaCompiler {


can we use anything from groovy to compile? maybe localclasses? bytebuddy?

Not with the same convenience I guess as you can load groovy classes from a string, but you require writing it back to a jar as far as I understand?

yes, I would have to get their byte code to write them to jar. Let's keep the InMemoryJavaCompiler here for now?

pgomulka · 2022-10-04T07:18:35Z

build-tools/src/testFixtures/java/org/elasticsearch/gradle/internal/test/JarUtils.java

+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.stream.Collectors.toUnmodifiableMap;
+
+public final class JarUtils {


we can possibly avoid using this class if we use real test classes or localclasses

pgomulka · 2022-10-04T11:25:35Z

build-tools/src/main/java/org/elasticsearch/gradle/plugin/StablePluginBuildPlugin.java

+            t.setPluginClasses(mainSourceSet.getOutput().getClassesDirs());
+            // it has to be compile classpath ?? otherwise libraries which are provided in runtime by es server won't be visible
+            // and we want to scan them too
+            t.setClasspath(project.getConfigurations().getByName(JavaPlugin.COMPILE_CLASSPATH_CONFIGURATION_NAME));


it has to be compile classpath ?? otherwise libraries which are provided in runtime by es server won't be visible and we want to scan them too

Usually a classpath property is the classpath required to run the GenerateNamedComponentsAction. I wonder if its needed at all. We probbaly can merge this with pluginClasses can't we?

pgomulka · 2022-10-04T11:59:59Z

...ools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy

        return jarFile;
    }

+    //todo maybe we could refer to ../libs/plugin-api compile it and create a jar?


any views on this? is this possible?

I don't see how this is possible. We run into a chicken egg problem here

pgomulka · 2022-10-05T07:22:45Z

@elasticmachine update branch

breskeby · 2022-10-05T08:38:27Z

build-tools/src/main/java/org/elasticsearch/gradle/plugin/StablePluginBuildPlugin.java

+            t.setPluginClasses(mainSourceSet.getOutput().getClassesDirs());
+            // it has to be compile classpath ?? otherwise libraries which are provided in runtime by es server won't be visible
+            // and we want to scan them too
+            t.setClasspath(project.getConfigurations().getByName(JavaPlugin.COMPILE_CLASSPATH_CONFIGURATION_NAME));


Usually a classpath property is the classpath required to run the GenerateNamedComponentsAction. I wonder if its needed at all. We probbaly can merge this with pluginClasses can't we?

breskeby · 2022-10-05T08:42:50Z

...ls/src/test/groovy/org/elasticsearch/gradle/plugin/scanner/test_classes/ExtensibleClass.java

@@ -0,0 +1,14 @@
+/*


We really do not want java classes in src/(main test)/groovy as joint compilation is ridicolous slow. You can either just rename those classes to groovy or can you move them into testFixtures/java? Ultimately they are test fixtures aren't they?

breskeby · 2022-10-05T08:43:19Z

build-tools/src/testFixtures/groovy/org/elasticsearch/extensible/ExtensibleClass.java

@@ -0,0 +1,14 @@
+/*


those should live in testFixtures/java and not in testFixtures/groovy

breskeby · 2022-10-05T08:49:40Z

...-tools/src/integTest/groovy/org/elasticsearch/gradle/plugin/PluginBuildPluginFuncTest.groovy

@@ -75,7 +75,7 @@ class PluginBuildPluginFuncTest extends AbstractGradleFuncTest {
            }


you can apply the BasePluginBuildPlugin just by using the classname

plugins.apply(org.elasticsearch.gradle.plugin. BasePluginBuildPlugin)

We do something similar to test internal plugins. see AbstractGradleInternalPluginFuncTest

…le_plugin_clean

pgomulka · 2022-10-05T15:09:00Z

@elasticmachine run elasticsearch-ci/bwc

pgomulka · 2022-10-05T15:36:56Z

@elasticmachine run elasticsearch-ci/bwc

breskeby

Some minor remarks.

breskeby · 2022-10-05T20:38:20Z

...tools/src/testFixtures/java/org/elasticsearch/gradle/internal/test/InMemoryJavaCompiler.java

+ *     Map<String, byte[]> result = compile(sources);
+ * }</pre>
+ */
+public class InMemoryJavaCompiler {


breskeby · 2022-10-05T20:41:11Z

build-tools/src/test/groovy/org/elasticsearch/gradle/plugin/scanner/ClassReadersSpec.groovy

+            """
+        )
+        )
+        );


formatting seems off here

it is intellijj's formatting. I will reformat manually. Side note - spotlessApply is not touching these files

breskeby · 2022-10-05T20:44:10Z

.../src/integTest/groovy/org/elasticsearch/gradle/plugin/StableBuildPluginPluginFuncTest.groovy

+        then:
+        result.task(":assemble").outcome == TaskOutcome.SUCCESS
+
+        println Files.readString(namedComponents)


guess this debug output can be removed

.../src/integTest/groovy/org/elasticsearch/gradle/plugin/StableBuildPluginPluginFuncTest.groovy

breskeby · 2022-10-05T20:49:49Z

build-tools/src/main/java/org/elasticsearch/gradle/plugin/GenerateNamedComponentsTask.java

+public abstract class GenerateNamedComponentsTask extends DefaultTask {
+    private static final Logger LOGGER = Logging.getLogger(GenerateNamedComponentsTask.class);
+    private static final String NAMED_COMPONENTS_FILE = "named_components.json";
+    private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();


That OBJECT_MAPPER should live in the nested worker class IMO as its only used there

breskeby · 2022-10-05T20:51:22Z

.../src/integTest/groovy/org/elasticsearch/gradle/plugin/StableBuildPluginPluginFuncTest.groovy

+
+    def "can scan and create named components file"() {
+        given:
+        println testProjectDir.root


debug output can be removed

...ools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy

breskeby

lgtm. Thank you!

breskeby · 2022-10-06T11:30:54Z

...ools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy


 package org.elasticsearch.gradle.fixtures

+


can we revert this little import ordering change before merging?

ChrisHegarty

LGTM.

ChrisHegarty · 2022-10-06T14:12:18Z

build-tools/build.gradle

            implementationClass = 'org.elasticsearch.gradle.plugin.PluginBuildPlugin'
        }
+        stableEsPlugin {
+          id = 'elasticsearch.stable-esplugin'


Let's go with this name for now, but reserve the right to change it.

pgomulka · 2022-10-06T15:04:09Z

@elasticmachine run elasticsearch-ci/part-1

Create gradle plugin for ES stable plugins

f135b63

New ES stable plugins when built should have a stable-plugin-descriptor.properties file instead of plugin-descriptor.properties. New plugins also do not use classname property in the plugin descriptor relates elastic#88980

elasticsearchmachine added the v8.6.0 label Sep 26, 2022

pgomulka marked this pull request as ready for review September 26, 2022 14:06

pgomulka added >enhancement :Delivery/Build Build or test infrastructure labels Sep 26, 2022

elasticsearchmachine added the Team:Delivery Meta label for Delivery team label Sep 26, 2022

pgomulka added 4 commits September 26, 2022 17:49

use only one template

673350d

Merge remote-tracking branch 'upstream/main' into stable_plugin_clean

1a82005

partial tests

9f6c7f6

remove additional extension

8d2212c

pgomulka requested a review from breskeby September 27, 2022 15:02

tests as spec

cb9f184

breskeby requested changes Sep 28, 2022

View reviewed changes

pgomulka added 8 commits September 28, 2022 16:11

code review follow up

7717d3b

Merge branch 'scanner_gradle_of_clean' into stable_plugin_clean

c44feaf

named components

0cea1a2

scanning

91e55f3

more tests

ad116a7

specs and groovy tasks

bab1859

gradle test

c43a91f

Merge remote-tracking branch 'upstream/main' into stable_plugin_clean

a0e73e8

pgomulka requested a review from breskeby October 3, 2022 14:30

pgomulka commented Oct 4, 2022

View reviewed changes

pgomulka mentioned this pull request Oct 4, 2022

Stable Analysis Plugin API #88980

Open

25 tasks

unused imports

e23ae57

pgomulka commented Oct 4, 2022

View reviewed changes

cleanup

7c4e61c

Merge branch 'main' into stable_plugin_clean

bb41a7e

breskeby requested changes Oct 5, 2022

View reviewed changes

pgomulka added 6 commits October 5, 2022 12:20

cleanup test classes

2709ff5

Merge remote-tracking branch 'upstream/main' into stable_plugin_clean

92f0d28

remove stream supplier and two class properties

69eceda

Merge remote-tracking branch 'pgomulka/stable_plugin_clean' into stab…

2f87873

…le_plugin_clean

test fix after move of testclasses

410b21e

fix tests

5c1065a

breskeby requested changes Oct 5, 2022

View reviewed changes

pgomulka added 4 commits October 6, 2022 09:57

code review follow up

d5e4d6a

spotless

e0a43c2

Merge remote-tracking branch 'upstream/main' into stable_plugin_clean

3acd65d

remove unused methods

681aa02

breskeby approved these changes Oct 6, 2022

View reviewed changes

revert import ordering

c16ae04

ChrisHegarty approved these changes Oct 6, 2022

View reviewed changes

ChrisHegarty reviewed Oct 6, 2022

View reviewed changes

pgomulka merged commit d1f2b5d into elastic:main Oct 10, 2022

		@@ -75,7 +75,7 @@ class PluginBuildPluginFuncTest extends AbstractGradleFuncTest {
		}

Conversation

pgomulka commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgomulka commented Sep 26, 2022

Uh oh!

elasticsearchmachine commented Sep 26, 2022

Uh oh!

breskeby left a comment

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!

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!

pgomulka Oct 5, 2022 • 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!

pgomulka commented Oct 5, 2022

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!

pgomulka commented Oct 5, 2022

Uh oh!

pgomulka commented Oct 5, 2022

Uh oh!

breskeby left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

breskeby left a comment

Choose a reason for hiding this comment

pgomulka commented Sep 26, 2022 •

edited

Loading

pgomulka Oct 5, 2022 •

edited

Loading

ChrisHegarty Oct 6, 2022 •

edited

Loading