jdaugherty commented on code in PR #15467:
URL: https://github.com/apache/grails-core/pull/15467#discussion_r3343148547
##########
build-logic/docs-core/build.gradle:
##########
@@ -48,7 +48,6 @@ dependencies {
api 'org.yaml:snakeyaml:2.4'
api
"org.asciidoctor:asciidoctorj:${gradleBomDependencyVersions['asciidoctorj.version']}"
- implementation
"org.springframework.boot:spring-boot-gradle-plugin:${gradleBomDependencyVersions['spring-boot.version']}"
Review Comment:
While I understand we're proposing to switch, don't we still need to offer
this support to existing Grails apps? That means we should have a test app
that uses it so we know we don't break anything.
##########
build-logic/docs-core/src/main/groovy/org/apache/grails/gradle/tasks/bom/ExtractDependenciesTask.groovy:
##########
@@ -257,93 +259,232 @@ abstract class ExtractDependenciesTask extends
DefaultTask {
}
Properties populatePlatformDependencies(CoordinateVersionHolder
bomCoordinates, List<CoordinateHolder> exclusionRules, Map<CoordinateHolder,
ExtractedDependencyConstraint> constraints, boolean error = true, int level =
0) {
- Dependency bomDependency =
dependencyHandler.create("${bomCoordinates.coordinates}@pom")
- Configuration dependencyConfiguration =
configurationContainer.detachedConfiguration(bomDependency)
+ def bomDependency =
dependencyHandler.create("${bomCoordinates.coordinates}@pom")
+ def dependencyConfiguration =
configurationContainer.detachedConfiguration(bomDependency).tap {
+ transitive = false
+ }
File bomPomFile = dependencyConfiguration.singleFile
- MavenXpp3Reader reader = new MavenXpp3Reader()
- Model model = reader.read(new FileReader(bomPomFile))
+ def doc = parsePom(bomPomFile)
Review Comment:
I strongly disagree with this approach. We're now rolling our own xml
parsing instead of using files that adhere to the maven standard. Libraries
like plexus and others exist to parse this. We shouldn't be rolling our own.
Especially because this is the dependencies task which is meant to resolve
properties like maven would. There is no guarantee this resolution will mirror
what upstream Maven does by not adopting the maven specific libraries.
--
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]
