jamesfredley commented on code in PR #15730:
URL: https://github.com/apache/grails-core/pull/15730#discussion_r3470889626
##########
grails-shell-cli/src/main/groovy/org/grails/cli/boot/GrailsDependencyVersions.groovy:
##########
@@ -85,27 +86,62 @@ class GrailsDependencyVersions implements
DependencyManagement {
@CompileDynamic
void addDependencyManagement(GPathResult pom) {
- versionProperties = pom.properties.'*'.collectEntries { [(it.name()):
it.text()] }
Review Comment:
The Maven model builder is the broader cleanup direction, and the shell CLI
already uses it for @DependencyManagementBom. I kept this PR narrower because
GrailsDependencyVersions also exposes legacy versionProperties used by
create-app plus the existing group/artifact lookup contract, so replacing it
with the model path would be a larger refactor than the BOM-pin cleanup. This
patch keeps the existing contract, adds recursive imports and precedence
protection needed for the removed pins, and now matches the model-builder
behavior by failing fast on unresolved imported BOMs.
##########
grails-shell-cli/src/main/groovy/org/grails/cli/boot/GrailsDependencyVersions.groovy:
##########
@@ -115,7 +151,11 @@ class GrailsDependencyVersions implements
DependencyManagement {
addDependencyManagement(importedPom)
}
} catch (Exception e) {
- // If the imported BOM cannot be resolved, skip it
Review Comment:
Agreed. In the healthy build this should not occur, and if it does occur the
build/CLI setup should fail rather than continue with incomplete dependency
management. Pushed a5a70ec to make imported BOM resolution fail fast with the
failing coordinates, and updated GrailsDependencyVersionsSpec to assert that
behavior and preserve the original cause.
##########
grails-shell-cli/src/main/groovy/org/grails/cli/boot/GrailsDependencyVersions.groovy:
##########
@@ -85,27 +86,62 @@ class GrailsDependencyVersions implements
DependencyManagement {
@CompileDynamic
void addDependencyManagement(GPathResult pom) {
- versionProperties = pom.properties.'*'.collectEntries { [(it.name()):
it.text()] }
+ // Capture this POM's <properties> in a local map so that ${...}
version references are
+ // resolved against the POM that declared them, and so recursing into
an imported BOM
+ // cannot clobber the property table mid-iteration. Merge into the
shared map with
+ // first-writer-wins precedence so Grails' own versions win over
imported BOM versions.
+ Map<String, String> localProperties =
pom.properties.'*'.collectEntries { [(it.name()): it.text()] }
+ localProperties.each { String key, String value ->
versionProperties.putIfAbsent(key, value) }
+
+ List<Map<String, String>> grailsImports = []
+ List<Map<String, String>> thirdPartyImports = []
+
pom.dependencyManagement.dependencies.dependency.each { dep ->
String groupId = dep.groupId.text()
String artifactId = dep.artifactId.text()
- String version = versionLookup(dep.version.text())
+ String version = versionLookup(dep.version.text(), localProperties)
String scope = dep.scope.text()
Review Comment:
Thanks, yes. The shell CLI breakage is that GrailsDependencyVersions is the
dependency-management source used by MavenProfileRepository/CreateAppCommand
and the CLI DependencyResolutionContext. Once this PR removes direct pins,
versions such as mongodb, junit, rxjava3, byte-buddy, and the Jakarta APIs live
only behind spring-boot-dependencies; without recursing into that import, the
legacy CLI no longer exposes those managed versions. The added specs cover a
Spring Boot-only managed dependency, duplicate imports, nested BOM imports, and
Grails-first precedence.
--
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]
