Skip to content

Preserve generated Groovy compiler script producer dependencies - #16327

Open
codeconsole wants to merge 6 commits into
apache:8.0.xfrom
codeconsole:test/compiler-script-producer-wiring
Open

Preserve generated Groovy compiler script producer dependencies#16327
codeconsole wants to merge 6 commits into
apache:8.0.xfrom
codeconsole:test/compiler-script-producer-wiring

Conversation

@codeconsole

@codeconsole codeconsole commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

A build that assigns its generated Groovy compiler script as a plain file and separately adds the producer to compileGroovy.dependsOn fails 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_CACHE while 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:

  • Latest commit: from 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.
  • Before the latest upstream merge and test additions: dependency-version validation, the guide build, and the full root check passed (18,404 tests, zero failures/errors, 447 skipped; all four violation reports clean).
  • Full validation of the updated branch is left to GitHub CI.

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.0790%. Comparing base (e9cd005) to head (6cfdb52).

Files with missing lines Patch % Lines
...rails/gradle/plugin/core/GrailsGradlePlugin.groovy 0.0000% 11 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                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     
Files with missing lines Coverage Δ
...rails/gradle/plugin/core/GrailsGradlePlugin.groovy 0.0000% <0.0000%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@matrei

matrei commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

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:

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 #16114 (comment): 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:

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.

implementation localGroovy()
}

@DisableCachingByDefault(because = 'Writing a short compiler script is inexpensive')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@codeconsole codeconsole Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bito-code-review

Copy link
Copy Markdown

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

def "a generated user script is consumed during compilation and follows changes"() {
        given: 'a compiler script wired through its producer output provider'
        setupTestResourceProject('compiler-config-generated-user-script')

        when: 'a clean build compiles and runs source that needs both sets of imports'
        def first = executeTask('clean', ['verifyCompilation'])

        then:
        first.task(':generateUserConfigScript').outcome == TaskOutcome.SUCCESS
        first.task(':generateCompileGroovyGrailsCompilerConfig').outcome == TaskOutcome.SUCCESS
        first.task(':compileGroovy').outcome == TaskOutcome.SUCCESS

@codeconsole codeconsole changed the title Test and document generated compiler script producer wiring Preserve generated Groovy compiler script producer dependencies Sep 9, 2026
@codeconsole

Copy link
Copy Markdown
Contributor Author

@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.
@jdaugherty

Copy link
Copy Markdown
Contributor

@matrei I believe I addressed all of our feedback. can you please confirm so we can merge this?

@testlens-app

testlens-app Bot commented Sep 11, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 6cfdb52
▶️ Tests: 73105 executed
⚪️ Checks: 89/89 completed


Learn more about TestLens at testlens.app/docs.

@matrei matrei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-daemon from grails-gradle/: 22 tests, 0 failures, code style clean.
  • Temporary probe spec (removed afterwards, nothing left in the working tree):
    • clean verifyCompilation -PadHocProducer on the execution fixture: success, CONFIGURED_TYPE=java.nio.file.Path.
    • Directory-only producer fixture, verifyCompilation from clean: fails with Input file does not exist ... doesn't exist on generateCompileGroovyGrailsCompilerConfig, matching the guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants