jamesfredley commented on PR #15467:
URL: https://github.com/apache/grails-core/pull/15467#issuecomment-4513138460
## Final review-feedback pass (commit `d67ef28`) - ready for merge
Three remaining open threads have been addressed in a single commit. The
branch is now rebased on the latest `8.0.x` (`e98b6a8`) and verified green
locally.
### Changes in this push
**1. `bom-property-overrides` consolidated into `grails-gradle-plugins`**
(jdaugherty: _"this should be under the Gradle package to match our other
plugins"_)
The standalone `grails-gradle/bom-property-overrides/` subproject is merged
into `grails-gradle/plugins/`, joining the 11 other plugins registered there:
- 3 main sources + 3 test sources + 4 test resources moved into
`grails-gradle/plugins/`
- Duplicate `GradleSpecification.groovy` dropped in favour of the existing
copy in `org.grails.gradle.plugin.core`
- `bomPropertyOverrides` registered as the 12th entry in the `gradlePlugin
{}` block in [`plugins/build.gradle`](../../grails-gradle/plugins/build.gradle)
- Self-referential `implementation
project(':grails-gradle-bom-property-overrides')` removed
- `include` + `projectDir` mapping removed from
[`grails-gradle/settings.gradle`](../../grails-gradle/settings.gradle)
- `grails-gradle-bom-property-overrides` removed from `publishedProjects` in
[`grails-gradle/gradle/publish-root-config.gradle`](../../grails-gradle/gradle/publish-root-config.gradle)
- Empty `grails-gradle/bom-property-overrides/` directory deleted
Plugin id (`org.apache.grails.gradle.bom-property-overrides`), extension
name (`bomPropertyOverrides`) and implementation class
(`org.grails.gradle.plugin.bom.BomPropertyOverridesPlugin`) are unchanged -
end-user usage is identical.
**2. `def` for inferrable locals in all new Groovy code** (matrei: _"this
applies generally to all added Groovy code"_)
`BomManagedVersions`, `BomPropertyOverridesPlugin`, `BomManagedVersionsSpec`
and `BomPropertyOverridesPluginSpec` now use `def` for local variables where
the type can be inferred. Public API method signatures and field declarations
keep explicit types - the public contract is unchanged. The now-unused
`org.w3c.dom.NodeList` import that the refactor exposed was dropped.
**3. `DuplicatesStrategy.INCLUDE` workaround removed** (jdaugherty: _"What
is causing the duplicate? We typically fix these so it's not needed"_)
Confirmed by removing the `tasks.withType(Copy) { duplicatesStrategy =
INCLUDE }` block from
[`plugins/build.gradle`](../../grails-gradle/plugins/build.gradle) and running
`:grails-gradle-plugins:build --rerun-tasks`: every Copy-derived task
(`processResources`, `jar`, `sourcesJar`, `javadocJar`,
`pluginUnderTestMetadata`) ran clean without it. The block was a copy-paste
from earlier Spring-DM-era builds and is no longer needed.
### Verification
```
./gradlew :grails-gradle-plugins:build
-> BUILD SUCCESSFUL in 1m 13s
-> 30 tests pass (12 newly-relocated bom tests + BomPlatformFunctionalSpec
end-to-end + all pre-existing plugin tests)
-> codenarcMain clean (31 files scanned, 0 violations)
-> validatePlugins clean
```
Zero orphan references to the removed module
(`grails-gradle-bom-property-overrides` and `file('bom-property-overrides')`
both return zero hits across `*.gradle`, `*.kts`, `*.adoc`, `*.md`, `*.groovy`,
`*.java`, `*.yml`, `*.sh`).
### Cross-check vs Spring DM behaviour
Two minor functional differences vs Spring DM that should be documented in
the 7.x -> 8.x upgrade notes (neither used inside `grails-core` itself):
- The `dependencyManagement { dependencies { dependency 'g:a:v' } }` DSL for
arbitrary non-BOM version pins is no longer available. Migration: use
`dependencies { implementation 'g:a:v' }` or `configurations.all {
resolutionStrategy.force 'g:a:v' }`.
- Version-range strings (`1.+`, `[1.0,2.0)`) in BOM `<properties>` values
are now passed through to Gradle's native `useVersion()` rather than Spring
DM's resolver. Zero impact on the Grails / Spring Boot BOM chain (all versions
are concrete); only relevant for users who consume custom BOMs with
range-valued properties.
All other Spring DM features that grails-core relied on
(`dependencyManagement.imports { mavenBom }`, BOM `<exclusions>`,
`overriddenByDependencies = true` / `applyMavenExclusions = true` defaults,
project-scoped `configureEach`) are covered identically by Gradle native
`platform()` + the new property-override plugin.
### Thread status
25 of 28 review threads were resolved in the previous pass. The remaining 3
are addressed by this commit:
| Thread | Reviewer | Resolution |
|---|---|---|
| `PRRT_...AJdXE` | jdaugherty | Consolidated into `grails-gradle-plugins` |
| `PRRT_...AJeU_` | jdaugherty | `DuplicatesStrategy.INCLUDE` removed, build
green without it |
| `PRRT_...85_rr6i` | matrei | `def` applied throughout new Groovy code |
### Deferred to follow-up (tracked separately)
- `grails { autoApplyBom = true }` configurability (thread `PRRT_...AJjff`)
- Configuration cache compatibility for `afterEvaluate` + detached
configuration access (thread `PRRT_...AJhsl`)
- Migrate `build-logic/docs-core/ExtractDependenciesTask.groovy` away from
Spring DM's shaded Maven model classes
PR description has been revised to describe the final state of the change
set rather than the journey.
--
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]
