From 083aff19a5c1789dfdf9170cda6a60e7dcf9f9ab Mon Sep 17 00:00:00 2001 From: Rene Groeschke Date: Fri, 15 Jul 2022 19:31:50 +0200 Subject: [PATCH] Remove duplicate definition of checkstyle version in use (#88339) We only rely on the checkstyle version in the buildLibs.toml gradle version catalogue with this change. Also added some hints for gradle best practices. This is an aftermath of #88283 --- BUILDING.md | 14 +++++- .../fixtures/LocalRepositoryFixture.groovy | 2 +- .../CheckstylePrecommitPluginFuncTest.groovy | 46 +++++++++++++++++++ .../precommit/CheckstylePrecommitPlugin.java | 27 +++++++---- build-tools-internal/version.properties | 2 - .../fixtures/AbstractGradleFuncTest.groovy | 17 +++++++ settings.gradle | 8 ++++ 7 files changed, 101 insertions(+), 15 deletions(-) create mode 100644 build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/precommit/CheckstylePrecommitPluginFuncTest.groovy diff --git a/BUILDING.md b/BUILDING.md index 01f24613968b1..ef3f78911b10f 100644 --- a/BUILDING.md +++ b/BUILDING.md @@ -48,6 +48,16 @@ in our build logic to resolve this. **The Elasticsearch build will fail if any deprecated Gradle API is used.** +### Follow Gradle best practices + +Tony Robalik has compiled a good list of rules that aligns with ours when it comes to writing and maintaining elasticsearch +gradle build logic at http://autonomousapps.com/blog/rules-for-gradle-plugin-authors.html. +Our current build does not yet tick off all those rules everywhere but the ultimate goal is to follow these principles. +The reasons for following those rules besides better readability or maintenance are also the goal to support newer gradle +features that we will benefit from in terms of performance and reliability. +E.g. [configuration-cache support](https://github.com/elastic/elasticsearch/issues/57918), [Project Isolation]([https://gradle.github.io/configuration-cache/#project_isolation) or +[predictive test selection](https://gradle.com/gradle-enterprise-solutions/predictive-test-selection/) + ### Make a change in the build There are a few guidelines to follow that should make your life easier to make changes to the elasticsearch build. @@ -190,7 +200,7 @@ by JitPack in the background before we can resolve the adhoc built dependency. **NOTE** -You should only use that approach locally or on a developer branch for for production dependencies as we do +You should only use that approach locally or on a developer branch for production dependencies as we do not want to ship unreleased libraries into our releases. --- @@ -229,5 +239,5 @@ As Gradle prefers to use modules whose descriptor has been created from real met flat directory repositories cannot be used to override artifacts with real meta-data from other repositories declared in the build. For example, if Gradle finds only `jmxri-1.2.1.jar` in a flat directory repository, but `jmxri-1.2.1.pom` in another repository that supports meta-data, it will use the second repository to provide the module. -Therefore it is recommended to declare a version that is not resolveable from public repositories we use (e.g. maven central) +Therefore, it is recommended to declare a version that is not resolvable from public repositories we use (e.g. maven central) --- diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/fixtures/LocalRepositoryFixture.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/fixtures/LocalRepositoryFixture.groovy index b96f076e2ab83..823c9e4c878bf 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/fixtures/LocalRepositoryFixture.groovy +++ b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/fixtures/LocalRepositoryFixture.groovy @@ -13,7 +13,7 @@ import net.bytebuddy.dynamic.DynamicType import org.junit.rules.ExternalResource import org.junit.rules.TemporaryFolder -class LocalRepositoryFixture extends ExternalResource{ +class LocalRepositoryFixture extends ExternalResource { private TemporaryFolder temporaryFolder diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/precommit/CheckstylePrecommitPluginFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/precommit/CheckstylePrecommitPluginFuncTest.groovy new file mode 100644 index 0000000000000..4d999f1b0ded4 --- /dev/null +++ b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/precommit/CheckstylePrecommitPluginFuncTest.groovy @@ -0,0 +1,46 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.gradle.internal.precommit + +import org.elasticsearch.gradle.fixtures.AbstractGradleInternalPluginFuncTest +import org.elasticsearch.gradle.fixtures.LocalRepositoryFixture +import org.elasticsearch.gradle.internal.conventions.precommit.PrecommitPlugin +import org.gradle.testkit.runner.TaskOutcome +import org.junit.ClassRule +import spock.lang.Shared + +/** + * This just tests basic plugin configuration, wiring and compatibility and not checkstyle itself + * */ +class CheckstylePrecommitPluginFuncTest extends AbstractGradleInternalPluginFuncTest { + Class pluginClassUnderTest = CheckstylePrecommitPlugin.class + + @Shared + @ClassRule + public LocalRepositoryFixture repository = new LocalRepositoryFixture() + + def setup() { + withVersionCatalogue() + buildFile << """ + apply plugin:'java' + """ + repository.configureBuild(buildFile) + repository.generateJar("org.elasticsearch", "build-conventions", "unspecified", 'org.acme.CheckstyleStuff') + repository.generateJar("com.puppycrawl.tools", "checkstyle", "10.3", 'org.puppycral.CheckstyleStuff') + + } + + def "can configure checkstyle tasks"() { + when: + def result = gradleRunner("precommit").build() + then: + result.task(":checkstyleMain").outcome == TaskOutcome.NO_SOURCE + result.task(":checkstyleTest").outcome == TaskOutcome.NO_SOURCE + } +} diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/precommit/CheckstylePrecommitPlugin.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/precommit/CheckstylePrecommitPlugin.java index fa8557361b959..ffc43c56655b5 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/precommit/CheckstylePrecommitPlugin.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/precommit/CheckstylePrecommitPlugin.java @@ -8,12 +8,13 @@ package org.elasticsearch.gradle.internal.precommit; -import org.elasticsearch.gradle.VersionProperties; import org.elasticsearch.gradle.internal.conventions.precommit.PrecommitPlugin; import org.gradle.api.Action; import org.gradle.api.Project; import org.gradle.api.Task; +import org.gradle.api.artifacts.VersionCatalogsExtension; import org.gradle.api.artifacts.dsl.DependencyHandler; +import org.gradle.api.plugins.JavaBasePlugin; import org.gradle.api.plugins.quality.Checkstyle; import org.gradle.api.plugins.quality.CheckstyleExtension; import org.gradle.api.provider.Provider; @@ -84,11 +85,13 @@ public void execute(Task task) { checkstyle.getConfigDirectory().set(checkstyleDir); DependencyHandler dependencies = project.getDependencies(); - String checkstyleVersion = VersionProperties.getVersions().get("checkstyle"); Provider conventionsDependencyProvider = project.provider( () -> "org.elasticsearch:build-conventions:" + project.getVersion() ); - dependencies.add("checkstyle", "com.puppycrawl.tools:checkstyle:" + checkstyleVersion); + dependencies.addProvider("checkstyle", project.provider(() -> { + var versionCatalog = project.getExtensions().getByType(VersionCatalogsExtension.class).named("buildLibs"); + return versionCatalog.findLibrary("checkstyle").get().get(); + })); dependencies.addProvider("checkstyle", conventionsDependencyProvider, dep -> dep.setTransitive(false)); project.getTasks().withType(Checkstyle.class).configureEach(t -> { @@ -98,13 +101,17 @@ public void execute(Task task) { // Configure checkstyle tasks with an empty classpath to improve build avoidance. // It's optional since our rules only rely on source files anyway. - project.getExtensions() - .getByType(SourceSetContainer.class) - .all( - sourceSet -> project.getTasks() - .withType(Checkstyle.class) - .named(sourceSet.getTaskName("checkstyle", null)) - .configure(t -> t.setClasspath(project.getObjects().fileCollection())) + project.getPlugins() + .withType( + JavaBasePlugin.class, + javaBasePlugin -> project.getExtensions() + .getByType(SourceSetContainer.class) + .all( + sourceSet -> project.getTasks() + .withType(Checkstyle.class) + .named(sourceSet.getTaskName("checkstyle", null)) + .configure(t -> t.setClasspath(project.getObjects().fileCollection())) + ) ); return checkstyleTask; diff --git a/build-tools-internal/version.properties b/build-tools-internal/version.properties index 51baa23a7b32f..2a606afa40d7f 100644 --- a/build-tools-internal/version.properties +++ b/build-tools-internal/version.properties @@ -4,8 +4,6 @@ lucene = 8.11.1 bundled_jdk_vendor = openjdk bundled_jdk = 18.0.1.1+2@65ae32619e2f40f3a9af3af1851d6e19 -checkstyle = 10.3 - # optional dependencies spatial4j = 0.7 jts = 1.15.0 diff --git a/build-tools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy b/build-tools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy index 5df072d6f7eba..e1f13fa2336b2 100644 --- a/build-tools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy +++ b/build-tools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy @@ -174,6 +174,23 @@ abstract class AbstractGradleFuncTest extends Specification { dir } + void withVersionCatalogue() { + file('build.versions.toml') << '''\ +[libraries] +checkstyle = "com.puppycrawl.tools:checkstyle:10.3" +''' + settingsFile << ''' + dependencyResolutionManagement { + versionCatalogs { + buildLibs { + from(files("build.versions.toml")) + } + } + } + ''' + + } + static class ProjectConfigurer { private File projectDir diff --git a/settings.gradle b/settings.gradle index e0265c5de5d81..df75b53e8940b 100644 --- a/settings.gradle +++ b/settings.gradle @@ -15,6 +15,14 @@ includeBuild "build-tools-internal" rootProject.name = "elasticsearch" +dependencyResolutionManagement { + versionCatalogs { + buildLibs { + from(files("gradle/build.versions.toml")) + } + } +} + List projects = [ 'rest-api-spec', 'docs',