matrei commented on code in PR #14324:
URL: https://github.com/apache/grails-core/pull/14324#discussion_r2053472705
##########
grails-test-examples/mongodb/base/build.gradle:
##########
@@ -6,10 +6,10 @@ apply plugin: 'org.apache.grails.gradle.grails-web'
dependencies {
- implementation platform("org.apache.grails:grails-bom:$grailsVersion")
- integrationTestImplementation
platform("org.apache.grails:grails-bom:$grailsVersion")
+ implementation platform(project(':grails-bom'))
+ integrationTestImplementation platform(project(':grails-bom'))
Review Comment:
Do we need this?
##########
build.gradle:
##########
@@ -35,6 +38,10 @@ subprojects {
}
}
+ if(name.startsWith('grails-doc') || name.startsWith('grails-data-doc') ||
name.endsWith('-docs') || name.endsWith("-test-report")) {
Review Comment:
Space after if?
Align on single quotes?
##########
gradle/functional-test-config.gradle:
##########
@@ -31,30 +46,60 @@ def debugArguments = [
'-Xmx2g', '-Xdebug', '-Xnoagent', '-Djava.compiler=NONE',
'-Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=5005'
]
-tasks.withType(Test).configureEach {
- onlyIf { !project.hasProperty('skipFunctionalTests') }
- useJUnitPlatform()
- testLogging {
+tasks.withType(Test).configureEach { Test task ->
+ boolean isHibernate5 =
!project.name.startsWith('grails-test-examples-hibernate5')
+ boolean isMongo = !project.name.startsWith('grails-test-examples-mongodb')
+
+ onlyIf {
+ if (project.hasProperty('skipFunctionalTests')) {
+ return false
+ }
+
+ if (project.hasProperty('onlyHibernate5Tests')) {
+ if (isHibernate5) {
+ return false
+ }
+ }
+
+ if (project.hasProperty('onlyMongodbTests')) {
+ if (isMongo) {
+ return false
+ }
+ }
+
+ if (project.hasProperty('onlyCoreTests')) {
+ return false
+ }
+
+ if(project.hasProperty('skipTests')) {
+ return false
+ }
+
+ return true
+ }
+
+ if (isMongo && project.hasProperty('serializeMongoTests')) {
+ // if the developer decides to run a local mongo instance, the tests
must be serialized instead of launching containers as needed
+ task.outputs.dir
rootProject.layout.buildDirectory.dir('mongo-test-serialize')
+ }
+
+ task.useJUnitPlatform()
+ task.testLogging {
events('passed', 'skipped', 'failed', 'standardOut', 'standardError')
showExceptions = true
exceptionFormat = 'full'
showCauses = true
showStackTraces = true
}
if (findProperty('testForked')) {
- if (isCiBuild) {
- maxParallelForks = findProperty('testMaxParallelFork') ?: 2
- forkEvery = findProperty('testForkEvery') ?: 0
- } else {
- maxParallelForks = findProperty('testMaxParallelFork') ?: 4
- forkEvery = findProperty('testForkEvery') ?: 0
- }
+ task.maxParallelForks = configuredTestParallel
+ task.forkEvery = (findProperty('testForkEvery') ?: 0)
}
if (findProperty('testJvmArgs')) {
- jvmArgs = findProperty('testJvmArgs')
+ task.jvmArgs = findProperty('testJvmArgs')
Review Comment:
It is already established that `testJvmArgs` exists, use
`property('testJvmArgs')`?
Cast to `List`?
##########
grails-test-examples/mongodb/hibernate5/build.gradle:
##########
@@ -6,15 +6,15 @@ apply plugin: 'org.apache.grails.gradle.grails-web'
dependencies {
- implementation platform("org.apache.grails:grails-bom:$grailsVersion")
- integrationTestImplementation
platform("org.apache.grails:grails-bom:$grailsVersion")
+ implementation platform(project(':grails-bom'))
+ integrationTestImplementation platform(project(':grails-bom'))
Review Comment:
Do we need this?
##########
grails-gradle/docs-core/src/main/groovy/org/apache/grails/gradle/tasks/bom/CoordinateVersionHolder.groovy:
##########
@@ -0,0 +1,25 @@
+package org.apache.grails.gradle.tasks.bom
+
+import groovy.transform.CompileStatic
+import groovy.transform.EqualsAndHashCode
+import groovy.transform.ToString
+import org.gradle.api.GradleException
+
+@EqualsAndHashCode(includes = ['version'], callSuper = true)
+@CompileStatic
+@ToString
+class CoordinateVersionHolder extends CoordinateHolder {
+ String version
+
+ CoordinateHolder toCoordinateHolder() {
+ new CoordinateHolder(groupId: groupId, artifactId: artifactId)
+ }
+
+ String getCoordinates() {
+ if (!version) {
+ throw new GradleException("Constraint does not have a version:
${this}")
+ }
+
+ "${groupId}:${artifactId}:${version}" as String
Review Comment:
Skip curly braces and no need to cast?
##########
grails-test-suite-persistence/build.gradle:
##########
@@ -95,4 +95,9 @@ apply {
// java-configuration must be applied first since tasks are now lazy
registered
from rootProject.layout.projectDirectory.file('gradle/java-config.gradle')
from rootProject.layout.projectDirectory.file('gradle/test-config.gradle')
-}
\ No newline at end of file
+}
+
+tasks.withType(Groovydoc).configureEach {
+ // tests do not have groovydoc
+ enabled = false
+}
Review Comment:
Move above `apply {}` block?
##########
grails-gradle/docs-core/src/main/groovy/org/apache/grails/gradle/tasks/bom/ExtractDependenciesTask.groovy:
##########
@@ -0,0 +1,262 @@
+package org.apache.grails.gradle.tasks.bom
+
+import io.spring.gradle.dependencymanagement.org.apache.maven.model.Model
+import
io.spring.gradle.dependencymanagement.org.apache.maven.model.io.xpp3.MavenXpp3Reader
+import org.gradle.api.DefaultTask
+import org.gradle.api.GradleException
+import org.gradle.api.NamedDomainObjectProvider
+import org.gradle.api.artifacts.*
+import org.gradle.api.artifacts.component.ModuleComponentSelector
+import org.gradle.api.artifacts.result.DependencyResult
+import org.gradle.api.artifacts.result.ResolvedDependencyResult
+import org.gradle.api.file.ConfigurableFileCollection
+import org.gradle.api.file.RegularFileProperty
+import org.gradle.api.provider.MapProperty
+import org.gradle.api.provider.Property
+import org.gradle.api.tasks.*
+
+import java.util.regex.Pattern
+
+/**
+ * Grails Bom files define their dependencies in a series of maps, this task
takes those maps and generates an
+ * asciidoc file containing all of the resolve dependencies and their versions
in the bom.
+ */
+@CacheableTask
+abstract class ExtractDependenciesTask extends DefaultTask {
+ @InputFiles
+ @Classpath
+ abstract ConfigurableFileCollection getDependencyArtifacts()
+
+ @OutputFile
+ abstract RegularFileProperty getDestination()
+
+ @Input
+ abstract MapProperty<String,String> getVersions()
+
+ @Input
+ abstract Property<String> getConfigurationName()
+
+ @Input
+ abstract MapProperty<String, String> getPlatformDefinitions()
+
+ @Input
+ abstract MapProperty<String, String> getDefinitions()
+
+ void setConfiguration(NamedDomainObjectProvider<Configuration> config) {
+ dependencyArtifacts.from(config)
+ configurationName.set(config.name)
+ }
+
+ ExtractDependenciesTask() {
+ doFirst {
+ if(!project.pluginManager.hasPlugin('java-platform')) {
+ throw new GradleException("The 'java-platform' plugin must be
applied to the project to use this task.")
+ }
+ }
+ }
+
+ @TaskAction
+ void generate() {
+ File outputFile = destination.get().asFile
+ outputFile.parentFile.mkdirs()
+
+ Map<CoordinateHolder, ExtractedDependencyConstraint> constraints = [:]
+ PropertyNameCalculator propertyNameCalculator = new
PropertyNameCalculator(
+ getPlatformDefinitions().get(),
+ getDefinitions().get(),
+ getVersions().get()
+ )
+
+ Configuration configuration =
project.configurations.named(configurationName.get()).get()
+ if(!configuration.canBeResolved) {
+ throw new GradleException("The configuration ${configuration.name}
must be resolvable to use this task.")
+ }
+
+ populateExplicitConstraints(configuration, constraints,
propertyNameCalculator)
+
+ Map<CoordinateHolder, List<CoordinateHolder>> exclusions =
determineExclusions(configuration)
+ populateInheritedConstraints(configuration, exclusions, constraints,
propertyNameCalculator)
+
+ List<String> lines = generateAsciiDoc(constraints)
+ destination.get().asFile.withWriter { writer ->
+ writer.writeLine '[cols="1,1,1,1,1,1", options="header"]'
+ writer.writeLine '|==='
+ writer.writeLine '| Index | Group | Artifact | Version | Property
Name | Source'
+ lines.each { line ->
+ writer.writeLine(line)
+ }
+ writer.writeLine '|==='
+ }
+ }
+
+ private List<String> generateAsciiDoc(Map<CoordinateHolder,
ExtractedDependencyConstraint> constraints) {
+ List lines = []
+ constraints.values().sort { ExtractedDependencyConstraint a,
ExtractedDependencyConstraint b -> a.groupId <=> b.groupId ?: a.artifactId <=>
b.artifactId }.withIndex().each {
+ int position = it.v2 + 1
+ lines << "| ${position} | ${it.v1.groupId} | ${it.v1.artifactId} |
${it.v1.version} | ${it.v1.versionPropertyReference ?: ''} | ${it.v1.source} "
+ }
+ lines
+ }
+
+ private populateExplicitConstraints(Configuration configuration,
+ Map<CoordinateHolder,
ExtractedDependencyConstraint> constraints,
+ PropertyNameCalculator
propertyNameCalculator) {
+ configuration.getAllDependencyConstraints().all { DependencyConstraint
constraint ->
+ String groupId = constraint.module.group as String
+ String artifactId = constraint.module.name as String
+ String artifactVersion = constraint.version as String
+
+ ExtractedDependencyConstraint extractConstraint =
propertyNameCalculator.calculate(groupId, artifactId, artifactVersion, false)
?: new ExtractedDependencyConstraint(groupId: groupId, artifactId: artifactId,
version: artifactVersion)
+ extractConstraint.source = project.name
+ constraints.put(new CoordinateHolder(groupId:
extractConstraint.groupId, artifactId: extractConstraint.artifactId),
extractConstraint)
+ }
+ }
+
+ private Map<CoordinateHolder, List<CoordinateHolder>>
determineExclusions(Configuration configuration) {
+ Map<CoordinateHolder, List<CoordinateHolder>> exclusions =
[:].withDefault { [] }
+ for (Dependency dep : configuration.allDependencies) {
+ if (dep instanceof ModuleDependency) {
+ CoordinateHolder foundCoordinate = new
CoordinateHolder(groupId: dep.group, artifactId: dep.name)
+ dep.excludeRules.each { ExcludeRule exclusionRule ->
+ CoordinateHolder exclusion = new CoordinateHolder(groupId:
exclusionRule.group, artifactId: exclusionRule.module)
+ exclusions.get(foundCoordinate).add(exclusion)
+ }
+ }
+ }
+ exclusions
+ }
+
+ private void populateInheritedConstraints(Configuration configuration,
Map<CoordinateHolder, List<CoordinateHolder>> exclusions, Map<CoordinateHolder,
ExtractedDependencyConstraint> constraints, PropertyNameCalculator
propertyNameCalculator) {
+ for (DependencyResult result :
configuration.incoming.resolutionResult.allDependencies) {
+ if(!(result instanceof ResolvedDependencyResult)) {
+ throw new GradleException("Dependencies should be resolved
prior to running this task.")
+ }
+
+ ResolvedDependencyResult dep = (ResolvedDependencyResult) result
+ ModuleComponentSelector moduleComponentSelector = dep.requested as
ModuleComponentSelector
+
+ // Any non-constraint via api dependency should *always* be a
platform dependency, so expand each of those
+ CoordinateVersionHolder bomCoordinate = new
CoordinateVersionHolder(
+ groupId: moduleComponentSelector.group,
+ artifactId: moduleComponentSelector.module,
+ version: moduleComponentSelector.version
+ )
+
+ // fetch the BOM as a pom file so it can be expanded
+ ExtractedDependencyConstraint constraint =
propertyNameCalculator.calculate(bomCoordinate.groupId,
bomCoordinate.artifactId, bomCoordinate.version, true)
+ constraint.source = bomCoordinate.artifactId
+ constraints.put(bomCoordinate.toCoordinateHolder(), constraint)
+
+ List<CoordinateHolder> exclusionRules =
exclusions.get(bomCoordinate.toCoordinateHolder())
+ populatePlatformDependencies(bomCoordinate, exclusionRules,
constraints)
+ }
+ }
+
+ Properties populatePlatformDependencies(CoordinateVersionHolder
bomCoordinates, List<CoordinateHolder> exclusionRules, Map<CoordinateHolder,
ExtractedDependencyConstraint> constraints, boolean error = true, int level =
0) {
+ Dependency bomDependency =
project.dependencies.create("${bomCoordinates.coordinates}@pom")
+ Configuration dependencyConfiguration =
project.configurations.detachedConfiguration(bomDependency)
+ File bomPomFile = dependencyConfiguration.singleFile
+
+ MavenXpp3Reader reader = new MavenXpp3Reader()
+ Model model = reader.read(new FileReader(bomPomFile))
+
+ Properties versionProperties = new Properties()
+ if(model.parent) {
+ // Need to populate the parent bom if it's present first
+ CoordinateVersionHolder parentBom = new CoordinateVersionHolder(
+ groupId: model.parent.groupId,
+ artifactId: model.parent.artifactId,
+ version: model.parent.version
+ )
+ populatePlatformDependencies(parentBom, exclusionRules,
constraints,false, level + 1)?.entrySet()?.each { Map.Entry<Object, Object>
entry ->
+ versionProperties.put(entry.key, entry.value)
+ }
+ }
+ model.properties.entrySet().each { Map.Entry<Object, Object> entry ->
+ versionProperties.put(entry.key, entry.value)
+ }
+ versionProperties.put('project.groupId', bomCoordinates.groupId)
+ versionProperties.put('project.version', bomCoordinates.version)
+
+ if (model.dependencyManagement &&
model.dependencyManagement.dependencies) {
+ for
(io.spring.gradle.dependencymanagement.org.apache.maven.model.Dependency
depItem : model.dependencyManagement.dependencies) {
+ CoordinateHolder baseCoordinates = new CoordinateHolder(
+ groupId: depItem.groupId,
+ artifactId: depItem.artifactId
+ )
+
+ CoordinateHolder resolvedCoordinates = new CoordinateHolder(
+ groupId:
resolveMavenProperty(baseCoordinates.coordinatesWithoutVersion,
depItem.groupId, versionProperties),
+ artifactId:
resolveMavenProperty(baseCoordinates.coordinatesWithoutVersion,
depItem.artifactId, versionProperties)
+ )
+
+ if (!constraints.containsKey(resolvedCoordinates)) {
+ boolean isExcluded = exclusionRules.any { CoordinateHolder
excludedCoordinate ->
+ if (excludedCoordinate.groupId &&
excludedCoordinate.artifactId) {
+ return resolvedCoordinates == excludedCoordinate
+ }
+
+ if (excludedCoordinate.groupId &&
!excludedCoordinate.artifactId) {
+ return depItem.groupId ==
excludedCoordinate.groupId
+ }
+
+ if (!excludedCoordinate.groupId &&
excludedCoordinate.artifactId) {
+ return depItem.artifactId ==
excludedCoordinate.artifactId
+ }
+
+ false
+ }
+
+ if (!isExcluded) {
+ String resolvedVersion =
resolveMavenProperty(resolvedCoordinates.coordinatesWithoutVersion,
depItem.version, versionProperties)
+ String propertyName = depItem.version.contains('$') ?
depItem.version : null
+ ExtractedDependencyConstraint constraint = new
ExtractedDependencyConstraint(
+ groupId: resolvedCoordinates.groupId,
artifactId: resolvedCoordinates.artifactId,
+ version: resolvedVersion,
versionPropertyReference: propertyName, source: bomCoordinates.artifactId
+ )
+ if (depItem.scope == 'import') {
+ constraints.put(resolvedCoordinates, constraint)
+
+ CoordinateVersionHolder resolvedBomCoordinates =
new CoordinateVersionHolder(
+ groupId: resolvedCoordinates.groupId,
+ artifactId: resolvedCoordinates.artifactId,
+ version: resolvedVersion
+ )
+
populatePlatformDependencies(resolvedBomCoordinates, exclusionRules,
constraints, error, level + 1)
+ } else {
+ constraints.put(resolvedCoordinates,constraint)
+ }
+ }
+ }
+ }
+ } else {
+ if (error) {
+ // only the boms we directly include need to error since we
expect a dependency management;
+ // parent boms are sometimes use to share properties so we
need to not error on these cases
+ throw new GradleException("BOM ${bomCoordinates.coordinates}
has no dependencyManagement section.")
+ }
+ }
+
+ versionProperties
+ }
+
+ private String resolveMavenProperty(String errorDescription, String
dynamicVersion, Map properties, int maxIterations = 10) {
+ Pattern dynamicPattern = ~/\$\{([^}]+)\}/
+ String expandedVersion = dynamicVersion
+
+ int iterations = 0
+ while ((expandedVersion =~ dynamicPattern).find() && iterations <
maxIterations) {
+ expandedVersion = expandedVersion.replaceAll(dynamicPattern) {
String fullMatch, String propName ->
+ String replacement = properties[propName] as String
+ return replacement ? replacement : fullMatch
+ }
+ iterations++
+ }
+
+ if ((expandedVersion =~ dynamicPattern).find()) {
+ logger.warn("Reached max iterations for ${errorDescription} while
resolving properties in: ${dynamicVersion}")
Review Comment:
Lazy log message evaluation?
##########
grails-test-suite-uber/build.gradle:
##########
@@ -134,4 +134,9 @@ tasks.named('test', Test).configure {
apply {
// java-configuration must be applied first since tasks are now lazy
registered
from rootProject.layout.projectDirectory.file('gradle/java-config.gradle')
-}
\ No newline at end of file
+}
+
+tasks.withType(Groovydoc).configureEach {
+ // tests do not have groovydoc
+ enabled = false
+}
Review Comment:
Move above `apply {}` block?
##########
grails-data-mongodb/gson-templates/build.gradle:
##########
@@ -0,0 +1,56 @@
+plugins {
+ id 'groovy'
+ id 'java-library'
+}
+
+version = projectVersion
+group = 'org.apache.grails'
+
+ext {
+ pomTitle = 'Grails MongoDB JSON Views'
+ pomDescription = 'Provides JSON Views for MongoDB to the Grails framework.'
+ pomDevelopers = ['puneetbehl': 'Puneet Behl']
+}
+
+dependencies {
+
+ implementation(platform(project(':grails-bom')))
Review Comment:
Remove parentheses for `implementation`?
##########
gradle/test-config.gradle:
##########
@@ -13,7 +13,29 @@ dependencies {
}
tasks.withType(Test).configureEach {
- onlyIf { !project.hasProperty('onlyFunctionalTests') }
+ onlyIf {
+ if(project.hasProperty('onlyFunctionalTests')) {
+ return false
+ }
+
+ if (project.hasProperty('onlyMongodbTests')) {
+ return false
+ }
+
+ if (project.hasProperty('onlyHibernate5Tests')) {
+ return false
+ }
+
+ if(project.hasProperty('skipCoreTests')) {
+ return false
+ }
+
+ if(project.hasProperty('skipTests')) {
+ return false
+ }
+
+ true
+ }
Review Comment:
Make more declarative?
```groovy
onlyIf {
![
'onlyFunctionalTests',
'onlyHibernate5Tests',
'onlyMongodbTests',
'skipCoreTests',
'skipTests'
].find {
project.hasProperty(it)
}
}
```
##########
gradle.properties:
##########
@@ -31,16 +39,11 @@ org.gradle.daemon=true
org.gradle.jvmargs=-Dfile.encoding=UTF-8 -Xmx1536M -XX:MaxMetaspaceSize=1024M
# libraries only specific to test apps, these should not be exposed
-hibernateVersion=5.6.15.Final
jbossTransactionApiVersion=2.0.0.Final
micronautVersion=4.6.5
micronautSerdeJacksonVersion=2.11.0
springSecurityCoreVersion=7.0.0-SNAPSHOT
Review Comment:
Rename to `grailsSpringSecurityVersion`? (It applies to all Grails Spring
Security plugins)
##########
grails-test-examples/hibernate5/standalone-hibernate/build.gradle:
##########
@@ -17,6 +17,10 @@ dependencies {
testImplementation 'org.spockframework:spock-core'
testRuntimeOnly 'org.slf4j:slf4j-simple'
+
+ // This project also has unit tests in it, so we must define the launcher
per
+ //
https://docs.gradle.org/8.3/userguide/upgrading_version_8.html#test_framework_implementation_dependencies
+ add('testRuntimeOnly', 'org.junit.platform:junit-platform-launcher')
Review Comment:
We can use normal dependency config syntax here.
##########
grails-test-examples/hibernate5/grails-multiple-datasources/build.gradle:
##########
@@ -10,13 +10,13 @@ configurations {
}
dependencies {
- implementation platform("org.apache.grails:grails-bom:$grailsVersion")
- //astTransformation platform("org.apache.grails:grails-bom:$grailsVersion")
+ implementation platform(project(':grails-bom'))
+ //astTransformation platform(project(':grails-bom'))
Review Comment:
Remove?
##########
grails-data-mongodb/grails-plugin/build.gradle:
##########
@@ -23,10 +22,10 @@ ext {
dependencies {
- implementation platform("org.apache.grails:grails-bom:$grailsVersion")
- testFixturesCompileOnly
platform("org.apache.grails:grails-bom:$grailsVersion")
+ implementation platform(project(':grails-bom'))
+ testFixturesCompileOnly platform(project(':grails-bom'))
Review Comment:
Is this needed?
##########
build.gradle:
##########
@@ -1,6 +1,8 @@
ext {
grailsVersion = projectVersion
isCiBuild = System.getenv().get('CI') as Boolean
+ configuredTestParallel = find('maxTestParallel') as Integer ?: (isCiBuild
? 3 : Runtime.runtime.availableProcessors() * 3/4 as int ?: 1)
Review Comment:
Is this meant to be `findProperty`?
##########
grails-data-docs/guide-whats-new/build.gradle:
##########
Review Comment:
Remove `project` references?
##########
.github/workflows/gradle.yml:
##########
@@ -81,10 +62,9 @@ jobs:
develocity-access-key: ${{ secrets.GRAILS_DEVELOCITY_ACCESS_KEY }}
- name: "🔨 Build project"
id: build
- run: ./gradlew build assemble groovydoc -PskipFunctionalTests
--continue --stacktrace
+ run: ./gradlew build assemble groovydoc -PskipFunctionalTests
--continue --stacktrace -PskipMongodbTests -PskipHibernate5Tests
Review Comment:
Break over multiple lines and keep `-P` grouped together?
##########
.github/workflows/gradle.yml:
##########
@@ -105,10 +85,59 @@ jobs:
with:
develocity-access-key: ${{ secrets.GRAILS_DEVELOCITY_ACCESS_KEY }}
- name: "🔨 Functional Tests"
- run: ./gradlew build -PonlyFunctionalTests --stacktrace
+ run: ./gradlew build -PonlyFunctionalTests --stacktrace
-PskipMongodbTests -PskipHibernate5Tests
+ mongodbFunctional:
+ name: "Mongodb Functional Tests"
+ runs-on: ubuntu-24.04
+ strategy:
+ fail-fast: false
+ matrix:
+ java: [ 17, 21 ]
+ mongodb-version: [ '5.0', '6.0', '7.0', '8.0' ]
+ steps:
+ - name: "📥 Checkout the repository"
+ uses: actions/checkout@v4
+ - name: "☕️ Setup JDK"
+ uses: actions/setup-java@v4
+ with:
+ java-version: ${{ matrix.java }}
+ distribution: liberica
+ - name: "🐘 Setup Gradle"
+ uses: gradle/actions/setup-gradle@v4
+ with:
+ develocity-access-key: ${{ secrets.GRAILS_DEVELOCITY_ACCESS_KEY }}
+ - name: "🔨 Run Build"
+ id: build
+ env:
+ GITHUB_MAVEN_PASSWORD: ${{ secrets.GITHUB_TOKEN }}
+ run: ./gradlew cleanTest build --continue -PonlyMongodbTests
-PmongodbContainerVersion=${{ matrix.mongodb-version }}
Review Comment:
Break over multiple lines?
##########
grails-data-hibernate5/dbmigration/build.gradle:
##########
@@ -30,23 +29,27 @@ dependencies {
exclude group: 'org.liquibase', module: 'liquibase-commercial'
}
- api('org.apache.grails:grails-shell-cli') {
+ api(project(':grails-shell-cli')) {
exclude group: 'org.slf4j', module: 'slf4j-simple'
+
+ //TODO: These excludes groovy right now, where previously it didn't.
Review Comment:
Improve comment?
##########
grails-bom/build.gradle:
##########
@@ -57,175 +59,25 @@ dependencies {
}
}
-
configurations.register('bomDependencies').configure {
it.canBeResolved = true
it.transitive = true
it.extendsFrom(configurations.named('api').get())
}
-String resolveMavenProperty(String errorDescription, String dynamicVersion,
Map properties, int maxIterations = 10) {
- def dynamicPattern = ~/\$\{([^}]+)\}/
- String expandedVersion = dynamicVersion
+tasks.register('extractConstraints', ExtractDependenciesTask).configure {
ExtractDependenciesTask it ->
+ it.configuration =configurations.named('bomDependencies')
Review Comment:
Spacing?
##########
.github/workflows/gradle.yml:
##########
@@ -105,10 +85,59 @@ jobs:
with:
develocity-access-key: ${{ secrets.GRAILS_DEVELOCITY_ACCESS_KEY }}
- name: "🔨 Functional Tests"
- run: ./gradlew build -PonlyFunctionalTests --stacktrace
+ run: ./gradlew build -PonlyFunctionalTests --stacktrace
-PskipMongodbTests -PskipHibernate5Tests
Review Comment:
Break over multiple lines and keep `-P` grouped together?
##########
grails-datamapping-support/build.gradle:
##########
@@ -6,10 +6,23 @@ plugins {
version = projectVersion
group = 'org.apache.grails.data'
+ext {
+ pomTitle = 'Grails GORM'
+ pomDescription = 'GORM - Grails Data Access Framework'
+ pomDevelopers = [
+ 'graemerocher': 'Graeme Rocher',
+ 'jeffscottbrown': 'Jeff Brown',
+ 'burtbeckwith': 'Burt Beckwith',
+ 'jameskleeh': 'James Kleeh',
+ 'puneetbehl': 'Puneet Behl',
+ 'jamesfredley': 'James Fredley'
+ ]
+}
+
dependencies {
- implementation platform("org.apache.grails:grails-bom:$grailsVersion")
- api platform("org.apache.grails:grails-bom:$grailsVersion")
+ implementation platform(project(':grails-bom'))
+ api platform(project(':grails-bom'))
Review Comment:
Is it correct to have an `api` dependency on the bom?
##########
gradle/assemble-root-config.gradle:
##########
@@ -99,7 +99,17 @@ tasks.register('assemble', Zip).configure {
from(rootProject.layout.projectDirectory) {
// Some of these are probably not needed as they are not present
in the project folder
include 'bin/grails', 'bin/grails.bat', 'lib/', 'media/',
'samples/', 'scripts/', 'LICENSE', 'INSTALL'
- exclude 'ant/bin', 'src/grails', 'src/war', 'grails-doc'
+
+ def pathsToExclude = [
+ 'ant/bin', 'src/grails', 'src/war'
+ ]
+ docProjects.each { String projectName ->
Review Comment:
Remove type from parameter?
##########
grails-data-docs/guide-rx/build.gradle:
##########
Review Comment:
Remove `project` references? (They are implicit)
##########
gradle/assemble-root-config.gradle:
##########
@@ -99,7 +99,17 @@ tasks.register('assemble', Zip).configure {
from(rootProject.layout.projectDirectory) {
// Some of these are probably not needed as they are not present
in the project folder
include 'bin/grails', 'bin/grails.bat', 'lib/', 'media/',
'samples/', 'scripts/', 'LICENSE', 'INSTALL'
- exclude 'ant/bin', 'src/grails', 'src/war', 'grails-doc'
+
+ def pathsToExclude = [
+ 'ant/bin', 'src/grails', 'src/war'
+ ]
+ docProjects.each { String projectName ->
+ pathsToExclude <<
rootProject.projectDir.relativePath(project(projectName as String).projectDir)
+ }
+ testProjects.each { def projectName ->
Review Comment:
Remove `def` from parameter?
##########
grails-gradle/docs-core/src/main/groovy/org/apache/grails/gradle/tasks/bom/CoordinateHolder.groovy:
##########
@@ -0,0 +1,17 @@
+package org.apache.grails.gradle.tasks.bom
+
+import groovy.transform.CompileStatic
+import groovy.transform.EqualsAndHashCode
+import groovy.transform.ToString
+
+@EqualsAndHashCode(includes = ['groupId', 'artifactId'])
+@CompileStatic
+@ToString
+class CoordinateHolder {
+ String groupId
+ String artifactId
+
+ String getCoordinatesWithoutVersion() {
+ "${groupId}:${artifactId}" as String
Review Comment:
Skip curly braces and no need to cast?
##########
dependencies.gradle:
##########
@@ -119,4 +119,10 @@ ext {
'rxjava2' :
"io.reactivex.rxjava2:rxjava:${bomDependencyVersions['rxjava2.version']}",
'rxjava3' :
"io.reactivex.rxjava3:rxjava:${bomDependencyVersions['rxjava3.version']}",
]
+
+ // Because pom exclusions aren't properly supported by gradle, we can't
inherit the grails-gradle-bom
+ // this requires copying the platforms selectively to the grails-bom. Due
this via these combinations
Review Comment:
Typo?
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/views/AbstractGroovyTemplateCompileTask.groovy:
##########
@@ -54,6 +61,11 @@ abstract class AbstractGroovyTemplateCompileTask extends
AbstractCompile {
fileExtension = objectFactory.property(String)
scriptBaseName = objectFactory.property(String)
compilerName = objectFactory.property(String)
+ grailsConfigurationPaths = objectFactory.fileCollection()
+ grailsConfigurationPaths.from(
+ //TODO: historically this only used .yml, should it explore
all configuration paths?
+
project.layout.projectDirectory.file("grails-app/conf/application.yml")
Review Comment:
Single quotes for strings?
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/views/gsp/GroovyPageForkCompileTask.groovy:
##########
@@ -73,6 +79,11 @@ abstract class GroovyPageForkCompileTask extends
AbstractCompile {
srcDir = objectFactory.directoryProperty()
compileOptions = objectFactory.newInstance(ViewCompileOptions.class)
serverpath = objectFactory.property(String)
+ grailsConfigurationPaths = objectFactory.fileCollection()
+ grailsConfigurationPaths.from(
+
project.layout.projectDirectory.file("grails-app/conf/application.yml"),
+
project.layout.projectDirectory.file("grails-app/conf/application.groovy")
Review Comment:
Single quotes for strings?
##########
grails-test-suite-base/build.gradle:
##########
@@ -51,4 +51,9 @@ dependencies {
apply {
// java-configuration must be applied first since tasks are now lazy
registered
from rootProject.layout.projectDirectory.file('gradle/java-config.gradle')
-}
\ No newline at end of file
+}
+
+tasks.withType(Groovydoc).configureEach {
+ // tests do not have groovydoc
+ enabled = false
+}
Review Comment:
Move above `apply {}` block?
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/views/gsp/GroovyPageForkCompileTask.groovy:
##########
@@ -159,7 +165,7 @@ abstract class GroovyPageForkCompileTask extends
AbstractCompile {
@Input
protected String getCompilerName() {
- "org.grails.web.pages.GroovyPageCompilerForkTask"
+ "org.grails.web.pages.GroovyPageForkedCompiler"
Review Comment:
Single quotes? (also below)
##########
grails-test-suite-web/build.gradle:
##########
@@ -73,4 +73,9 @@ apply {
// java-configuration must be applied first since tasks are now lazy
registered
from rootProject.layout.projectDirectory.file('gradle/java-config.gradle')
from rootProject.layout.projectDirectory.file('gradle/test-config.gradle')
-}
\ No newline at end of file
+}
+
+tasks.withType(Groovydoc).configureEach {
+ // tests do not have groovydoc
+ enabled = false
+}
Review Comment:
Move above `apply {}` block?
##########
grails-test-examples/hibernate5/spring-boot-hibernate/build.gradle:
##########
@@ -20,6 +20,10 @@ dependencies {
runtimeOnly 'org.springframework.boot:spring-boot-starter-tomcat'
testImplementation 'org.spockframework:spock-core'
+
+ // This project also has unit tests in it, so we must define the launcher
per
+ //
https://docs.gradle.org/8.3/userguide/upgrading_version_8.html#test_framework_implementation_dependencies
+ add('testRuntimeOnly', 'org.junit.platform:junit-platform-launcher')
Review Comment:
We can use normal dependency config syntax here.
##########
grails-geb/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy:
##########
@@ -108,6 +110,7 @@ class WebDriverContainerHolder {
withEnv('SE_ENABLE_TRACING', grailsGebSettings.tracingEnabled)
withAccessToHost(true)
start()
+ withImagePullPolicy(PullPolicy.ageBased(Duration.of(1,
ChronoUnit.DAYS)))
Review Comment:
Move before `start()`
##########
gradle/grails-data-tck-config.gradle:
##########
@@ -65,9 +66,10 @@ tasks.withType(Test).configureEach {
// There is a known issue with IntelliJ that can cause it's inspection process
to lock up.
// this is a work around to help prevent the scenario that causes the lock up.
-tasks.register('cleanupTckClasses') {
+tasks.register('cleanupTckClasses').configure { Task it ->
Provider<Directory> extractDir =
layout.buildDirectory.dir('extracted-tck-classes')
- doLast {
+ it.doLast {
extractDir.get().asFile.deleteDir()
}
+ it.outputs.upToDateWhen {false}
Review Comment:
Spaces in closure?
##########
grails-test-examples/mongodb/base/build.gradle:
##########
@@ -28,6 +28,10 @@ dependencies {
testImplementation 'org.spockframework:spock-core'
testImplementation
'org.apache.grails.testing:grails-testing-support-mongodb'
+ // This project also has unit tests in it, so we must define the launcher
per
+ //
https://docs.gradle.org/8.3/userguide/upgrading_version_8.html#test_framework_implementation_dependencies
+ add('testRuntimeOnly', 'org.junit.platform:junit-platform-launcher')
Review Comment:
We can use normal dependency config syntax here.
--
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]
