jdaugherty commented on code in PR #16069:
URL: https://github.com/apache/grails-core/pull/16069#discussion_r3683252757
##########
gradle/functional-test-config.gradle:
##########
@@ -17,49 +17,53 @@
* under the License.
*/
-rootProject.subprojects
+def substitutableProjects = rootProject.subprojects
.findAll { !(it.name in testProjects) && !(it.name in docProjects) &&
!(it.name in cliProjects) }
- .each { project.evaluationDependsOn(it.path) }
+ .collect {
Review Comment:
Optional follow-up, not a blocker. This snapshot is rebuilt independently by
each of the 104 test projects that apply this script, so the `findAll` plus 134
`evaluationDependsOn`/`findProperty` calls all run 104 times over.
Computing it once and sharing it (keeping a cheap per-project
`evaluationDependsOn` loop so evaluation ordering is unchanged) measured a
further **~10% off configuration time, 3.69s → 3.32s**, with identical
resolution output.
For completeness, I also tried precomputing the `"$group:$artifactId"`
coordinate strings into the snapshot so the loop body does no GString work at
all — that was only 3.69s → 3.53s, near noise, so it is not worth doing for
performance alone. The bulk of the win is already in this PR.
##########
gradle/functional-test-config.gradle:
##########
@@ -17,49 +17,53 @@
* under the License.
*/
-rootProject.subprojects
+def substitutableProjects = rootProject.subprojects
.findAll { !(it.name in testProjects) && !(it.name in docProjects) &&
!(it.name in cliProjects) }
- .each { project.evaluationDependsOn(it.path) }
+ .collect {
+ project.evaluationDependsOn(it.path)
+ return [
+ project: it,
+ artifactId: it.findProperty('pomArtifactId') ?: it.name,
+ cliArtifactId: it.findProperty('cliArtifactId')
Review Comment:
This moves the `cliArtifactId` read from lazy (per configuration, when the
substitution rules are registered) to eager (immediately after
`evaluationDependsOn`). That matters because `cliArtifactId` is only set inside
an `afterEvaluate` in `GrailsCliArtifactGradlePlugin`, which
`gradle/cli-companion-bom-constraints.gradle` documents as the reason it uses a
lazy `withDependencies` window.
It is correct as written — `evaluationDependsOn` flushes the target's
`afterEvaluate` before returning, and I confirmed the snapshot matches the late
value for all 134 substitutable projects across all 104 applying test projects
with zero mismatches.
The only thing I would add is a short comment recording that invariant. The
snapshot is now silently order-dependent: if `cliArtifactId` ever moves to a
hook later than `afterEvaluate`, or gets set cross-project, the cli
substitutions would quietly stop being registered rather than fail loudly,
which is an unpleasant failure mode to debug.
--
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]
