jamesfredley commented on PR #15557: URL: https://github.com/apache/grails-core/pull/15557#issuecomment-4320818951
## Update - review feedback addressed + Groovy 5.0.6 workaround sweep This is a summary of changes pushed in this session. ### Review feedback (all 5 of jdaugherty's threads addressed and resolved) | File | Reviewer comment | Resolution | Commit | |---|---|---|---| | `LICENSE` | "Why are we changing the license? If for line endings, separate PR" | Reverted line-ending normalization. LICENSE, gradlew.bat, .gitattributes, and 9 source files restored byte-for-byte to base. | `d280308db5` | | `MongoCodecSession.groovy:176` | "`++` should work, seems like a Groovy bug" | Restored `[name]++` form. Drive-by fix: the workaround had a copy-paste typo on line 179 (read `numberOfOptimisticUpdates[name]` while writing `numberOfPessimisticUpdates[name]`), silently miscounting pessimistic updates. | `4040590fd6` | | `NavigableMap.groovy:152` | "performance regression from ConfigObject->Map deep conversion" | Made `convertConfigObjectToMap` shallow + lazy. Nested ConfigObjects are converted on demand by `mergeMapEntry` only when the recursive merge actually descends into them; subtrees filtered out by spring-profile guards are no longer copied. Dropped `@CompileDynamic`. | `edb40f2628` | | `HibernateEntity.groovy:40` | "I thought we were going to open a groovy ticket on this" | Filed as [GROOVY-11907](https://issues.apache.org/jira/browse/GROOVY-11907) by Paul King; resolved in 5.0.6. Restored static `findAllWithSql` / `findWithSql` on the trait and switched the regression specs back to the public trait API. | `8af5d1dc4c` | | `groovy-joint-workflow.yml:144` | "Can't this be done with a gradle init-script instead?" | New `.github/scripts/groovy-joint-build.init.gradle` applied via `--init-script`. Uses `settingsEvaluated` + `pluginManager.withPlugin('com.gradle.develocity')` (defensive guard - no-op if Groovy ever drops the plugin). Removes 5 workflow steps + the awk block. Drive-by fix: corrected `'GRAILS_DEVELOCITY_ACCESS_KEY '` (trailing space) typo from the deleted `develocity-conf-2` step that made `isAuthenticated` always false. | `558e132737` | ### Groovy 5.0.6-SNAPSHOT workaround sweep Walked every Groovy 5 workaround in the PR and tested whether the upstream fix has landed. **Removed (workaround no longer needed)** - 4 commits removing duct tape: - `8af5d1dc4c` - **HibernateEntity** static SQL methods restored to trait (GROOVY-11907 fixed in 5.0.6) - `a71c8b5ebb` - **ClassPropertyFetcherTests** generic trait property coverage restored (Groovy 5 generic trait fix landed) - `2bb0930a5d` - **JspTagImpl** `@CompileDynamic` workaround removed (Groovy 5 union-type instanceof fix landed) - `a290b37156` (partial - CommandLineHelper part only) - **CommandLineHelper** `SUCCESS` / `FAILURE` static finals restored (GROOVY-11907) **Kept after attempted removal** (verified the workaround is still needed): - `b8ee60d460` - **ContainerGebConfiguration** stays a trait. Initial attempt to revert to interface w/ default methods (`b0c7f347e8`) compiled cleanly but produces a runtime `VerifyError: get long/double overflows locals` for downstream consumers compiled with `-PgrailsIndy=false`. The interface `$getCallSiteArray()` regression is still real on Groovy 5.0.6 indy=false bytecode generation. - `0804a4fc73` - **ContainerSupport** stays `@CompileDynamic`. The GROOVY-11907 fix is incomplete for indy=false: `@CompileStatic` on a trait with static fields still generates invalid bytecode for the static setter helpers (`Trait$Helper.setContainer @0: dload_3` with only 2 locals). Reproduced locally with `./gradlew :grails-test-examples-app2:integrationTest -PgrailsIndy=false`. Documented at the file site for upstream follow-up. Should be filed as a GROOVY-11907 follow-up ticket. **Kept (verified still required)** - the items left in the inventory: - Group B (5 files - VariableScopeVisitor NPE during AST canonicalization) - reverting reproduced the canonicalization NPE - Group C (`NavigableMap`, `GroovyConfigPropertySourceLoader`) - reverting reproduced `StackOverflowError` in `ConfigMapSpec` - Group F (`JsonViewTemplateResolverSpec` skip annotations) - Spock can't mock final `GrailsWebRequest.getRequest()` - Group G (`GrailsWebDataBinderSpec` manual `Comparable`) - reverting reproduced canonicalization NPE - Group I (`AbstractConstraint`, `BsonPersistentEntityCodec`, `Validateable`, `ConfigurationBuilder`) - real fallbacks, not removable - Group D (`GrailsApplicationCommand`, `TemplateRendererImpl`, `GenerateControllerCommand`) - flagged for human review; verification cycle exceeds the time budget for this sweep (long Forge/TestKit cycle) ### Local verification The CI matrix `-PgrailsIndy=false` jobs (Build Grails Forge, Functional Tests, Mongodb Functional Tests, Hibernate5 Functional Tests) were initially broken by `b0c7f347e8` (`VerifyError` on `ContainerGebSpec.<clinit>`) and recovered after `b8ee60d460` + `0804a4fc73`. Verified locally on Groovy 5.0.6-SNAPSHOT + JDK 21: | Local command | Result | |---|---| | `./gradlew :grails-test-examples-app2:integrationTest -PgrailsIndy=false --rerun-tasks` | BUILD SUCCESSFUL (3m 6s, 2 specs PASSED) | | `./gradlew :grails-test-examples-hibernate5-grails-hibernate:integrationTest -PgrailsIndy=false --rerun-tasks` | BUILD SUCCESSFUL (3m 12s, 6 specs PASSED) | | `./gradlew :grails-data-hibernate5-core:test --tests "grails.gorm.tests.SqlQuerySpec" --tests "grails.gorm.tests.HibernateEntityTraitGeneratedSpec" -PgrailsIndy=false` | BUILD SUCCESSFUL (1m 50s, 7 specs PASSED) | | `./gradlew :grails-gsp:test --tests "org.grails.gsp.jsp.SimpleTagTests" --tests "org.grails.gsp.jsp.IterativeJspTagTests"` | BUILD SUCCESSFUL (2m 34s, 3 specs PASSED) | | `./gradlew :grails-datastore-core:test --tests ClassPropertyFetcherTests` | BUILD SUCCESSFUL | | `./gradlew :grails-bootstrap:test --tests grails.config.ConfigMapSpec` (after NavigableMap optimization) | BUILD SUCCESSFUL (12 specs PASSED) | | `./gradlew :grails-data-mongodb-core:compileGroovy` (after `++` restoration) | BUILD SUCCESSFUL | | `./gradlew help --init-script .github/scripts/groovy-joint-build.init.gradle` | BUILD SUCCESSFUL with build scan published to develocity.apache.org | PR description has been revised to reflect the current state - workarounds that the Groovy 5.0.6 release has actually fixed are no longer listed. -- 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]
