matrei commented on PR #16180:
URL: https://github.com/apache/grails-core/pull/16180#issuecomment-5634376130
# Review Findings
Head `2fb4179e56` on `8.0.x`, merge base `43510784cf`. #16179 has merged, so
the diff is now only the `end-to-end/` module (14 files, 477 additions). `git
merge-tree` is clean.
CI on this head is green, including "End to End Tests (end-to-end build
only)", which runs `./gradlew check` in `end-to-end` and therefore the new
`i18nCheckOnJvm` task.
I also ran the native half, which the PR says nobody has. Setup followed the
README exactly: Oracle GraalVM for JDK 21 (21.0.12) as `JAVA_HOME` and
`GRAALVM_HOME`, `grails-gradle` and the framework republished to
`build/local-maven` with `-PgrailsIndy=true` (verified: 0 of 300 sampled
`grails-core` classes carry a `CallSiteArray`, 168 contain `invokedynamic`),
then `./gradlew :native-i18n:check -PnativeTests`. The JVM half passed. The
native half fails, twice over, for reasons that are upstream of this PR but
that the PR's README presents as solved. Details in the first finding.
The PR is no longer marked draft, but the description still says "the reason
this is a draft" and "the native half has not been executed yet". The second
statement is accurate and should stay; the draft flag should come back until
the first finding is resolved.
## Findings
### High: the native half cannot pass on 8.0.x as pinned, and the README
recipe does not produce a working image
What happens when the README is followed:
| Step | GraalVM for JDK 21 (21.0.12) | Oracle GraalVM 25.3.4 (native-image
25.0.4.1) |
|---|---|---|
| `nativeCompile` | fails in 4s at "Initializing": `Could not find target
method: ...
Target_org_codehaus_groovy_vmplugin_v7_IndyInterface_invalidateSwitchPoints.invalidateSwitchPoints()`
| succeeds, 5m 34s, 157 MiB binary |
| `i18nCheckOnNative` | not reached | binary dies before Spring starts:
`BootstrapMethodError: NullPointerException` at
`IndyInterface.makeBootHandle(IndyInterface.java:247)` from
`Application.<clinit>` |
The first failure is GraalVM's bundled Groovy substitution targeting a
method Groovy 4+ no longer has (oracle/graal #10200, #13096; fixed on master in
April 2026 and present in the 25 line, not in 21). So the README's "a GraalVM
JDK" has to say "GraalVM for JDK 25 or later", and the JDK-matching advice in
the same section needs rewording, since the framework is then published on 21
and the image built on 25.
The second failure is the real blocker. Line 247 passes
`FROM_CACHE_HANDLE_METHOD`, a `static final` handle that
`IndyInterface.<clinit>` assigns, and it is null: the bootstrap method ran
before its declaring class was initialised. This is not Grails-specific and not
a metadata gap. I reproduced the identical exception with a ten-line Groovy
hello-world, no Spring, no Grails, metadata from the tracing agent, against
Groovy 5.1.2 and against Groovy 4.0.27. Registering every `IndyInterface`
method for reflection changes nothing; `--exact-reachability-metadata` raises
no missing-registration error; `--initialize-at-build-time` for the class fails
the build on image-heap objects, as Groovy's own tracker says it will. The
class-initialisation report confirms `IndyInterface` is `RUN_TIME`.
Groovy knows about it. `IndyInterface.java` in 6.0.0-RC-1 carries this
comment above a new `ensureInitialized()` guard:
> Guards against a GraalVM native-image gap: the runtime invokedynamic
linkage invokes a bootstrap method without running its declaring class's
`<clinit>` first (observed on GraalVM CE 25.2.4; on HotSpot the bootstrap's
`DirectMethodHandle` carries a class-initialization barrier).
The same hello-world against 6.0.0-RC-1 fails at that guard with `BUG!
unreachable: LOOKUP read before initialization`, so even the Groovy 6
workaround does not hold on Oracle GraalVM 25.3.4. The wider work is all on the
Groovy 6 line: GROOVY-12234 "Indy: AOT link mode so dynamic Groovy dispatch
works in GraalVM native images" (6.0.0-beta-2), GROOVY-12227, and in the
unreleased 6.0.0-RC-2 GROOVY-12364, GROOVY-12365 (ship reachability metadata in
the groovy jar) and GROOVY-12366 (ship `native-image.properties`).
GROOVY-12234's description states the status plainly: before it, dynamic Groovy
cannot run in an image at all, indy or not, and the only workaround was
compiling *everything* non-indy with `groovy-callsite`.
Grails 8.0.x pins `groovy.version` 5.1.2 in `dependencies.gradle`. None of
the above is in any 5.1.x release.
Consequences for this PR:
- The README section "Native image verification" describes a recipe (GraalVM
JDK, indy on both sides) as if following it yields an image that runs the
check. It does not, on any released Groovy the BOM can use. The section should
say what the actual precondition is (a Groovy 6 with the AOT link mode, and a
GraalVM on which the class-init gap is closed) and that neither exists in a
release today, so `-PnativeTests` is expected to fail until then. Otherwise the
next person loses a day the way I nearly did.
- The two build comments in `native-i18n/build.gradle` about 16 threads and
six minutes, and the README's `UnsupportedFeatureError: Tried to define class`
paragraph, are observations from some earlier build. Whichever toolchain and
Groovy they came from should be named, because they are not reproducible with
what 8.0.x pins.
- "Released Grails artifacts are built without indy, so no released Grails
can produce a working image" (`README.md:103`) is true but misleading in
isolation: with Groovy 5 no build of Grails can, indy or not. The sentence
should say so.
- The claim in the description that this module lets a missing hint
"actually be falsified" is not yet true. The JVM half is a fixture guard; the
native half needs the Groovy 6 line before it can assert anything. That is fine
for a first landing as long as the module and README say it, and `#16176` is
updated with the two upstream blockers.
What would make the native half useful sooner: the snapshot canary workflow
already rebuilds Grails against a Groovy branch by rewriting `groovy.version`.
Running this module in that job, on GraalVM 25, once GROOVY-12366 ships in
RC-2, is the cheapest way to learn when the upstream gap closes. I did not
attempt a Groovy 6 rebuild of the framework here; it is a different review.
### Medium: the "bundle never registered" diagnostic can never fire under
this configuration
References:
- `end-to-end/native-i18n/grails-app/conf/application.yml:27` sets
`use-code-as-default-message: true`
-
`end-to-end/native-i18n/grails-app/init/nativei18n/Application.groovy:96-99`
catches an exception from `getMessage` and reports "its bundle was never
registered"
- `end-to-end/README.md:60` and the class Javadoc say a missing hint shows
up "as a `NoSuchMessageException`"
With `useCodeAsDefaultMessage` on, `AbstractMessageSource.getMessage(code,
args, locale)` returns the code itself instead of throwing, so a bundle that is
missing from the image produces `actual == code`, and the failure surfaces
through the *other* branch as `'native.plugin.greeting' for fr resolved to
'native.plugin.greeting', expected '...'`. The check still fails, so this is
not a false pass, but the catch block is dead code, its message is the one a
reader would want, and the README and Javadoc describe an exception that this
application is configured never to see.
Suggested fix: keep the property (the unknown-code assertion needs it), but
make `check` treat `actual == code` as "bundle not registered" and say so, and
drop or reword the `NoSuchMessageException` sentence in the README and the
Javadoc.
### Low: leftover debugging in the build script
Reference: `end-to-end/native-i18n/build.gradle:28-31`
```groovy
// Applied the legacy way: the Spring Boot plugin is on this build's
classpath through the
// Grails plugin, so the id resolves, but it carries no version for the
plugins block.
// TEMP probe: disabled
//apply plugin: 'org.springframework.boot.aot'
```
Should go. It is also unnecessary: Spring Boot's Gradle plugin applies
`org.springframework.boot.aot` itself when it sees
`org.graalvm.buildtools.native`, so the native path already runs `processAot`
and the JVM path deliberately does not. If the point was to run AOT processing
on the JVM too, that is a separate decision worth a sentence, not a
commented-out line.
### Low: `grails { indy = true }` duplicates a convention the Grails Gradle
plugin already sets
References:
- `end-to-end/native-i18n/build.gradle:36-41`
-
`grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy:1066-1071`
(`configureNativeImage` sets `indy.convention(true)` when the GraalVM plugin
is applied)
Not wrong, and setting it unconditionally means the JVM run is compiled the
same way as the image, which is a reasonable choice. Note that with Groovy 5 it
also makes no difference to the outcome, see the first finding. But the comment
presents it as something the application has to do, when the plugin does it for
any application that builds an image. Either drop the setting and let the
convention prove itself (arguably more end-to-end), or keep it and say that it
is there so the JVM half matches the native half rather than because an image
needs it.
### Low: the README's justification for the JDK requirement does not match
the build
Reference: `end-to-end/README.md:87-90`: "`grails-gradle` pins no release
target, so it stamps whatever JDK ran it onto its class files".
`grails-gradle` includes `build-logic`, and its four modules apply
`org.apache.grails.buildsrc.compile`, whose `configureJavaVersion` sets
`options.release` on every `JavaCompile`. What is true is that `GroovyCompile`
has no `release` option, so the Groovy half of those modules targets the
bytecode level of the JDK running the compiler. The advice (export the GraalVM
JDK before publishing) is right; the reason given is not. And as the first
finding shows, the GraalVM has to be the 25 line, so the README's implied "one
JDK for everything" does not survive contact either: publish on 21, build the
image with `GRAALVM_HOME` on 25, which the GraalVM plugin honours with
`toolchainDetection = false`. Say "the Groovy sources in `grails-gradle` are
compiled for the running JDK" or similar.
### Low: the GraalVM plugin version is pinned in a new place, one step behind
Reference: `end-to-end/gradle.properties:27`,
`graalvmBuildtoolsVersion=1.1.7`
Nothing else in the repository pins `org.graalvm.buildtools` (checked
`dependencies.gradle` and `grails-gradle`), so a property is a fair place for
it. Maven Central has 1.1.12 as the latest. Not blocking, but since this is the
first pin it may as well start current, and a one-line comment saying why it
lives in `end-to-end/gradle.properties` rather than `dependencies.gradle` would
save the next person the search.
### Nits
- `end-to-end/native-i18n/grails-app/init/nativei18n/Application.groovy:88`:
`System.exit(0)` is only on the success path. On failure the exception leaves
`main` and the JVM exits non-zero only once every non-daemon thread has
stopped. The context is closed in `finally`, so this works today, but a `catch`
that prints and calls `System.exit(1)` would make the exit code independent of
thread cleanup, which matters more for the native binary than the jar.
- `end-to-end/native-i18n/build.gradle:91`: `commandLine` resolves the
binary path eagerly with `.get()` at configuration time. Wiring it from
`nativeCompile`'s output (`tasks.named('nativeCompile').flatMap { it.outputFile
}` or the binary's `outputDirectory`) keeps the path and the dependency in one
place.
- `end-to-end/native-i18n/build.gradle:84`: the comment on
`toolchainDetection = false` explains why a failed image is a failure, not what
the setting does (use `GRAALVM_HOME`/`JAVA_HOME` instead of a Gradle
toolchain). Worth one clause.
- `end-to-end/native-i18n/grails-app/i18n/messages_fr.properties:19`: "de l
application" reads as a typo. Spring's `ResourceBundleMessageSource` does not
pass a message with no arguments through `MessageFormat`, so `l'application`
renders as written; if the apostrophe was dropped to be safe, `l''application`
is the conventional spelling.
- Neither new module applies
`org.apache.grails.buildsrc.vulnerability-scan`, which `legacy-commands`,
`legacy-commands-plugin` and `spring-dependency-management` all do. Probably
deliberate for a fixture with two dependencies, but consistency is cheap.
- README table rows are inserted between `spring-dependency-management` and
`taglib-index-incremental`; the table was alphabetical.
## Verified as correct
- The plugin fixture does exercise the multi-word matching it claims:
`NativeMessagesGrailsPlugin` yields `native-messages` via
`GrailsNameUtils.getPluginName` in the descriptor, the runtime reports
`nativeMessages`, and `EffectiveI18nDescriptors` normalises both through
`PluginUtils.normalizePluginName`. The base name `native-messages` also passes
`GenerateI18nDescriptorTask.validatePluginNamespace`.
- The dotted base name is covered by
`I18nRuntimeHintsProcessor.toResourcePath`, which is exactly the conversion the
third case tests, and `I18nEnvironmentPostProcessor.compose` keeps an
application-declared base name ahead of the discovered ones, so
`config.i18n.custom` survives the merge.
- `fallback-to-system-locale: false` is what makes the `Locale.ENGLISH`
assertions land on the base bundle regardless of the machine's default locale.
Good.
- Apache license headers are present on every new file, including the
bundles.
## Beyond this PR
- **Native image support in Grails 8 depends on Groovy 6.** Everything above
reduces to this. `#16176` should record it, and whoever owns the AOT work
(#16094) should decide whether native image is an 8.0 claim or an
8.x-with-Groovy-6 claim, because the upgrade guide and the Gradle plugin's
`configureNativeImage` convention currently imply the former.
- **Indy default for releases.** `README.md:103` is right that
`CompilePlugin` defaults `grailsIndy` to `false`, so even once Groovy is fixed
a released Grails cannot be imaged. That needs an issue or a decision before
8.0.0.
--
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]
