jamesfredley commented on PR #16073: URL: https://github.com/apache/grails-core/pull/16073#issuecomment-5141290420
## Correction: option 2 as implemented is NOT ready - it silently downgrades versions My previous comment reported option 2 passing every check. A review pass then found a defect that none of those checks could detect, so treat that comment as superseded on the readiness claim. The approach is still sound; this implementation of it is not. **The defect.** The 16 explicitly managed dependencies are an incomplete set. Where an entry is missing, `io.spring.dependency-management` falls back to the automatically imported `spring-boot-dependencies` BOM, which supplies an **older** version - and the build succeeds, so nothing flags it: | Coordinate | Repo-managed | Actually resolved | |---|---|---| | `groovy-console`, `groovy-json`, `groovy-sql`, `groovy-templates`, `groovy-xml` | 5.0.7 | **5.0.6** | | `commons-codec` | 1.22.0 | **1.21.0** | | `log4j-api`, `log4j-to-slf4j` | 2.25.5 | **2.25.4** | Only Groovy *core* resolves to 5.0.7. Everything else in those families comes from Spring Boot. This is the same class of bug the PR set out to fix - the example resolving versions the project did not choose - just arriving by a different route. A silent log4j downgrade is not something to ship past a green build. **Why no check caught it.** Cold build, offline build, clean build, integration test, BOM integrity - all passed. Every one of them asks "does it resolve and run?", and it does. None asks "does it resolve to the versions this repository manages?" That question needs a resolution assertion, not a successful compile. **What it needs before it can land:** 1. Override `groovy.version`, `commons-codec.version` and `log4j2.version` from `bomDependencyVersions` so Spring Boot's values cannot win, or explicitly manage every consumed coordinate in those families. 2. Add a verification task or spec asserting that the resolved `compileClasspath` and `integrationTestRuntimeClasspath` coordinates equal their `dependencies.gradle` values. Without this the list will drift again the next time a dependency is added or a version bumped - the failure is silent by construction. 3. Fix the `settings.gradle` comment, which currently implies the example sources the external versions it needs from `dependencies.gradle`. In reality it sources its Grails-specific overrides there and Spring DM supplies the rest from the Spring Boot BOM. Confirmed clean in the same pass: `build.gradle` and `GrailsRepoSettingsPlugin.groovy` revert to `origin/8.0.x` exactly, with no local-BOM residue; all 16 listed versions reference the source maps rather than duplicating literals; the `bom = null` + Spring DM coverage is preserved; and `bomSnapshotNotPublished` becomes harmless dead configuration on the forward branches. **Also noted, non-blocking:** dropping the `mavenBom` import means the example no longer exercises whether the generated Grails BOM is consumable through Spring DM by an external application. That is the deliberate trade-off, since that path never validated the commit under test - it always resolved the last published BOM. Worth a conscious decision by reviewers rather than an accident. The obvious counter-proposal, resolving the BOM from an in-commit repository, is the approach already ruled out earlier in this PR. Nothing has been pushed. The branch remains at `9bc5aefbf1`. -- 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]
