matrei commented on PR #16327:
URL: https://github.com/apache/grails-core/pull/16327#issuecomment-5596791466
# AI Review Findings
Head `3973ca341a`, branched from `e8d33a9318`, before #16325 landed on
`8.0.x`. The two PRs are complementary and do not overlap: #16325 pins that the
generator declares no classpath input, which was the failure mode behind the
2025 `doFirst` revert, and this PR covers the other half of the same concern, a
producer that had only to precede `compileGroovy` and now also has to precede
the generator. I merged this head with `60515a5ec8` locally: the spec
auto-merges, and the combined 13 cases pass with code style clean. No rebase is
needed.
The execution fixture is a real improvement over the dry-run check it
replaces: `bom = null`, `cliAutoProvision = false` and `localGroovy()` let the
fixture compile and run offline, and the test now proves the generated script
reaches the compiler and follows changes. The third case, an extra compile
prerequisite that itself depends on script preparation, is a good guard against
a future "fix" that copies `compileGroovy`'s dependencies onto the generator.
The finding below is about the choice this PR makes for the pattern it
exposes.
## [P2] The plain-file pattern can be supported by the plugin instead of
documented as a migration
**Files:**
-
`grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy:381-386`
-
`grails-gradle/plugins/src/test/groovy/org/grails/gradle/plugin/core/GrailsGroovyCompilerConfigSpec.groovy:153-176`
- `grails-doc/src/en/guide/upgrading/upgrading80x.adoc:3101-3165`
This wiring worked on Grails 7 and with the `doFirst` on 8.0.x before #16114:
```groovy
tasks.named('compileGroovy', GroovyCompile) {
dependsOn generateUserConfigScript
groovyOptions.configurationScript = file('build/user-config.groovy')
}
```
It now fails, and not only in the forced order the PR description and the
test use. On this head, a plain `verifyCompilation -PplainFile` from a clean
checkout fails too: Gradle schedules
`generateCompileGroovyGrailsCompilerConfig` before `generateUserConfigScript`
(the plugin's `dependsOn` is registered before the build script's), and the
generator's `@InputFile` validation fails on the missing file. This is the
concrete form of the concern raised in
https://github.com/apache/grails-core/pull/16114#issuecomment-5516571268: a
task that only had to precede `compileGroovy` now also has to precede the
intermediate task. The PR documents that as a required migration. I think the
plugin can absorb it.
The generator already resolves the compile task in its `dependsOn` closure.
When the configured script is a plain file, it can look for the producer among
`compileGroovy`'s own direct dependencies by matching outputs:
```groovy
t.dependsOn({
GroovyCompile compile = project.tasks.named(compileTaskName,
GroovyCompile).get()
RegularFileProperty configured =
compile.groovyOptions.configurationScriptFile
if (!configured.present) {
return []
}
List<Object> deps = [project.files(configured)]
// A plain file assignment carries no producer. When one of the compile
task's own
// dependencies writes that file, order the generator after it.
File file = configured.asFile.get()
deps.addAll(compile.taskDependencies.getDependencies(compile).findAll {
Task d ->
d != t && d.outputs.files.contains(file)
})
deps
} as Callable)
```
I ran the PR's spec with that change in place. All four plain-file
invocations succeed, with `generateUserConfigScript` scheduled ahead of the
generator each time:
| Invocation | Result |
|---|---|
| `verifyCompilation -PplainFile` | `CONFIGURED_TYPE=java.nio.file.Path` |
| `verifyCompilation -PplainFile -PimportedType=java.net.URI` |
`CONFIGURED_TYPE=java.net.URI` |
| `generateCompileGroovyGrailsCompilerConfig verifyCompilation -PplainFile`
| success (the order this PR uses to force the failure) |
| `verifyCompilation -PplainFile -PadditionalDependency` |
`ADDITIONAL_COMPILE_DEPENDENCY=ran`, no cycle |
The only test that fails is *a plain script file plus compile dependency
needs producer provider wiring*, because the build it expects to fail now
succeeds. The lookup is exact, so it cannot form a cycle unless the matched
producer already depends on `compileGroovy`, which was broken under the
`doFirst` too. `prepareCompilation` in the third case is not matched because it
writes no file.
Suggested shape for this PR:
1. Add the producer lookup to `GrailsGradlePlugin`.
2. Flip the plain-file test to assert success under the natural invocation,
and keep the forced order as a second `when`.
3. Rewrite section 54 as a recommendation: provider wiring is the idiomatic
form and carries the dependency on its own, while the plain file plus
`dependsOn` form keeps working. Drop "replace this wiring".
If the migration route is kept instead, section 54 should say the build
*does* fail from clean rather than "can", and be marked as a breaking change
for builds coming from Grails 7, since the pattern was valid there.
## Verification
- `./gradlew :grails-gradle-plugins:test --tests
'org.grails.gradle.plugin.core.GrailsGroovyCompilerConfigSpec' --no-daemon`: 12
tests, 0 failures on the PR head.
- Same spec and `codeStyle` after merging the PR head with `origin/8.0.x`
(`60515a5ec8`) in a scratch worktree: 13 tests, 0 failures.
- `./gradlew :grails-gradle-plugins:codeStyle --no-daemon`: passed.
- Natural-order probe on the PR head (`verifyCompilation -PplainFile`, fresh
project dir): fails with `property 'configurationScript' specifies file
'.../build/user-config.groovy' which doesn't exist`; only `:compileJava` and
`:generateCompileGroovyGrailsCompilerConfig` ran.
- Same probe plus the three invocations in the table with the producer
lookup applied: all succeed. Plugin change reverted afterwards; nothing from it
is in the working tree.
--
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]
