daniellansun commented on PR #2544: URL: https://github.com/apache/groovy/pull/2544#issuecomment-4581127366
> Claude's comments: > > ## Review of PR #2544 ([GROOVY-12019](https://issues.apache.org/jira/browse/GROOVY-12019): Enable Gradle configuration cache) > Solid build-infra work. CI is green, BND migration is clean, and most of the refactors carry inline comments explaining the CC constraint they exist to satisfy. A few items worth a second look: > > 1. **PR description.** I've added a summary to the Jira issue ([[GROOVY-12019](https://issues.apache.org/jira/browse/GROOVY-12019)|https://issues.apache.org/jira/browse/[GROOVY-12019](https://issues.apache.org/jira/browse/GROOVY-12019)]) capturing the motivation, the asciidoctor workaround, and the OSGi-DSL → BND migration. Could you check the contents and adjust anything I got wrong? > 2. **`SharedConfiguration.isDocumentationTask` misses Gradle abbreviations.** Verified that `gradle.startParameter.taskNames` holds request strings verbatim — Gradle's task selector expands abbreviations later, after configuration. So: > > > > User typed > `taskNames` > Resolved to > > > > > `prop` > `[prop]` > `:properties` > > > `dI` > `[dI, …]` > `:dependencyInsight` > > > > Consequence: `./gradlew jA` (abbreviating `javadocAll`) leaves `taskNames = [jA]` → `isDocumentationBuild = false` → asciidoctor plugin is not applied → later `tasks.named('asciidoctor')` in `groovy-distribution.gradle` fails with "task not found". This is a regression vs. master, where asciidoctor is always applied. Options: drop the gating heuristic (accept CC-warn cost for everyone), document that abbreviations aren't supported for doc builds, or match camelCase prefixes of the known doc task names. > (The "unrelated user task ending in 'doc' / starting with 'dist'" angle I worried about initially is weaker — those would be self-inflicted from the user's CLI, not a real failure mode. Abbreviation is the real one.) > 3. **`gradle.startParameter.warningMode = WarningMode.None` in `groovy-base.gradle`** silences _all_ Gradle warnings during documentation builds, not just the asciidoctor 4.0.5 deprecation. That hides legitimate Gradle 9 deprecations across the rest of the build. Consider scoping narrower (or filtering the specific known message) and adding a TODO to revert once asciidoctor 4.0.6+ ships. > 4. **`bndInstruction(k, v)` accumulates with comma-separators.** Correct for `Require-Capability`/`Provide-Capability`, but for single-valued headers a second call from a sub-plugin produces `"old,new"` instead of overriding. The defaults inside `JarJarTask` use `analyzer.setProperty` (replace), but user-side calls accumulate — that asymmetry will bite someone eventually. Worth a doc comment on `bndInstruction()` calling this out, or a separate `setBndInstruction()` for the override case. > 5. **`Main-Class: groovy.ui.GroovyMain` added to every core jarjar manifest.** The old `withManifest` block in `groovy-core.gradle` didn't set it. Tests pass, but worth confirming this isn't an unintended new header in the published `groovy-N.N.N.jar` manifest — easy to cross-check against the previous release's MANIFEST.MF. > 6. **`Signing.shouldSign()` semantics change.** Old code inspected the resolved task graph (`taskGraph.hasTask(':artifactoryPublish')`), which catches publish tasks reached via `dependsOn`. New code only inspects requested task names. If anything internally depends on `:artifactoryPublish` without naming it on the CLI, signing now silently skips. Probably no such caller exists, but the test suite doesn't exercise signing, so worth a manual sanity check before merging. > > None of these are blocking — (2) is the only one I'd want addressed before merge. @paulk-asert (2) has been addressed, please review it again. -- 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]
