jdaugherty commented on code in PR #16011: URL: https://github.com/apache/grails-core/pull/16011#discussion_r3610549387
########## 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: This comment concedes the point the PR's headline claim rests on: nothing in the suite executes a command class that was actually compiled by Grails 7 / Groovy 4. Trait consumers get the trait implementation woven in at *their* compile time — a published plugin's class carries static calls into `ApplicationCommand$Trait$Helper`, the `$Trait$FieldHelper` accessors for `applicationContext`, and (for `GrailsApplicationCommand`) the `@Delegate`-generated forwarders to `TemplateRenderer`/`FileSystemInteraction` that the Grails 7 compiler baked into the plugin class. Whether all of that links against these re-authored traits as compiled by Groovy 5 is exactly the question this PR exists to answer, and recompiling the fixture answers a different one. The fixture needs to compile against a hard-coded, published Grails 7 release (e.g. `org.apache.grails:grails-core:7.1.1` with its Groovy 4 toolchain) — not left open to resolve this repo's Groovy 5 `grails-core-cli`, which is what happens here and is what makes the test answer the wrong question. And this really needs two example apps, not one: 1. a **legacy** fixture: the command plugin built against the pinned Grails 7 release, producing a genuine Groovy 4 trait-consumer binary, exactly as published plugins exist in the wild today; 2. an **upgraded** fixture: a Grails 8 application that consumes that unchanged legacy plugin binary and executes its commands through the registry/runner/Gradle-task path. Until that combination passes, "unchanged published Grails 7 plugins keep working without a re-release" is unverified — and if it doesn't hold, the layer only serves plugins that recompile anyway, which could migrate to the new API for the same effort. Given we'd be committing to a deprecation/compat contract in the upgrade docs, this needs to be proven before merge, not strengthened later. Separately: the fact that `compileOnly 'org.apache.grails:grails-core-cli'` is sufficient to compile `grails.dev.commands.*` sources is itself the ABI-leak problem flagged on `ApplicationCommand.groovy`. ########## 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: This resolves `runtimeClasspath` during configuration (`afterEvaluate`) for every project applying the plugin, on every invocation — including `gradle help` — which the split's own companion discovery deliberately avoids by staying inside `grailsCli.withDependencies` with lenient artifact views. Beyond the cost, the `catch (Throwable)` fallback makes the *task set* nondeterministic: whether `helloLegacyApp` exists depends on whether sibling projects happened to be configured yet, so the same command line can succeed locally and fail on CI with "task not found", and a configuration-cache entry can bake in either outcome. An info-level log line is the only signal that half the feature silently disengaged. If legacy per-command tasks are kept, they need a resolution-order-independent source (the marker/`withDependencies` mechanism the cli companion discovery already uses), or should be dropped in favor of documenting `runCommand -Pargs=...` for legacy plugins — a sometimes-there task is worse than a consistently-absent one. This is also an argument for hosting the whole legacy path in a separate extension-applied compat project, where its wiring can be opt-in rather than a tax on every build. ########## grails-core/src/cli/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: Restoring `grails.dev.commands.*` inside `grails-core-cli` makes the deprecated contract part of the new artifact's compile ABI: any plugin that compiles against `grails-core-cli` (as this PR's own `legacy-commands-plugin` fixture does via `compileOnly`) can keep authoring *new* commands against the old package with nothing but a deprecation warning to stop them. That sets plugin authors up to be switched twice — once now if they migrate, and again in the next major when these types are deleted out of `grails-core-cli` itself, which at that point is a second breaking change to a core artifact rather than the removal of a shim. Suggest moving the restored types plus `LegacyApplicationCommandAdapter` into a separate compatibility project (e.g. `grails-cli-compat7`) that is never part of `grails-core-cli`'s API surface. The Grails Gradle plugin then auto-applies that artifact to the command classpath, gated by an extension flag that **defaults to `false`** — e.g. `grails { legacyCommandSupport = true }` next to `cliAutoProvision` — so a user with an unmigrated Grails 7 command plugin flips one switch, and nothing legacy is provisioned for anyone else. That way: - the legacy surface is off by default: new builds cannot silently compile or run against `grails.dev.commands.*` without an explicit, visible opt-in in their build script; - a plugin updating to Grails 8 compiles against the new API only, so the rename impacts it exactly once; - retiring the layer means dropping one artifact and one flag from the extension wiring, not breaking `grails-core-cli` again; - the dual-load logic added to `ApplicationContextCommandRegistry` can live in the compat artifact behind a hook instead of permanently inside the new registry. ########## 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: Two concerns here: 1. Swallowing `Throwable` means the most likely real-world failure — a Grails 7 binary whose trait-woven bytecode does not link against the restored traits (`NoSuchMethodError`/`NoClassDefFoundError` at instantiation) — degrades to a warning in build output and a silently missing command. That is the same user-visible symptom as having no compatibility layer at all, but harder to diagnose because the docs now promise the command should work. If linkage failure is possible, it should surface loudly for the named command, not blend into log noise. 2. More structurally, this hardwires the deprecated discovery path into the new registry's constructor for the lifetime of the compat window. If the legacy contract lived in a separate extension-applied compat artifact instead, this could be a lookup through an extension hook (or a second factories key contributed by that artifact), keeping `ApplicationContextCommandRegistry` free of `grails.dev.commands` references — and its eventual removal wouldn't touch this class at all. -- 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]
