jamesfredley commented on code in PR #16011: URL: https://github.com/apache/grails-core/pull/16011#discussion_r3630652190
########## 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: Update after the latest push (`c798c93778`): There are now **two** flags, both default **on**: | Flag | Default | Controls | |---|---|---| | `cliAutoProvision` | `true` | Modern CLI tier only: `grails-core-cli`, `grails-console`, discovered companion `-cli` artifacts | | `legacyCommandSupport` | `true` | Grails 7 application-command bridge only: `grails-core-cli-legacy` | Rules in the current code: - Modern CLI auto-provisions when `cliAutoProvision = true` - Legacy bridge auto-provisions only when **both** `cliAutoProvision = true` **and** `legacyCommandSupport = true` - `legacyCommandSupport = false` turns off only the Grails 7 bridge; modern companions still auto-provision - `cliAutoProvision = false` turns off the whole auto-provisioned CLI tier, including the legacy bridge DSL / properties: ```groovy grails { legacyCommandSupport = false // keep modern CLI, disable only G7 bridge } // or grails { cliAutoProvision = false // disable all CLI auto-provisioning } ``` Also: `-PgrailsLegacyCommandSupport=false` / `-PgrailsCliAutoProvision=false` On the ABI point from this thread: that half is already in place. Deprecated `grails.dev.commands.*` lives only in `grails-core-cli-legacy` (execution-only `grailsCliLegacy` bucket). `grails-core-cli` has no compile ABI for the deprecated package. Default remains **on** for `legacyCommandSupport` so unchanged published G7 command plugins keep working with zero user action. The separate flag is there so people who want modern companions without the bridge can opt out cleanly. ########## 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: Update after the latest push: Agreed this was a real problem. Current branch direction for legacy execution is: - keep the execution-only `grails-core-cli-legacy` bridge - do **not** rely on configuration-time `runtimeClasspath` scans to invent sometimes-present per-command Gradle tasks for legacy plugins - run legacy application commands through the generic `runCommand` path (shell adapter routes legacy adapters there) So the nondeterministic "task exists only if sibling projects configured first" path is no longer the intended user surface for legacy commands. Modern companion commands still get named tasks from the CLI classpath discovery path. ########## 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: Update after the latest push: The catch is split as requested: - `LinkageError` (including unwrapped constructor linkage failures): logged at **error** with class + origin, isolated so siblings still load - ordinary `Exception`: warning + skip - `VirtualMachineError` / `ThreadDeath`: rethrown - registry provider boundary also rethrows VM/ThreadDeath before its normal provider failure catch So a binary-incompatible G7 command is loud and named, not a generic swallowed warning, without taking down the whole command set. ########## 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: Update / status on the precompiled G7 fixture: Agreed the composite-build approach is the right way to prove real Groovy 4 trait-consumer bytecode. That is still the plan for proving the headline binary-compat claim end-to-end. What is on the branch now is the isolation + flag + default-on bridge wiring. The dedicated Grails 7 / Groovy 4 included-build fixture pair remains the outstanding proof item called out in the PR body, not something I am claiming is already closed by the recompiled in-repo example alone. -- 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]
