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

@@ -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)
 ---

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
 

build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/BuildPluginFuncTest.groovy

@@ -13,7 +13,6 @@ import org.elasticsearch.gradle.fixtures.AbstractGradleFuncTest
 import org.elasticsearch.gradle.fixtures.LocalRepositoryFixture
 import org.gradle.testkit.runner.TaskOutcome
 import org.junit.ClassRule
-import org.junit.rules.TemporaryFolder
 import spock.lang.Shared
 
 import java.nio.charset.StandardCharsets
@@ -128,6 +127,7 @@ class BuildPluginFuncTest extends AbstractGradleFuncTest {
 
     def "applies checks"() {
         given:
+        withVersionCatalogue()
         repository.generateJar("org.elasticsearch", "build-conventions", "unspecified", 'org.acme.CheckstyleStuff')
         repository.configureBuild(buildFile)
         setupJarHellJar(dir('local-repo/org/elasticsearch/elasticsearch-core/current/'))
@@ -142,7 +142,6 @@ class BuildPluginFuncTest extends AbstractGradleFuncTest {
               api "junit:junit:4.12"
               // missing classes in thirdparty audit
               api 'org.hamcrest:hamcrest-core:1.3'
-
             }
             licenseFile.set(file("LICENSE"))
             noticeFile.set(file("NOTICE"))

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<? extends PrecommitPlugin> 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
+    }
+}

build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/test/rest/YamlRestCompatTestPluginFuncTest.groovy

@@ -141,7 +141,7 @@ class YamlRestCompatTestPluginFuncTest extends AbstractRestResourcesFuncTest {
 
     def "yamlRestTestVxCompatTest is wired into check and checkRestCompat"() {
         given:
-
+        withVersionCatalogue()
         subProject(":distribution:bwc:maintenance") << """
         configurations { checkout }
         artifacts {

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 class CheckstylePrecommitPlugin extends PrecommitPlugin {
         checkstyle.getConfigDirectory().set(checkstyleDir);
 
         DependencyHandler dependencies = project.getDependencies();
-        String checkstyleVersion = VersionProperties.getVersions().get("checkstyle");
         Provider<String> 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 class CheckstylePrecommitPlugin extends PrecommitPlugin {
 
         // 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;

build-tools-internal/version.properties

@@ -4,8 +4,6 @@ lucene            = 9.3.0-snapshot-2d05f5c623e
 bundled_jdk_vendor = openjdk
 bundled_jdk = 18.0.1.1+2@65ae32619e2f40f3a9af3af1851d6e19
 
-checkstyle = 10.3
-
 # optional dependencies
 spatial4j         = 0.7
 jts               = 1.15.0

build-tools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy

@@ -188,6 +188,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
 

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',