jamesfredley commented on PR #15557:
URL: https://github.com/apache/grails-core/pull/15557#issuecomment-4316529796
## Recent changes (this push)
Four commits pushed on top of `8c5d1d182d` to get the PR's remaining CI
failures unblocked. In diagnostic order:
### 1. `fe05ce8f3b` - CI workflow: unbrittle `groovy-joint-workflow`
The `build_groovy` job was failing in 30s because the "Comment `include
'groovy-all'`" step used a hardcoded `sed -i '72c\\ ... '` against Apache
Groovy's `settings.gradle`. That line number was correct for `GROOVY_4_0_X` at
the time of authoring but is wrong for `GROOVY_5_0_X` (the line moved). The sed
command then silently rewrote the wrong line, breaking the downstream
`./gradlew install` of Groovy itself, and everything else in that workflow
failed as a consequence.
Rewritten as a pattern-based `awk` transform that:
- Finds any line containing `include 'groovy-all'` (quoted or unquoted, with
or without leading whitespace) and comments it out in place.
- Guards with an explicit check: if the pattern is never matched, the step
fails loudly so that a future rename in Apache Groovy does not silently no-op
the joint build again.
- Was verified locally against both the `GROOVY_4_0_X` and `GROOVY_5_0_X`
checkouts of `apache/groovy` to confirm the replacement is idempotent and
correctly targets the one line.
### 2. `ba6c488b92` - build: bump Apache Groovy to `5.0.6-SNAPSHOT`
Moved `'groovy.version'` in `dependencies.gradle` from `5.0.5` to
`5.0.6-SNAPSHOT` so the PR can pull in three upstream fixes that landed on
`GROOVY_5_0_X` after the 5.0.5 tag:
- **GROOVY-11907** (`7bd24933c1`, "trait field reference transform
restructure") - the ticket was opened from this migration. Its fix reworks how
trait field-reference helpers generate getter/setter bytecode; eventually
should let us revert the PR's `CommandLineHelper` static-fields workaround
(commit `d2441fbcb5`).
- **GROOVY-11899** (`e5647650c5`, "Static methods in Traits do not compile
in Java/Groovy joint compilation") - relevant to Grails' joint stub generation
path.
- **GROOVY-11930** (`1497cf5adf`, "unwrap null into argument array for
reference type") - fixes `ClassCastException` in indy dispatch when
named-argument maps carry null values.
The Apache snapshots repo and the `org.apache.groovy.*-SNAPSHOT` content
filter were already wired in root `settings.gradle` (line 504),
`grails-forge/settings.gradle`, and in every Forge-generated app via
`GradleRepository.getDefaultRepositories("*-SNAPSHOT")`, so the snapshot
resolves with no further build-script changes. Confirmed by running the failing
Forge test against 5.0.6-SNAPSHOT and observing the banner print `Groovy:
5.0.6-SNAPSHOT`.
**Caveat**: the snapshot bump alone did not fix the scaffolding test. The
actual fix is in commit 3 below. The bump is kept because its upstream fixes
are directly relevant to this migration and let us reduce the number of
downstream workarounds over time; it can be reverted to `5.0.5` independently
if reviewers prefer to stay on a tagged release.
### 3. `325e2fee08` - fix: replace File truthy-check with explicit null
checks in `TemplateRenderer`
**Root cause of the scaffolding silent no-op** (`ScaffoldingSpec.test
generate-controller command`). Three prior attempts mis-diagnosed this as a
`@Delegate` / trait dispatch regression. The actual bug:
`TemplateRendererImpl.render(Map)` and `render(Resource, File, Map,
boolean)` both guarded with `if (template && destination) { ... }`. Groovy
applies `DefaultGroovyMethods.asBoolean(File)` to that coercion, which is:
```java
public static boolean asBoolean(File file) {
return file.exists() && (file.isDirectory() || file.length() > 0);
}
```
For a newly-generated scaffolding file whose destination does not yet exist,
`asBoolean(File)` returns `false`. The guard evaluates `true && false = false`,
the method silently returns, and the command exits 0 with no files written and
nothing on `System.err` - which is exactly what CI was reporting.
Why this only surfaces on Groovy 5: Groovy 4's `@CompileStatic` compiled `if
(destination)` using a direct reference-null check at bytecode level, so
`File.asBoolean` was never invoked. Groovy 5's `@CompileStatic` invokes the
full DGM dispatch, which correctly honours `asBoolean(File)`. The bug has
therefore been latent in this code since Groovy added the `File.asBoolean` DGM,
but only surfaced now.
**The fix**:
- `TemplateRendererImpl.render(Map<String, Object>)`: use
`Map.get('template')` / `Map.get('destination')` instead of dynamic property
access (the dynamic-property path also reached `!File.asBoolean` through the
`!map?.destination` coercion); throw `IllegalArgumentException` with a clear
message when the map is non-null but carries a null `template` or `destination`
entry (was: silent return); preserve `Resource` and `File` inputs instead of
re-routing them back through `template(Object)` / `file(Object)`, which under
Groovy 5 `@CompileStatic` dispatch we now know we cannot rely on to be a no-op
for already-normalised inputs.
- `TemplateRendererImpl.render(Resource, File, Map, boolean)`: replace `if
(template && destination) { ... }` with explicit `if (template == null)` and
`if (destination == null)` checks; throw `TemplateException` /
`IllegalArgumentException` on null inputs (was: silent no-op); flatten the
nested else chain with early returns.
- `TemplateRendererImpl.render(CharSequence, File, Map, boolean)` and
`render(File, File, Map, boolean)`: same `template && destination` trap
replaced with explicit null checks. These overloads preserve the old
silent-return-on-null behaviour (they are less critical), but they no longer
trip over `File.asBoolean`.
- `GenerateControllerCommand`: extracted a private `generateFile(Resource,
Model, String, String, boolean)` helper that calls the explicit
`templateRenderer.render(Resource, File, Model, boolean)` overload directly and
asserts non-null template/destination before dispatch. This is defence-in-depth
- it bypasses the named-arg -> `render(Map)` -> `@Delegate`-bridge path
entirely, so any future dispatch regression in that chain cannot silently break
scaffolding again, and surfaces failures loudly instead of relying on
`render(Map)`'s guards.
**The original PR description's `@Delegate in traits silently return null`
claim** (section 3 of the Groovy 5 workarounds) is *also* true - that is a
separate Groovy 5 behaviour and converting `GrailsApplicationCommand` from
trait to class was correct and still needed. But it was not the cause of the
scaffolding test failure. That was the `File.asBoolean` trap on top of it.
### 4. `39b5e58de6` - style: single-quoted Strings for non-interpolated
exceptions
CodeNarc `UnnecessaryGString` violations introduced by the two
non-interpolated exception messages added in commit 3 (lines 57 and 193 of
`TemplateRendererImpl`). Switched those two literal Strings from `"..."` to
`'...'`. Trivial follow-up.
## Verification
Against Corretto 21.0.10 on Linux, with `core.autocrlf=false` (the local env
here defaults to `true`, which creates false-positive Spotless and
`./gradlew`-CRLF failures unrelated to this PR):
| Build | Invocation | Result |
|---|---|---|
| `grails-gradle` | `./gradlew build --continue --stacktrace --rerun-tasks
-PskipCodeStyle` | **SUCCESS** (~3m 35s, 41 tasks) |
| Scaffolding test (the original failure) | `cd grails-forge && ./gradlew
:test-core:test --tests
'org.grails.forge.features.scaffolding.ScaffoldingSpec.test generate-controller
command'` | **SUCCESS** (all 4 scaffolded files generated) |
| `grails-core` | `./gradlew build :grails-shell-cli:installDist groovydoc
--continue --stacktrace -PonlyCoreTests -PskipCodeStyle` (CI invocation) |
Scaffolding path passes. Remaining local failures are pre-existing Spring Boot
4 `pluginManagerPostProcessor` bean-wire issues in `grails-test-suite-uber`
(`BeanCreationException: grails/transaction/TransactionManagerAware`); CI does
not hit these. |
| `grails-forge` | `./gradlew build --continue --rerun-tasks --stacktrace
-PgrailsIndy=false -PskipCodeStyle` (CI invocation) | Scaffolding path passes.
Remaining local failures are pre-existing `core.autocrlf=true` CRLF issues in
Spotless and in the forge-cli tests' generated `./gradlew`; CI does not hit
these. |
The three Forge matrix jobs on CI (`Build Grails Forge Java 21
indy=false/true, Java 25 indy=false`) and the `build_groovy` job are the four
failures this push targets.
--
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]
