codeconsole commented on PR #16009:
URL: https://github.com/apache/grails-core/pull/16009#issuecomment-5013854229
Pushed a follow-up commit addressing review feedback on the first commit.
**What was wrong with the first commit:** it fixed the race by calling
`BomPropertyOverridesPlugin.applyOverrides()` directly instead of
`project.plugins.apply(BomPropertyOverridesPlugin)`. That broke the plugin's
identity/idempotency contract -
`project.plugins.findPlugin('org.apache.grails.gradle.bom-property-overrides')`
returned `null` even when the override machinery was active (breaking all 4
`BomPlatformFunctionalSpec` cases), and manually recreating the
`bomPropertyOverrides` extension could collide with the plugin's own extension
creation if a build applied `org.apache.grails.gradle.bom-property-overrides`
explicitly alongside a Grails app/plugin. It also shipped with no automated
test reproducing the actual multi-project race - the existing suite is all
single-project fixtures.
**Two alternative fixes investigated and discarded:**
- Deferring `GrailsCliGradlePlugin`'s classpath probe to
`project.gradle.projectsEvaluated` - throws `IllegalResolutionException`
("attempted without an exclusive lock"); resolving a configuration there isn't
safe.
- Switching `BomManagedVersions.applyTo()` to register constraints via
`Configuration.withDependencies()` instead of eagerly - passes the entire
existing suite, but does **not** actually fix the race: registering
`withDependencies()` on an already-observed configuration is rejected the same
way the eager mutation was, just at a different call site
(`DefaultConfiguration.withDependencies`). Only caught by testing against the
real multi-project reproduction, not the unit suite.
**The actual fix:** keep `project.plugins.apply(BomPropertyOverridesPlugin)`
(restoring identity + idempotency), but call it as a top-level statement in
`applyGrailsBom` instead of nesting it inside the method's own `afterEvaluate`
callback. Since the `grails.bom = null` opt-out can only be determined once
`afterEvaluate` runs (too late to conditionally gate this call without
reintroducing the race), the plugin is now applied unconditionally -
functionally inert when there's no BOM (auto-detection finds nothing to
override), but `findPlugin()` now reports it as applied even when opted out.
`BomOptOutFunctionalSpec` is updated to assert the functional outcome (zero
override constraints) instead, with the tradeoff documented in both the code
and the test for maintainers to weigh in on.
**New regression coverage:** `BomCliMultiprojectRaceFunctionalSpec` - a
two-project fixture (`consumer` depending on `producer` via `project(...)`,
`producer` carrying an active property-based BOM override) that reproduces the
original crash exactly. Confirmed it fails against the pre-fix code with the
same `Cannot mutate the dependencies of configuration ... after ... was
resolved` error, and passes with the fix.
Full `:grails-gradle-plugins:test` suite (75 tests) passes. Re-verified
against the original real-world reproduction (a Grails 8 multi-project app with
a `project(...)` dependency chain and a `grailsVersion` property override) by
publishing to `mavenLocal` and rebuilding.
--
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]
