jdaugherty commented on code in PR #16011:
URL: https://github.com/apache/grails-core/pull/16011#discussion_r3670045936
##########
grails-core/src/main/groovy/org/grails/core/io/support/GrailsFactoriesLoader.groovy:
##########
@@ -101,6 +101,56 @@ class GrailsFactoriesLoader extends FactoriesLoaderSupport
{
results
}
+ /**
+ * Loads factory class names grouped by the resource that declares them
without loading the
+ * classes themselves. This lets callers that need diagnostic context
apply stricter class
+ * loading semantics than the framework-wide best-effort factory loader.
+ */
+ static Map<String, List<String>> loadFactoryDeclarations(
+ Class<?> factoryClass,
+ ClassLoader classLoader = GrailsFactoriesLoader.classLoader,
+ String resourceLocation = FACTORIES_RESOURCE_LOCATION) {
+ Assert.notNull(factoryClass, "'factoryClass' must not be null")
+ loadFactoryDeclarations(factoryClass.name, classLoader,
resourceLocation)
+ }
+
+ static Map<String, List<String>> loadFactoryDeclarations(
+ String factoryClassName,
+ ClassLoader classLoader = GrailsFactoriesLoader.classLoader,
+ String resourceLocation = FACTORIES_RESOURCE_LOCATION) {
+ Assert.hasText(factoryClassName, "'factoryClassName' must not be
empty")
+ ClassLoader factoryClassLoader = classLoader ?:
GrailsFactoriesLoader.classLoader
+ Map<String, List<String>> declarations = new LinkedHashMap<>()
+ Enumeration<URL> resources =
factoryClassLoader.getResources(resourceLocation)
+ while (resources.hasMoreElements()) {
+ URL resource = resources.nextElement()
+ Properties properties = new Properties()
+ try {
+ resource.openStream().withCloseable { InputStream input ->
+ properties.load(input)
+ }
+ }
+ catch (IOException | IllegalArgumentException ignored) {
Review Comment:
`catch (IOException | IllegalArgumentException ignored) { continue }` drops
the entire resource with no log line at any level — the class has no logger.
That would be defensible if this only backed the legacy diagnostic, but it
doesn't. `loadFactoryDeclarations` is now the reader for modern discovery too:
`ApplicationContextCommandRegistry:64` for commands and `:159` for providers,
both against `CLI_FACTORIES_RESOURCE_LOCATION`. So a single plugin shipping a
`META-INF/grails-cli.factories` with a stray backslash — `Properties.load`
throws `IllegalArgumentException("Malformed \\uxxxx encoding")` — loses *every*
one of that plugin's Grails 8 commands and providers. The user sees commands
that simply aren't there, with nothing to chase.
It's also a loudness regression against what it replaced.
`FactoriesLoaderSupport.loadFactoryNames`
(`grails-gradle/model/.../FactoriesLoaderSupport.groovy:99-101`) rethrows
`IOException` as `IllegalArgumentException` and doesn't catch
`IllegalArgumentException` at all, so a malformed factories file propagated
instead of vanishing. `ApplicationCommandProviderSpec:301` currently locks the
new silence in — it asserts only `missingCommandHint == null` and
`noExceptionThrown()`.
This is the same failure mode you and I already agreed on for linkage
errors: a command the user expects, silently absent, diagnosable by nobody.
Give the class a logger and `warn` with the resource URL and the cause before
continuing.
Implemented on
[test/8.0.x-legacy-command-e2e](https://github.com/jdaugherty/grails-core/tree/test/8.0.x-legacy-command-e2e):
the class gets `@Slf4j` and the catch warns with the resource URL and cause,
keeping the per-resource `continue`. The existing malformed-resource test now
asserts the warning instead of only `noExceptionThrown()`, and a new one proves
a malformed `grails-cli.factories` in one plugin no longer costs a *different*
plugin its modern commands. `:grails-core:test --tests
ApplicationCommandProviderSpec` green (23 features).
##########
grails-test-examples/legacy-g7-command-plugin/src/main/groovy/legacy/g7/commands/HelloG7PrecompiledCommand.groovy:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package legacy.g7.commands
+
+import groovy.transform.CompileStatic
+
+import grails.dev.commands.ApplicationCommand
+import grails.dev.commands.ExecutionContext
+
+/**
+ * Precompiled against Grails 7 / Groovy 4 so the monorepo can prove that an
unchanged
+ * published Grails 7 application-command binary still links and runs through
the Grails 8
+ * compatibility bridge.
+ */
+@CompileStatic
+class HelloG7PrecompiledCommand implements ApplicationCommand {
+
+ @Override
+ String getName() {
Review Comment:
Overriding `getName()` here is a legitimate Grails 7 shape and this command
should keep it. The gap is that it's the *only* shape covered.
The deprecated trait ships two behaviours. One is "the command declares its
own name", which this fixture exercises. The other is "the command declares no
name and the trait derives one" — `ApplicationCommand.getName()` at
`grails-core-cli-legacy/.../grails/dev/commands/ApplicationCommand.groovy:50-57`,
which runs `GrailsNameUtils.getScriptName(getLogicalName(...))` and, from a
Groovy 4 binary, reaches it through `ApplicationCommand$Trait$Helper`. That
second path has no coverage anywhere in the tree: both fixture commands
override the getters (here at :35/:39, `HelloG7PrecompiledGrailsCommand` at
:29/:33), `LegacyApplicationCommandAdapterSpec:54-55` overrides them, and so do
all eight classes in `LegacyCommandRegistryLoadingSpec` (:332, :354, :373,
:395, :415, :440, :460, :484).
It matters because `class FooBarCommand implements ApplicationCommand` with
no `getName()` is the shape `create-command` generated, so a large share of the
published Grails 7 commands this bridge exists to keep working never declare a
name at all. For those the derivation *is* the registration key — if the Groovy
5 recompile of the trait changes how that default is woven or dispatched into
Groovy-4-compiled implementors, they register under the wrong key or not at
all. That's precisely the class of failure this fixture was built to catch, and
it's the one variant it currently can't see.
So: keep this command as-is, and add a third precompiled one alongside it
that declares neither getter, letting the trait derive the name. The spec then
looks it up under whatever the derivation produces, which only passes if the
default survives the version boundary.
A unit test over in `grails-core-cli-legacy` wouldn't substitute for this —
that would be a Groovy 5 implementor against a Groovy 5 trait, which isn't the
cross-version question. It has to be a Groovy 4 binary to mean anything, which
is why it belongs in this fixture.
There's a working version of this on
[test/8.0.x-legacy-command-e2e](https://github.com/jdaugherty/grails-core/tree/test/8.0.x-legacy-command-e2e):
a third precompiled `HelloDerivedNameCommand` matching the `create-command`
template exactly - implements `GrailsApplicationCommand`, declares neither
getter - which reads its own `name` inside `handle()` so the derivation is
exercised from the Groovy 4 bytecode rather than only from the recompiled
trait. It registers as `hello-derived-name` (confirmed by running the trait
against the precompiled class), and the integration spec asserts both the
registration key and the rendered output. Green.
##########
grails-console/src/main/groovy/grails/ui/command/GrailsApplicationContextCommandRunner.groovy:
##########
@@ -43,10 +44,12 @@ class GrailsApplicationContextCommandRunner extends
DevelopmentGrailsApplication
@Override
ConfigurableApplicationContext run(String... args) {
- def command =
ApplicationContextCommandRegistry.instance.findCommand(commandName)
+ ApplicationContextCommandRegistry commandRegistry =
ApplicationContextCommandRegistry.instance
+ def command = commandRegistry.findCommand(commandName)
if (command) {
+ Object autowireTarget = resolveAutowireTarget(command)
- Object skipBootstrap =
command.hasProperty('skipBootstrap')?.getProperty(command)
+ Object skipBootstrap =
autowireTarget.hasProperty('skipBootstrap')?.getProperty(autowireTarget)
Review Comment:
Reading `skipBootstrap` off the unwrapped target is the right call — the
adapter forwards `handle`, not arbitrary properties — but nothing tests it.
`grep -rn skipBootstrap` across the repo returns only production code: this
file, and the `skipBootstrap = true` declarations in the hibernate5/7
`SchemaExportCommand` and `ApplicationContextDatabaseMigrationCommand`,
scaffolding, and spring-security. No spec mentions it.
That matters more than a coverage count, because the flag decides whether
`BootStrap.groovy` runs. If a later change collapses this back to
`command.hasProperty('skipBootstrap')`, a legacy command declaring `Boolean
skipBootstrap = true` silently stops setting `Settings.SETTING_SKIP_BOOTSTRAP`
and BootStrap executes during something like `dbm-update` against whatever
datasource is configured — with the whole suite still green.
`run()` has the same hole: `LegacyCommandCompatibilityIntegrationSpec`
performs the autowire unwrap by hand at :85-98 instead of driving the runner,
so reverting line 79 back to `command` also leaves every test passing.
Small fix: extract the flag lookup into a static helper next to
`resolveAutowireTarget` and cover adapter-target / plain-command / no-property
in `GrailsApplicationContextCommandRunnerSpec`, which this commit already added.
Implemented on
[test/8.0.x-legacy-command-e2e](https://github.com/jdaugherty/grails-core/tree/test/8.0.x-legacy-command-e2e):
the lookup moves into `static Boolean resolveSkipBootstrap(Object)` beside
`resolveAutowireTarget`, with `run()` behaviour unchanged (the
`System.getProperty` guard stays put). Four features in
`GrailsApplicationContextCommandRunnerSpec` cover plain command, no-property,
adapter-target, and non-Boolean. Mutation-checked: pointing the helper back at
`command` instead of the resolved target fails exactly the adapter-target case.
`:grails-console:test --tests GrailsApplicationContextCommandRunnerSpec` green
(34 features).
##########
grails-test-examples/legacy-g7-command-plugin/build.gradle:
##########
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// Standalone composite-build fixture compiled against published Grails 7 /
Groovy 4.
+// Included by the monorepo root so grails-test-examples-legacy-commands can
consume a real
+// precompiled Grails 7 ApplicationCommand binary instead of recompiling
sources under Grails 8.
+plugins {
+ id 'java-library'
+ id 'groovy'
+}
+
+group = 'legacy.g7.commands'
+version = '0.0.1'
+
+java {
+ toolchain {
+ languageVersion = JavaLanguageVersion.of(17)
Review Comment:
Commenting on this line, though the Java version itself isn't the issue — 17
is correct and should stay. It's the minimum JDK for a Grails 7 app (`7.0.x`
pins `java=17.0.18-librca` in `.sdkmanrc` and `javaVersion=17` in
`gradle.properties`), and matching what a Grails 7 app would actually be built
with is the entire point of this fixture; anything newer would weaken what it
proves. The issue is *where* that requirement is expressed.
Right now it's a Gradle toolchain, and this is the only production build in
the repo that pins one — every other `JavaLanguageVersion` is a TestKit
resource substituted with `__CURRENT_JDK__` or a gated java-compat test
project. We've deliberately stayed off toolchain configuration, and I don't
want this fixture to be what introduces it into the core build.
As written it also can't be satisfied. Nothing provisions a JDK 17: there's
no `foojay-resolver` or `toolchainManagement` block in this build's
`settings.gradle` (which is just `rootProject.name`) or in the root
`settings.gradle`, and no `org.gradle.java.installations.*` property anywhere.
The root `.sdkmanrc` pins `java=21.0.7-librca` and nothing else, and
`etc/bin/Dockerfile` is `FROM bellsoft/liberica-openjdk-debian:21.0.7` with
only a secondary JDK 25 at `/opt/liberica-jdk25`.
And it isn't confined to running the integration test. The fixture jar is an
`implementation` dependency (`legacy-commands/build.gradle:45`), so
`buildLegacyG7CommandFixture` is in the task graph for plain compilation:
```
$ ./gradlew :grails-test-examples-legacy-commands:compileGroovy --dry-run
-PskipTests
:grails-test-examples-legacy-commands:buildLegacyG7CommandFixture SKIPPED
:grails-test-examples-legacy-commands:compileGroovy SKIPPED
```
That's reached by `etc/bin/test-reproducible-builds.sh:42` (`./gradlew build
--rerun-tasks -PskipTests ...`). With JDK 17 hidden:
```
$ ./gradlew -p grails-test-examples/legacy-g7-command-plugin jar \
-Dorg.gradle.java.installations.auto-detect=false
> Cannot find a Java installation on your machine matching:
{languageVersion=17, vendor=any vendor, ...}. Toolchain auto-provisioning
is not enabled.
```
So a contributor whose only JDK is the one we document can't build the repo.
CI can't catch it — it's green only because the GitHub runner images happen to
ship a JDK 17 that Gradle auto-detects.
The fix isn't a different Java version or a resolver plugin. It's to stop
wiring a second JDK requirement into the core build at all, and instead use the
composition model we already use for every other build in this repo that has
its own needs: `grails-forge`, `grails-gradle`, `build-logic`. Those are
independent builds with their own `settings.gradle` and their own `gradlew`,
and the dependency runs one way — `grails-forge/settings.gradle:75` does
`includeBuild('..')`, so Forge composes grails-core and grails-core's own
`./gradlew build` never reaches Forge. CI runs each as its own job
(`working-directory: 'grails-forge'`, `working-directory: 'grails-gradle'`).
I'd like a new top-level `end-to-end` build to serve as the base for this
kind of test, with the Grails 7 fixture and the `legacy-commands` example that
consumes it as its first occupants:
- drop the `java { toolchain { ... } }` block;
- move the fixture and its consuming example into `end-to-end`, out of the
root build graph, so `./gradlew build -PskipTests` never triggers
`buildLegacyG7CommandFixture` and the core build never needs a JDK it doesn't
have;
- declare the Java 17 requirement in a `.sdkmanrc` next to the Grails 7 half
— same mechanism the root and `7.0.x` already use, and it keeps 17 an explicit,
greppable statement of intent rather than something Gradle has to go find;
- run it from its own CI workflow, the way Forge and the Gradle plugins are
run.
That also gives us an obvious home for the next end-to-end compatibility
test instead of finding a new corner for each one.
Two things worth planning for in that layout, both of which a dedicated
workflow handles cleanly and the core build can't:
1. **Two JDKs, explicitly.** The Grails 7 fixture wants 17; the Grails 8 app
consuming it needs 21+. In a dedicated workflow that's just two `setup-java`
steps around two build invocations, which is more honest than asking one Gradle
build to straddle both — and it's what removes the auto-detection dependency
that's making CI pass by luck today.
2. **Keep the Grails 7 half outside dependency substitution.** If
`includeBuild('..')` reaches it, `org.apache.grails:grails-core:7.0.14` gets
rewritten to the local Groovy 5 project and the proof evaporates — which is
exactly why you chose `GradleBuild` over `includeBuild` originally, and I
agreed with that reasoning. The Grails 8 consuming app can compose the root
normally; the fixture can't.
I've put this together as a working branch rather than leave it as a sketch:
[test/8.0.x-legacy-command-e2e](https://github.com/jdaugherty/grails-core/tree/test/8.0.x-legacy-command-e2e)
([single commit](https://github.com/jdaugherty/grails-core/commit/afa4d80440)).
The three projects move to a top-level `end-to-end` build and the toolchain
block is gone. Only the Grails 7 fixture carries a `.sdkmanrc` (17); the Grails
8 side tracks the repository's root `.sdkmanrc` rather than re-pinning 21,
since it has to run on whatever the core build it consumes runs on.
`.github/workflows/end-to-end.yml` reads both versions out of those two files,
so it cannot drift from what `sdk env` gives a developer.
The part worth a look is how it resolves Grails. Rather than
`includeBuild('..')`, it consumes the artifacts the core build publishes — the
same `build/local-maven` that grails-forge points its generated applications
at, populated by `publishAllPublicationsToTestCaseMavenRepoRepository` in the
root and grails-gradle builds. That is what makes these end-to-end: they go
through real poms and module metadata the way an application does.
`settings.gradle` scopes the repository with `exclusiveContent` so a remote
snapshot can't quietly satisfy an `org.apache.grails` request and leave the
suite testing something other than the working tree.
It also disposes of the CLI companion problem instead of working around it.
`grails-core-cli` is a secondary capability of `:grails-core` rather than a
project, so composite substitution can't express it and dies on a capability
self-conflict — but it's a first-class published module whose metadata
`CliPublishingSupport` already rewrites for external consumers, so resolving
from the repository just works. Verified byte-for-byte: the `grails-core-cli`
jar the suite resolves is sha1-identical to the one the publish task had just
written to `build/local-maven`.
Verified end to end: `end-to-end/./gradlew check` green with all six
integration tests passing, the root `./gradlew` configures with zero references
to the moved projects, and the fixture builds under JDK 17 with
`-Dorg.gradle.java.installations.auto-detect=false`. Take it as a proposal, not
a demand.
--
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]
