jamesfredley commented on code in PR #16011: URL: https://github.com/apache/grails-core/pull/16011#discussion_r3610868930
########## grails-core-cli-legacy/src/main/groovy/grails/dev/commands/ApplicationCommand.groovy: ########## @@ -0,0 +1,77 @@ +/* + * 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 grails.dev.commands + +import groovy.transform.CompileStatic + +import org.springframework.context.ConfigurableApplicationContext + +import grails.util.Described +import grails.util.GrailsNameUtils +import grails.util.Named + +/** + * Represents a command that runs with access to the + * {@link org.springframework.context.ApplicationContext}. + * + * @author Graeme Rocher + * @since 3.0 + * @deprecated since 8.0, use {@link org.apache.grails.core.cli.ApplicationCommand}. Retained only for backwards compatibility with Grails 7 command plugins and slated for removal in a future major release. + */ +@Deprecated +@CompileStatic +trait ApplicationCommand implements Named, Described { Review Comment: Addressed in d1fe4956a6 by adopting the separate-artifact half of this and declining the opt-in-default-`false` half. **ABI leak — fixed.** The deprecated `grails.dev.commands.*` contract and `LegacyApplicationCommandAdapter` now live in a new published artifact, `grails-core-cli-legacy`, and are gone from `grails-core-cli`. `grails-core-cli` retains **zero** references to `grails.dev.commands` (its registry talks only to the neutral SPI `ApplicationCommandProvider` / `ApplicationCommandRegistrar` / `ApplicationCommandTargetAware`). The legacy artifact is provisioned **execution-only**: `GrailsCliGradlePlugin` puts it in a non-resolvable `grailsCliLegacy` bucket that feeds only `grailsCliClasspath` (the command runner) and the test-runtime classpaths, and **never any compile classpath** — see the new `configurations.matching { it.name in ['testRuntimeClasspath','integrationTestRuntimeClasspath'] }` block and `grailsCliClasspath.extendsFrom(grailsCli, grailsCliLegacy)` in `GrailsCliGradlePlugin.groovy`, plus `CliAutoDiscoverySpec`, which now asserts the artifact is absent from `compileClasspat h` / `testCompileClasspath` / `integrationTestCompileClasspath`. So a new plugin can no longer compile against `grails.dev.commands.*` through `grails-core-cli`; authoring against the deprecated package now requires an explicit `grails-core-cli-legacy` dependency. Your "second breaking change to a core artifact" concern also goes away: retiring the window is dropping `grails-core-cli-legacy`, not re-breaking `grails-core-cli`. Your fourth bullet is implemented literally — the dual-load logic is no longer "permanently inside the new registry"; it lives in `LegacyApplicationCommandProvider` in the compat artifact, contributed through a second factories key that the registry discovers generically. **Opt-in flag defaulting to `false` — declined, deliberately.** This is the one point I'm pushing back on. The goal is a zero-touch upgrade. Spring Boot 3.x is already EOL and Grails 7 is EOL while Grails 8 is not, so users need to move to 8 now — but the plugin ecosystem moves on a 12–24 month horizon (the 3→4/5 transition is still incomplete years on). A flag that defaults to `false` means an unmigrated command plugin **silently contributes no commands** until the user discovers a switch they have no reason to know exists — which is a worse failure than the command simply working behind a one-time deprecation warning. There is also no "switched twice" problem in practice: an author either does nothing (the shim keeps the Grails 7 plugin working) or migrates once to `org.apache.grails.core.cli.*`. The only way to be "switched twice" is to author *brand-new* code against a package that is already `@Deprecated` and warns at every compile — that is an explicit opt-in aga inst visible warnings, not something the framework does to anyone by default. So: legacy support stays **on by default** via the existing `cliAutoProvision`, but it is now isolated in its own artifact and off every compile classpath — which I believe answers the real hazard (accidental new adoption + ABI surface) without degrading the upgrade experience for the ecosystem we're trying not to strand. ########## grails-core/src/cli/groovy/org/apache/grails/core/cli/ApplicationContextCommandRegistry.groovy: ########## @@ -29,21 +33,73 @@ import org.grails.core.io.support.GrailsFactoriesLoader @Singleton(strict = false) class ApplicationContextCommandRegistry { + private static final Logger LOG = LoggerFactory.getLogger(ApplicationContextCommandRegistry) + private final Map<String, ApplicationCommand> commands = [:] + private boolean legacyCommandWarningLogged ApplicationContextCommandRegistry() { + ClassLoader registryClassLoader = ApplicationContextCommandRegistry.classLoader + ClassLoader contextClassLoader = Thread.currentThread().contextClassLoader + for (ApplicationCommand cmd : GrailsFactoriesLoader.loadFactories(ApplicationCommand, - ApplicationContextCommandRegistry.classLoader, GrailsFactoriesLoader.CLI_FACTORIES_RESOURCE_LOCATION)) { + registryClassLoader, GrailsFactoriesLoader.CLI_FACTORIES_RESOURCE_LOCATION)) { if (!commands.containsKey(cmd.name)) { commands[cmd.name] = cmd } } - // If this is reflectively loaded from the delegating cli, we need to make sure the context class loader is also used to pull any commands that are loaded from the gradle classpath - for (ApplicationCommand cmd : GrailsFactoriesLoader.loadFactories(ApplicationCommand, - Thread.currentThread().contextClassLoader, GrailsFactoriesLoader.CLI_FACTORIES_RESOURCE_LOCATION)) { - if (!commands.containsKey(cmd.name)) { - commands[cmd.name] = cmd + // If this is reflectively loaded from the delegating cli, we need to make sure the context class loader is + // also used to pull any commands that are loaded from the gradle classpath. Only when it is a distinct + // classloader: repeating the scan for the same classloader would re-instantiate every command (whose + // constructor may have side effects) just to discard it on the name-collision check below. + if (contextClassLoader != registryClassLoader) { + for (ApplicationCommand cmd : GrailsFactoriesLoader.loadFactories(ApplicationCommand, + contextClassLoader, GrailsFactoriesLoader.CLI_FACTORIES_RESOURCE_LOCATION)) { + if (!commands.containsKey(cmd.name)) { + commands[cmd.name] = cmd + } + } + } + + loadLegacyCommands(registryClassLoader, contextClassLoader) + } + + @SuppressWarnings('deprecation') + private void loadLegacyCommands(ClassLoader registryClassLoader, ClassLoader contextClassLoader) { + // Gather the legacy command classes from the registry classloader and, when it is a distinct + // classloader, the thread context classloader, de-duplicated by Class identity before any are + // instantiated. A child context classloader delegates to its parent, so it also reports the + // parent's grails.factories entries; de-duplicating by the resolved Class avoids instantiating a + // parent-visible legacy command twice (its constructor may have side effects) only to discard the + // duplicate on the name-collision check below. + Set<Class<? extends grails.dev.commands.ApplicationCommand>> legacyClasses = new LinkedHashSet<>() + legacyClasses.addAll(GrailsFactoriesLoader.loadFactoryClasses( + grails.dev.commands.ApplicationCommand, registryClassLoader, FactoriesLoaderSupport.FACTORIES_RESOURCE_LOCATION)) + if (contextClassLoader != null && contextClassLoader != registryClassLoader) { + legacyClasses.addAll(GrailsFactoriesLoader.loadFactoryClasses( + grails.dev.commands.ApplicationCommand, contextClassLoader, FactoriesLoaderSupport.FACTORIES_RESOURCE_LOCATION)) + } + // Instantiate each in isolation: a single stale Grails 7 command whose no-arg constructor (or + // getName()) throws under Grails 8 must be skipped with a warning, never abort the whole registry + // and take valid legacy and new-contract commands down with it. + for (Class<? extends grails.dev.commands.ApplicationCommand> legacyClass : legacyClasses) { + try { + grails.dev.commands.ApplicationCommand legacyCommand = legacyClass.getDeclaredConstructor().newInstance() + ApplicationCommand command = new LegacyApplicationCommandAdapter(legacyCommand) + String name = command.name + if (commands.containsKey(name)) { + continue + } + commands[name] = command + if (!legacyCommandWarningLogged) { + LOG.warn("Command '{}' from a Grails 7 plugin was loaded through the deprecated grails.dev.commands compatibility layer. Ask the plugin author to migrate to the org.apache.grails.core.cli command API and publish a -cli companion artifact; this compatibility path will be removed in a future major release.", name) + legacyCommandWarningLogged = true + } + } + catch (Throwable e) { Review Comment: Split response, since the structural half is now done and the `Throwable` half is a judgment call. **Structural concern — fixed in d1fe4956a6.** This is exactly the redesign you asked for. `ApplicationContextCommandRegistry` no longer hardwires the deprecated discovery path: it now loads `ApplicationCommandProvider` implementations from `META-INF/grails-cli.factories` (a second factories key), dual-classloader, class-identity deduplicated, and failure-isolated, and has **zero** `grails.dev.commands` references. The legacy discovery lives in `LegacyApplicationCommandProvider` inside the separate `grails-core-cli-legacy` artifact and is reached purely through that SPI hook. Eventual removal of the compat window is dropping the artifact; this class is not touched. **`Throwable` / loud linkage failure — point taken, partially.** The catch is deliberate per-command isolation (one plugin's faulty command must not suppress every other command), and it now logs a warning that names the failing class. I agree that a linkage failure (`NoSuchMethodError` / `NoClassDefFoundError`) for a command the upgrade docs promise should work is more serious than generic "command failed to load" noise, and blending it into `info`/`warn` makes it hard to diagnose. I've listed "loud linkage failures" as a tracked follow-up on the PR (surface linkage errors prominently for the named command rather than as a generic warning), and I'd value your take on the right signal there — fail the whole run, or elevate to a distinct error log keyed to the command name while still isolating it. ########## grails-test-examples/legacy-commands-plugin/build.gradle: ########## @@ -0,0 +1,45 @@ +/* + * 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. + */ + +plugins { + id 'org.apache.grails.buildsrc.properties' + id 'org.apache.grails.buildsrc.dependency-validator' + id 'org.apache.grails.buildsrc.compile' + id 'org.apache.grails.buildsrc.vulnerability-scan' + id 'org.apache.grails.gradle.grails-plugin' +} + +version = '0.0.1' +group = 'legacy.commands.plugin' + +dependencies { + implementation platform(project(':grails-bom')) + + // This fixture recompiles legacy command sources against Grails 8's grails-core-cli to + // validate discovery, adapter, registry, and runner wiring end-to-end. It does not + // re-validate a pre-compiled Grails 7 binary's Groovy-trait ABI. That relies on Groovy's + // stable trait encoding across 4->5 and could be strengthened later with a prebuilt Grails 7 + // fixture jar. + compileOnly 'org.apache.grails:grails-core-cli' Review Comment: Two parts here; one is fixed, one is a legitimate open item I've tracked. **The ABI-leak sub-point — fixed.** "compileOnly `grails-core-cli` is sufficient to compile `grails.dev.commands.*`" is no longer true. As of d1fe4956a6 those types are gone from `grails-core-cli` and live only in `grails-core-cli-legacy`; the fixture now compiles against `grails-core-cli-legacy` explicitly, which is the intended and only way to build against the deprecated contract. `CliAutoDiscoverySpec` asserts the legacy artifact is absent from every compile classpath. **The precompiled-binary / trait-ABI point — valid, and I'm not going to hand-wave it.** You're right that recompiling the fixture with this repo's Groovy 5 toolchain answers a different question than "does a plugin class that a Grails 7 / Groovy 4 compiler already wove trait bytecode into (`ApplicationCommand$Trait$Helper` calls, `$Trait$FieldHelper` accessors, the `@Delegate` forwarders to `TemplateRenderer` / `FileSystemInteraction`) still link against these traits as recompiled by Groovy 5?" The restored public signatures were checked against 7.1.1 bytecode, but that is not the same as executing a genuine Groovy 4 trait-consumer binary. I've added this as an explicit **pre-release** follow-up on the PR: a fixture pinned to `org.apache.grails:grails-core:7.1.1` with its Groovy 4 toolchain producing a real trait-consumer jar (the "legacy" app), consumed **unchanged** by a Grails 8 app that executes its commands through the registry/runner/Gradle-task path (the "upgraded" app) — the two-app combination you describe. Agreed it should be green before this is relied on as a compat contract in the upgrade docs. ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/commands/GrailsCliGradlePlugin.groovy: ########## @@ -359,7 +372,63 @@ class GrailsCliGradlePlugin implements Plugin<Project> { } } catch (IOException ignored) { - // unreadable jar — skip + // unreadable jar - skip + } + } + names + } + + /** + * Loads legacy command class names from the {@code META-INF/grails.factories} files of the + * resolved {@code runtimeClasspath} jars, registered under the deprecated + * {@code grails.dev.commands.ApplicationCommand} key. Unchanged Grails 7 command plugins ship + * their commands (and this registration) in their normal runtime jar, so this backwards-compat + * scan registers their per-command Gradle tasks without requiring the plugin to be re-released + * or split into a {@code -cli} companion. Resolution is lenient; unreadable or unbuilt jars are + * skipped (the generic {@code runCommand} task can always execute those commands regardless). + */ + @CompileDynamic + protected Collection<String> loadLegacyCommandNamesFromRuntimeClasspath(Project project) { + Set<String> names = new LinkedHashSet<String>() + Configuration runtimeClasspath = project.configurations.findByName('runtimeClasspath') + if (runtimeClasspath == null) { + return names + } + // Resolving runtimeClasspath at configuration time can race with the configuration of + // sibling source projects in a large multi-project build ("components not calculated yet"). + // A real application resolves this against the module cache without that race and still + // gets its legacy per-command tasks; degrade gracefully (the generic runCommand task can + // always execute the command) rather than failing the whole build if resolution is not yet + // possible - matching the lenient, skip-on-failure handling used for the cli classpath. + Collection<File> files + try { + files = runtimeClasspath.incoming.artifactView { it.lenient(true) }.files.files Review Comment: Valid and separate from the artifact-isolation work; I've tracked it as an explicit follow-up rather than leave it unaddressed. You're right that resolving `runtimeClasspath` in `afterEvaluate` for every project on every invocation is both a cost (it runs even for `gradle help`) and, worse, a correctness problem: the `catch (Throwable)` fallback makes the *task set* depend on whether sibling projects were configured yet, so `helloLegacyApp` may exist locally and be "task not found" on CI, and the configuration cache can bake in either outcome. A sometimes-there task is worse than a consistently-absent one. The fix is to make the legacy per-command task discovery resolution-order-independent — sourced from the same marker / `grailsCli.withDependencies` lenient-view mechanism the CLI companion discovery already uses, rather than an eager `afterEvaluate` classpath resolution — or, failing that, to drop the per-command tasks for legacy plugins and document `runCommand -Pargs=...` instead. I've listed this on the PR under follow-ups. Note the artifact isolation in d1fe4956a6 makes this cleaner to do, since the legacy path is now a well-defined provisioned artifact rather than something interleaved with the core wiring. -- 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]
