jdaugherty commented on code in PR #16011:
URL: https://github.com/apache/grails-core/pull/16011#discussion_r3610984575
##########
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:
"The non-legacy path resolves at configuration time too" doesn't carry the
weight this deferral puts on it. The cli path's cost is a pre-existing issue;
what this PR adds is new *nondeterminism*, and the deferral means shipping it
knowingly.
The pattern being merged here — eager cross-project classpath resolution in
`afterEvaluate`, wrapped in `catch (Throwable)` with an info-level log as the
only signal — is precisely the class of Gradle logic I spent Grails 7 removing.
Gradle is a highly parallel system: project configuration order is not
guaranteed, parallel configuration actively exploits that freedom, and
resolution that reaches across project boundaries at configuration time is the
canonical way to get behavior that depends on scheduling. A `catch (Throwable)`
around it doesn't handle the failure — it converts a scheduling accident into a
silently different **task graph**. Whether `helloLegacyApp` exists becomes a
function of which sibling projects happened to configure first; the same
command line succeeds on one machine and dies with "task not found" on another.
There is no error to act on — the user's first contact with any of this is a
missing task, and our first contact is a bug report we cannot rep
roduce.
It also moves us backwards on the roadmap. We don't support the
configuration cache yet, but getting there is the goal — and
configuration-cache compatibility is fundamentally about eliminating exactly
this: order-dependent configuration-time resolution whose outcome can differ
run to run. Every instance of this pattern we merge is one more thing that has
to be found and unwound before we can turn config-cache support on, and this
one is being added with the defect already identified.
These are not hypothetical failure modes. Ordering-dependent `afterEvaluate`
wiring and swallowed configuration-time failures are exactly what made the
pre-7 Grails Gradle integration fragile enough that I rewrote most of it — the
explicit goal of that rewrite, and of this release line, is that Grails builds
behave like plain, predictable Gradle builds. Merging a new feature built on
the pattern we just spent a major release eliminating walks that back, and it
does so in the compatibility layer — the code that will be exercised precisely
by users mid-upgrade, who are least equipped to distinguish "Gradle scheduling
artifact" from "my plugin is broken on Grails 8."
You offered two options in your reply, and either one resolves this **in
this PR**:
1. **Drop the legacy per-command tasks** and document `runCommand
-Pargs=...` for legacy plugins. That's a deletion, not new machinery — the
generic task already works, it's deterministic, and per-command tasks can
arrive later with marker-based discovery done properly across both paths. A
consistently-absent task beats a sometimes-there one, which was the original
point.
2. Source the legacy names from the same `grailsCli.withDependencies`
lenient-view mechanism the companion discovery uses, scoped to the legacy
branch only.
Option 1 is my steer — it's smaller than the code being deferred. What I'm
not willing to do is merge the eager-resolution + `catch (Throwable)` version
with its removal tracked as a follow-up: this PR's follow-ups are already
accumulating pieces of the headline claim that the merged code doesn't actually
hold, and this one would be merged with the defect already on the record.
##########
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:
Answering the question you asked — and asking that the answer land in this
PR rather than the follow-up list, because it's a few lines, not a design
effort:
Split the catch. `LinkageError` (`NoSuchMethodError`,
`NoClassDefFoundError`, `IncompatibleClassChangeError`) is not "a faulty
command" — it's the compatibility layer itself failing to deliver its contract
for a command the upgrade docs promise will work. Catch it separately from
`Exception` and log at **error** level with the command class, the originating
jar, and explicit wording that this is a Grails 7 binary-compatibility failure
that should be reported as a framework bug — while still isolating it so other
commands register. Ordinary `Exception` (a command constructor with side
effects blowing up) can stay a warning; that genuinely is the plugin's fault
and per-command isolation is the right call there. Don't fail the whole run — a
broken command in one plugin shouldn't take out `run-app` — but the two failure
classes must not share a log line, because one is diagnosable by the user and
the other is only diagnosable by us.
The reason this can't wait: until the precompiled Grails 7 fixture exists, a
linkage failure in the wild is the *first* signal we'd get that the trait ABI
doesn't hold — and in the current code that signal is a generic warning. The
distinct-error version is exactly the `catch` block you already have, split in
two.
Reopening this thread: the structural half is fixed, but the
loud-linkage-failure half is still open, so resolving was premature.
##########
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:
The discoverability argument — "an unmigrated plugin silently contributes no
commands until the user finds a switch they don't know exists" — is solvable
without defaulting the shim on, and solving it that way is strictly better.
Keep the legacy `grails.factories` **detection** always-on but make
**provisioning** opt-in: when the registry (or the Gradle plugin) sees a
`grails.dev.commands.ApplicationCommand` entry on the classpath and
`legacyCommandSupport` is not enabled, it fails or errors loudly with the exact
remediation — *"plugin X ships Grails 7 commands; set `grails {
legacyCommandSupport = true }` or upgrade the plugin."* Nothing is silent, the
user flips one documented switch during the upgrade they are already
performing, and nobody who doesn't need the shim carries it.
That matches how we've handled every comparable transition in this release
line — the BOM handling change shipped behind an explicit setting this very
release, indy before it — and it matches the wider ecosystem: Spring Boot did
not keep Jackson 2 on the classpath by default alongside 3; the old line is an
explicit dependency you add if you need it. I'm not aware of a precedent for
default-provisioning a deprecated contract onto every Grails 8 build, test
runtime included.
Default-on also has costs that fall on people who *don't* use legacy
plugins: every application gets `grails-core-cli-legacy` and the
dual-classloader factories scan on its command and test-runtime classpaths for
the lifetime of the window, and — as you noted yourself — when the artifact is
eventually dropped, plugins that never migrated break at that point with no
action ever having been asked of anyone. Default-on removes the only pressure
that makes the deprecation window converge; a visible opt-in is the signal that
tells both users and plugin authors migration is actually expected. The
one-time runtime warning doesn't do that — plugin authors never see their
users' logs.
So: keep everything you built in d1fe4956a6 — the isolation is right — but
gate provisioning on an explicit `legacyCommandSupport = true`, with loud,
actionable detection when it's off. That keeps zero-*confusion* upgrades
without committing the framework to zero-*touch* support of a deprecated ABI by
default.
--
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]
