Preserve generated Groovy compiler script producer dependencies - #16327
Preserve generated Groovy compiler script producer dependencies#16327codeconsole wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16327 +/- ##
==================================================
- Coverage 55.0810% 55.0790% -0.0019%
- Complexity 20804 20810 +6
==================================================
Files 2111 2111
Lines 101378 101387 +9
Branches 18005 18007 +2
==================================================
+ Hits 55840 55843 +3
- Misses 37490 37499 +9
+ Partials 8048 8045 -3
🚀 New features to boost your workflow:
|
AI Review FindingsHead The execution fixture is a real improvement over the dry-run check it replaces: 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 migrationFiles:
This wiring worked on Grails 7 and with the 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 The generator already resolves the compile task in its 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
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 Suggested shape for this PR:
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
|
| implementation localGroovy() | ||
| } | ||
|
|
||
| @DisableCachingByDefault(because = 'Writing a short compiler script is inexpensive') |
There was a problem hiding this comment.
I think it's important to test cached based scripts since the original regression revolved around that. Can we keep the existing test and add yours as new ones?
There was a problem hiding this comment.
Restored the original producer-order test and fixture in ea9c585; the execution tests now use a separate fixture. Added build-cache tests for both wiring forms that assert FROM_CACHE after cleaning, execute compilation against the restored script, and verify input changes invalidate the cache. All 275 plugin tests and style checks pass.
|
The pull request already replaces the previous test with new, more robust tests that verify the producer-consumer relationship using provider wiring. The new tests cover clean builds, incremental builds (up-to-date checks), and change detection, which directly address the regression concerns regarding cached-based scripts. Keeping the old test is unnecessary as the new implementation provides better coverage for the fix. grails-gradle/plugins/src/test/groovy/org/grails/gradle/plugin/core/GrailsGroovyCompilerConfigSpec.groovy |
|
@matrei Agreed — matching the exact declared output preserves the old wiring without inheriting every compilation dependency. Implemented in 2d7fdf0. The lookup runs only when the configured script carries no producer metadata. It matches the file against direct compile dependencies, excludes the generator itself, and adds only matching producers. Provider-wired scripts keep their existing handling. The tests now assert success for the plain-file form in natural and explicitly reordered invocations. Both wiring forms cover clean builds, unchanged builds, changed script content, configuration-cache reuse after cleaning, and an unrelated prerequisite that declares a different output file and depends on script preparation. I also included the merged #16325 cycle guard. Three plain-file cases failed before the fix and now pass. The guide recommends provider wiring while documenting that the plain-file-plus-dependsOn form remains supported when the producer declares the exact script file as an output. The new section is numbered 58 to follow the guide additions now on 8.0.x. Verification: all 272 plugin tests passed, including 17 compiler-configuration cases; plugin style and dependency validation passed. The full root clean/violations/aggregate-test check passed (18,404 tests reported, zero failures/errors, 447 skipped), all four violation reports are clean, and the guide builds successfully. |
# Conflicts: # grails-doc/src/en/guide/upgrading/upgrading80x.adoc
…dation error The plain-file lookup matches outputs.files however the producer declares them, but the execution fixture only used a typed @OutputFile task. Add an -PadHocProducer variant that declares the script with outputs.file, the shape most Grails 7 builds used, and run the follows-changes and producer-order cases against it. Quote the generateCompileGroovyGrailsCompilerConfig validation error in the upgrade guide so a build whose producer cannot be matched leads to the provider wiring.
|
@matrei I believe I addressed all of our feedback. can you please confirm so we can merge this? |
✅ All tests passed ✅🏷️ Commit: 6cfdb52 Learn more about TestLens at testlens.app/docs. |
matrei
left a comment
There was a problem hiding this comment.
Review Findings (round 2)
Head 6cfdb523d9, merged with origin/8.0.x at e9cd00567b; nothing on the base branch is missing from this head.
The P2 from the previous round is resolved. The generator now looks for a producer among compileGroovy's direct dependencies when the configured script carries no producer of its own, matched on the exact output file, and skips the lookup when the property already carries one. The plain-file test was flipped to assert success under the natural invocation, with the forced generateCompileGroovyGrailsCompilerConfig verifyCompilation order kept as a second when. The new cases for an ad-hoc outputs.file producer, the additional compile dependency that itself depends on the generator, the configuration cache and the build cache are all good guards for this wiring, and the fixture stays offline with bom = null, cliAutoProvision = false and localGroovy().
Section 60 of the upgrade guide now reads as a recommendation with the plain-file form kept working, and it is accurate about the limits: I checked the directory-only case it describes. A producer that declares outputs.dir('build/cfg') and writes build/cfg/user-config.groovy with configurationScript = file('build/cfg/user-config.groovy') fails from clean with the quoted Input file does not exist validation error on the generator, with only compileJava and the generator having run. The renumbering of the two preceding sections fixes a duplicate 54. left by the merge.
[P3] The ad-hoc producer's provider wiring in the fixture has no test row
File: grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/build.gradle:112-113
} else if (providers.gradleProperty('adHocProducer').isPresent()) {
groovyOptions.configurationScriptFile.fileProvider(genUserScript.map { it.outputs.files.singleFile })
}Every row that passes -PadHocProducer also passes -PplainFile, so the plainFile branch wins and this branch never runs. It does work: clean verifyCompilation -PadHocProducer on this head runs generateUserConfigScript and compileGroovy with CONFIGURED_TYPE=java.nio.file.Path. Either add a row to a generated user script is consumed during compilation so the branch is covered:
'ad-hoc provider' | ['-PadHocProducer']or drop the branch and keep the ad-hoc task for the plain-file rows only. While there, the where: table in that test has one row out of alignment with the others.
Note, no action needed
When the configured script carries no producer, which includes a checked-in script, the lookup resolves outputs.files on each direct compileGroovy dependency while the graph is built. Gradle resolves those outputs before running the tasks anyway, and the existing checked-in-script cases (compiler-config-late-assignment, compiler-config-missing-user-script) still pass, so I do not see a problem with it. Mentioning it only so the cost is a known one.
Verification
./gradlew :grails-gradle-plugins:test --tests 'org.grails.gradle.plugin.core.GrailsGroovyCompilerConfigSpec' :grails-gradle-plugins:codeStyle --no-daemonfromgrails-gradle/: 22 tests, 0 failures, code style clean.- Temporary probe spec (removed afterwards, nothing left in the working tree):
clean verifyCompilation -PadHocProduceron the execution fixture: success,CONFIGURED_TYPE=java.nio.file.Path.- Directory-only producer fixture,
verifyCompilationfrom clean: fails withInput file does not exist ... doesn't existongenerateCompileGroovyGrailsCompilerConfig, matching the guide.
A build that assigns its generated Groovy compiler script as a plain file and separately adds the producer to
compileGroovy.dependsOnfails after #16114: Grails prepares the combined script before the user script exists. This change preserves that previously working form when the producer declares the exact script file as an output.When the script has no producer metadata, the plugin matches it against the declared outputs of the compile task's direct dependencies and adds only the matching producer to script preparation. Provider-wired scripts retain their existing dependency handling. The generator does not inherit unrelated compilation dependencies or consume the compile/runtime classpath.
The original producer-order dry-run test and its fixture are retained. The new execution tests use a separate fixture and compile and run source requiring both Grails imports and the generated script. They cover both wiring forms through clean builds, unchanged builds, changed script content, configuration-cache reuse after cleaning, and an unrelated compile prerequisite with a different output that must not introduce a cycle. The plain-file case also succeeds when script preparation is explicitly requested first. Three plain-file cases failed before the plugin fix and pass with it.
Build-cache tests cover both wiring forms with an isolated cache: after cleaning, the producer must report
FROM_CACHEwhile compilation actually executes against the restored script. Changing the producer input must miss the cache, and reverting it must restore the original cached script.The Grails 8 upgrade guide now recommends producer-provider wiring and explains the supported plain-file form, including the requirement to declare the exact output file. It no longer presents a mandatory migration for that supported form.
Follow-up to #16114 and @matrei's review. Includes the merged #16325 runtime-classpath cycle guard for verification alongside this fix.
Validation:
grails-gradle/,./gradlew :grails-gradle-plugins:test :grails-gradle-plugins:codeStyle --offline --no-daemon --max-workers=3 -PmaxTestParallel=3— passed; 275 tests, no failures or skips, including 20 compiler-configuration cases.git diff --check— passed.