[
https://issues.apache.org/jira/browse/GROOVY-12019?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18084483#comment-18084483
]
ASF GitHub Bot commented on GROOVY-12019:
-----------------------------------------
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.
> Enable gradle configuration cache
> ---------------------------------
>
> Key: GROOVY-12019
> URL: https://issues.apache.org/jira/browse/GROOVY-12019
> Project: Groovy
> Issue Type: Improvement
> Components: build
> Reporter: Daniel Sun
> Priority: Major
>
> h2. Summary
> Enables Gradle's configuration cache by default (with CC problems downgraded
> to warnings) and refactors the build to be CC-safe. Targets
> [GROOVY-12019|https://issues.apache.org/jira/browse/GROOVY-12019].
> h2. Motivation
> Gradle's configuration cache (CC) skips the configuration phase on repeat
> builds, giving a substantial speed-up — especially on incremental
> compile/test cycles. The Groovy build currently breaks CC in several ways:
> * {{gradle.taskGraph.whenReady}} hooks (CC cannot serialize the task graph).
> * Direct {{System.properties\[...\]}} reads (CC cannot track invalidation).
> * Execution-time access to {{Project}} / {{configurations}} from task actions.
> * Gradle's legacy OSGi DSL ({{osgi.osgiManifest \{\}}}) captures
> non-serializable state.
> * The asciidoctor-gradle-jvm 4.0.5 plugin captures dependency-management
> state in its extension.
> This PR removes all of those for the common compile/test paths and gates the
> still-incompatible doc plugins behind opt-in flags so they only apply when
> documentation tasks are requested.
> h2. Changes
> h3. Build configuration
> * {{gradle.properties}}: {{org.gradle.configuration-cache=true}},
> {{org.gradle.configuration-cache.problems=warn}}.
> * Replace {{gradle.taskGraph.whenReady}} usages with
> {{startParameter.taskNames}} introspection ({{SharedConfiguration}}, root
> {{build.gradle}}, {{org.apache.groovy-published-library.gradle}},
> {{org.apache.groovy-tested.gradle}}).
> * Replace {{System.properties\[k\]}} with {{providers.systemProperty(k)}} /
> {{providers.systemPropertiesPrefixedBy(...)}} so CC invalidation tracks
> {{-D}} flag changes.
> * Capture {{project.version}}, {{configurations.xxx}}, and other Project
> state at configuration time rather than inside {{doFirst}}/{{doLast}}.
> * {{PerformanceTestsExtension}}: model JavaExec args as a
> {{CommandLineArgumentProvider}} with tracked inputs.
> h3. OSGi manifest generation ({{JarJarTask}})
> Migrated from Gradle's legacy OSGi DSL to BND {{Analyzer}} directly:
> * Adds {{biz.aQute.bnd:biz.aQute.bndlib:6.4.1}}.
> * New {{bndInstruction(key, value)}} API on {{JarJarTask}}; multiple calls
> for the same key accumulate with comma-separators (correct BND syntax for
> multi-value headers such as {{Require-Capability}}).
> * New {{bndClasspath}} input feeds the analyzer for {{Import-Package}}
> resolution.
> * {{-removeheaders}} strips BND housekeeping headers ({{Bnd-LastModified}},
> {{Tool}}, {{Created-By}}, ...) so released jars don't pick up new manifest
> entries.
> * Updated call-sites: {{org.apache.groovy-base.gradle}},
> {{org.apache.groovy-core.gradle}}, {{org.apache.groovy-library.gradle}}.
> h3. Documentation plugins (still not CC-compatible upstream)
> * {{org.apache.groovy-asciidoctor}} and {{org.asciidoctor.jvm.pdf}} are now
> applied *only* when the requested tasks look like a doc/dist build *and*
> {{src/spec/doc}} exists.
> * Asciidoctor tasks are marked {{notCompatibleWithConfigurationCache(...)}}
> so the CC fail-on-store path doesn't trip when docs are built.
> * New {{asciidocElements}} consumable variant on the base plugin means
> non-doc consumers still wire correctly (empty artifact when docs aren't
> built).
> h3. Signing
> * {{Signing.shouldSign()}} no longer takes a {{TaskExecutionGraph}}; instead
> {{SharedConfiguration}} eagerly inspects {{startParameter.taskNames}} for
> {{artifactoryPublish}} / {{publishAllPublicationsToApacheRepository}}.
> h3. Minor
> * {{me.champeau.jmh:jmh-gradle-plugin}} 0.7.2 -> 0.7.3.
> * {{GroovydocAntPlugin}}: replaces {{project.ant}} with a fresh
> {{AntBuilder}}, models source dirs as task inputs.
> * {{verification-metadata.xml}}: updates for the bumped/added build
> dependencies.
> h2. Test plan
> * Full test suite green (71,219 tests, all 56 checks).
> * SonarQube quality gate: 0 new issues.
> * Codecov: no regressions (+0.005%).
> h2. Notes for reviewers
> * {{org.gradle.configuration-cache.problems=warn}} is intentional — the
> documentation plugins still emit CC problems, so {{fail}} would break
> dist/publish builds. Compile/test builds have zero CC problems and benefit
> from the cache as normal.
> * Several inline comments explain *why* a particular refactor was needed
> (search for "configuration cache" in the diff) so the constraints survive
> future edits.
> * The {{notCompatibleWithConfigurationCache}} call in
> {{org.apache.groovy-asciidoctor.gradle}} should be removed once
> asciidoctor-gradle-jvm ships a CC-compatible release.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
