terrymanu commented on PR #39658: URL: https://github.com/apache/shardingsphere/pull/39658#issuecomment-5470274039
### Result **Review Result: Mergeable** The comment classification has been corrected, and another complete review has been performed. No required changes remain. ### Evidence 1. Why `maven-checkstyle-plugin` is needed This does not introduce another Checkstyle plugin or execution. It moves the existing plugin version and runtime dependency into the shared `build/pluginManagement` section for two consumers: - The Checkstyle report under `reporting`. - The Checkstyle validation under the `check` profile. The PR adds `LineEnding`, which has only been available since Checkstyle 13.3.0. Maven Checkstyle Plugin 3.6.0 uses Checkstyle 9.3 by default. Without overriding it with the repository’s Checkstyle 14.0.0: - `./mvnw checkstyle:check -Pcheck -T1C` succeeds because the `check` profile already overrides the Checkstyle runtime. - `./mvnw -pl infra/common site -DskipTests` fails because `reporting` does not activate the `check` profile. Maven cannot initialize the `LineEnding` module. Maven’s `ReportPlugin` model does not support a `<dependencies>` element. Its version is resolved from `build/plugins` or `build/pluginManagement`, so the plugin runtime dependency must be declared on the `build` side. The official Maven Checkstyle Plugin example also uses `build/pluginManagement`: - https://maven.apache.org/plugins/maven-checkstyle-plugin/examples/upgrading-checkstyle.html - https://maven.apache.org/ref/current/maven-model/maven.html#class_reportplugin - https://checkstyle.org/checks/misc/lineending.html#LineEnding The effective POM now confirms: - The default build has one Maven Checkstyle Plugin 3.6.0 declaration in `pluginManagement`, using Checkstyle 14.0.0. - The default `build/plugins` section does not contain the Checkstyle plugin, so it does not execute automatically. - The `reporting` section still contains one Checkstyle report plugin. - When the `check` profile is active, its execution correctly inherits Checkstyle 14.0.0. 2. The problem with `<!-- Check plugins -->` You were right. I previously confused what the plugin does with which responsibility the declaration serves at that location. The global declaration exists specifically to support `reporting`, while the `check` profile still owns the validation lifecycle. Therefore: - The newly added `<!-- Check plugins -->` group has been removed. - The shared declaration is now under the existing `<!-- Report plugins -->` group at `pom.xml:926`. - The existing `<!-- Check plugins -->` group inside the `check` profile continues to own the validation configuration and execution around `pom.xml:1365`. Final minimal change: - `pom.xml` → share Checkstyle 3.6.0/14.0.0 between reporting and the `check` profile → prevent the new rules from breaking site report generation → required. Verification results: - `./mvnw -N spotless:apply -Pcheck -T1C`: exit 0; the file was already formatted. - Default and `-Pcheck` effective POM generation: exit 0; inheritance is correct. - `./mvnw -pl infra/common site -DskipTests`: exit 0; the Checkstyle report was generated successfully. - `./mvnw checkstyle:check -Pcheck -T1C`: exit 0; 0 violations. - `./mvnw spotless:check -Pcheck -T1C`: exit 0. - `./mvnw -pl infra/common validate -DskipTests`: exit 0; Checkstyle was not executed. ### Coverage - Public PR head: `cdc990be37f6b1426e9fe8fcf42b039a8c1adcdc`. - Public PR file: `src/resources/checkstyle.xml`. - Authorized local fix: `pom.xml`. - Reviewed rule semantics, Maven/Checkstyle compatibility, reporting and `check` profile paths, default lifecycle behavior, side effects, dependencies, distribution impact, performance, and formatting. - No new project runtime dependency was added; only the ownership of an existing plugin runtime configuration changed. - No unresolved review findings remain. - This is a code-correctness result only; CI was not reviewed. - No Git staging, commit, push, or other Git write was performed. - The two pre-existing untracked `__pycache__` directories were not touched. Suggested commit message: `Align Checkstyle runtime for Maven reports` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
