jdaugherty commented on code in PR #16011:
URL: https://github.com/apache/grails-core/pull/16011#discussion_r3634939275
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/commands/GrailsCliGradlePlugin.groovy:
##########
@@ -307,6 +380,10 @@ class GrailsCliGradlePlugin implements Plugin<Project> {
if (grails.cliAutoProvision.get()) {
commandClassNames.addAll(loadCommandNamesFromCliClasspath(project))
}
+ // Legacy Grails 7 application commands intentionally do not get
named Gradle tasks.
Review Comment:
Confirmed the `loadLegacyCommandNamesFromRuntimeClasspath` scan is fully
deleted and `LegacyCommandTaskDiscoverySpec` asserts both halves of the
contract (`LEGACY_COMMAND_TASK_PRESENT=false`,
`RUN_COMMAND_TASK_PRESENT=true`). This resolves the nondeterministic-task-graph
concern exactly the way I'd hoped — a consistently-absent task plus the
always-present `runCommand`. Thanks for taking option 1 rather than deferring
it.
##########
grails-console/src/main/groovy/grails/ui/command/GrailsApplicationContextCommandRunner.groovy:
##########
@@ -45,8 +46,9 @@ class GrailsApplicationContextCommandRunner extends
DevelopmentGrailsApplication
ConfigurableApplicationContext run(String... args) {
def command =
ApplicationContextCommandRegistry.instance.findCommand(commandName)
if (command) {
+ Object autowireTarget = resolveAutowireTarget(command)
Review Comment:
What prevents the legacy cli from inheriting the cli commands? Wouldn't
then all of the current code then be able to reference the new command class?
##########
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:
On the default-on vs. opt-in question: I've moved that to the dev list so
it's decided by the project deliberately rather than in this PR — let's keep
this thread to mechanics. On the mechanics, one important sizing correction to
"the flip is one convention change": my proposal is default-off **plus
always-on detection that fails loudly**, and the detection half doesn't exist
yet in any form — under `legacyCommandSupport = false` today, an unmigrated
plugin's commands vanish silently, which is precisely the failure mode you
(rightly) objected to.
And the detection must be designed carefully or it reintroduces the problem
we just removed: a configuration-time `runtimeClasspath` scan for legacy
`grails.factories` entries would be the same eager cross-project resolution the
named-task scan had. The detection belongs on the **runtime side**, where the
classpath is already resolved and the check is deterministic:
1. **In `ApplicationContextCommandRegistry` (neutral tier, no
`grails.dev.commands` reference needed):** after provider loading, enumerate
`META-INF/grails.factories` resources on the registry/context classloaders and
check for the `grails.dev.commands.ApplicationCommand` key — a cheap properties
read, no class loading, no linkage risk. If entries exist and no legacy
provider contributed (i.e. `grails-core-cli-legacy` absent), log a single
**error** naming each plugin jar and the exact remediation: `Plugin <jar> ships
Grails 7 commands; set grails { legacyCommandSupport = true } or upgrade the
plugin.`
2. **In the unknown-command path:** when `runCommand`/the shell adapter
fails to find a command name and legacy factories were detected, append the
same hint — that's the moment the user is actually staring at the failure.
3. **Nothing at Gradle configuration time.** If we want a build-side signal
at all, it goes in the runner task's execution action, never configuration.
That design keeps zero-*confusion* regardless of which default the list
lands on — and it's worth building even if the decision is default-on, since it
also covers the `legacyCommandSupport = false` opt-out case. I'm happy to
contribute this piece onto your branch once the list settles the default, so it
doesn't block anything else here.
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/commands/GrailsCliGradlePlugin.groovy:
##########
@@ -254,7 +296,38 @@ class GrailsCliGradlePlugin implements Plugin<Project> {
GrailsCliArtifactGradlePlugin.CLI_ARTIFACT_MANIFEST_ATTRIBUTE, advertised,
project.name)
return null
}
- project.dependencies.create(advertised)
+ // Manifests advertise group:artifact only. Prefer the producer
module's own version so
+ // third-party plugins (versioned independently of Grails) resolve
their matching companion.
+ // Fall back to the current Grails version for framework modules /
unversioned components.
+ String companionVersion = resolveAdvertisedCompanionVersion(artifact,
advertised, project)
+ project.dependencies.create("${advertised}:${companionVersion}")
+ }
+
+ protected static String resolveAdvertisedCompanionVersion(
+ ResolvedArtifactResult artifact, String advertised, Project
project) {
+ def componentIdentifier = artifact.id.componentIdentifier
+ if (componentIdentifier instanceof ModuleComponentIdentifier) {
+ String moduleVersion = ((ModuleComponentIdentifier)
componentIdentifier).version
+ if (moduleVersion) {
+ return moduleVersion
+ }
+ }
+ // Included-build / project components have no module version. The
resolved file is usually
+ // the producer runtime jar (my-plugin-1.2.0-SNAPSHOT.jar), not the
companion jar, so peel a
+ // trailing Maven-style version from whatever jar we have rather than
assuming the companion
+ // artifactId is the filename prefix.
+ File file = artifact.file
+ if (file != null && file.name.endsWith('.jar')) {
+ String baseName = file.name.substring(0, file.name.length() - 4)
+ // First "-<digit>..." from the left is the version start, so
+ // my-plugin-1.2.0-rc-1 and my-plugin-cli-1.0.0-SNAPSHOT both work.
+ for (int i = 0; i < baseName.length() - 1; i++) {
+ if (baseName.charAt(i) == '-' &&
Character.isDigit(baseName.charAt(i + 1))) {
Review Comment:
The left-to-right "first `-<digit>` starts the version" heuristic misfires
on artifact names that legitimately contain a digit after a hyphen:
`my-plugin-2fa-1.0.0.jar` yields version `2fa-1.0.0`, and something like
`grails-oauth2-provider` style names are only safe by luck of where the digit
falls. Since this branch is already the fallback for included-build/project
components, two hardening options:
1. Scan from the **right**: take the last `-` whose following segment parses
as a plausible version (`\d+(\.\d+)*([.-].+)?`), which handles
`my-plugin-2fa-1.0.0` and `my-plugin-cli-1.0.0-SNAPSHOT` correctly; or
2. For `ProjectComponentIdentifier` cases, read the producer project's
version through the artifact's variant/module metadata rather than the filename
at all, and keep the filename peel only as a last resort.
Either way, a unit test with a digit-bearing plugin name would lock the
behavior in.
##########
grails-core-cli-legacy/src/main/groovy/org/apache/grails/core/cli/compat/LegacyApplicationCommandProvider.groovy:
##########
@@ -0,0 +1,88 @@
+/*
+ * 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 org.apache.grails.core.cli.compat
+
+import java.lang.reflect.InvocationTargetException
+
+import groovy.transform.CompileStatic
+import groovy.util.logging.Slf4j
+
+import grails.dev.commands.ApplicationCommand as LegacyApplicationCommand
+import org.apache.grails.core.cli.ApplicationCommand
+import org.apache.grails.core.cli.ApplicationCommandProvider
+import org.apache.grails.core.cli.ApplicationCommandRegistrar
+import org.grails.core.io.support.GrailsFactoriesLoader
+import org.grails.io.support.FactoriesLoaderSupport
+
+/**
+ * Loads commands implemented against the deprecated Grails 7 command contract.
+ */
+@Slf4j
+@CompileStatic
+class LegacyApplicationCommandProvider implements ApplicationCommandProvider {
+
+ private boolean warningLogged
+
+ @Override
+ @SuppressWarnings('deprecation')
+ void contributeCommands(
+ ClassLoader registryClassLoader,
+ ClassLoader contextClassLoader,
+ ApplicationCommandRegistrar registrar) {
+ Set<Class<? extends LegacyApplicationCommand>> legacyClasses = new
LinkedHashSet<>()
+ legacyClasses.addAll(GrailsFactoriesLoader.loadFactoryClasses(
+ LegacyApplicationCommand, registryClassLoader,
FactoriesLoaderSupport.FACTORIES_RESOURCE_LOCATION))
+ if (contextClassLoader != null && contextClassLoader !=
registryClassLoader) {
+ legacyClasses.addAll(GrailsFactoriesLoader.loadFactoryClasses(
+ LegacyApplicationCommand, contextClassLoader,
FactoriesLoaderSupport.FACTORIES_RESOURCE_LOCATION))
+ }
+
+ for (Class<? extends LegacyApplicationCommand> legacyClass :
legacyClasses) {
+ try {
+ LegacyApplicationCommand legacyCommand =
instantiate(legacyClass)
+ ApplicationCommand command = new
LegacyApplicationCommandAdapter(legacyCommand)
+ String installedName = registrar.register(command)
+ if (installedName != null && !warningLogged) {
+ 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.', installedName)
+ warningLogged = true
+ }
+ }
+ catch (LinkageError e) {
Review Comment:
The `LinkageError` split is in and structured the way I asked — thanks.
Three follow-ons on this exact block:
1. **The thread description says `VirtualMachineError`/`ThreadDeath` are
rethrown, but I can't find that on the tip** — this `catch (Throwable)` at line
70 and the provider-boundary `catch (Throwable)` in
`ApplicationContextCommandRegistry.loadCommandProviders` both swallow
everything that isn't a `LinkageError`, so an OOM during command loading
degrades to a warning. If the rethrow guard was dropped in the style pass,
please restore it (`if (e instanceof VirtualMachineError) throw e` before the
generic handling, in both places); if it was never pushed, the same fix applies.
2. **Name the originating jar in the error.**
`legacyClass.protectionDomain?.codeSource?.location` is right there and turns
"which of my 12 plugins is broken?" into a one-line answer for the user
reporting it.
3. **Reword the error away from plugin blame.** Under the compatibility
contract this PR documents, a `LinkageError` against the restored traits is the
*bridge* failing to deliver — the plugin binary was valid on Grails 7 and the
docs promise it keeps working. The message should say this is a Grails
binary-compatibility issue and ask the user to report it to the framework (with
class + jar), rather than "the plugin is likely binary-incompatible... must be
recompiled", which sends the user to the wrong maintainer for what is our bug
to fix.
##########
grails-test-examples/legacy-g7-command-plugin/build.gradle:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.0.10
/ Groovy 4.0.30.
+// 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)
+ }
+}
+
+repositories {
+ mavenCentral()
+}
+
+dependencies {
+ implementation platform('org.apache.grails:grails-bom:7.0.10')
Review Comment:
This is a genuine precompiled fixture and I'm glad it landed in the PR — and
I accept the `GradleBuild` rationale over `includeBuild`: composite
auto-substitution would indeed rewrite `org.apache.grails:grails-core:7.0.10`
to the local Groovy 5 project, which is exactly what the fixture must prevent.
Four tightenings to make it prove what the docs will claim:
1. **`platform` → `enforcedPlatform`, and pin `7.0.14`.** With a plain
`platform`, the BOM's Groovy 4 constraint is only a preference — anything else
on the fixture's classpath could win an upgrade and the fixture would silently
compile under a different Groovy than advertised.
`enforcedPlatform('org.apache.grails:grails-bom:7.0.14')` makes drift
impossible, and 7.0.14 is the current 7.0.x — the fixture should represent what
plugins in the wild were most recently built against.
2. **Derive the manifest stamps instead of hand-typing them.**
`Grails-Compile-Version: 7.0.10` / `Groovy-Compile-Version: 4.0.30` are string
literals that nothing checks; if the BOM pin changes, they lie. Resolve them at
jar time from the compile classpath (the resolved `grails-core` and
`org.apache.groovy:groovy` module versions) so the manifest is evidence, not
assertion — and have the integration spec assert the attributes, not just the
jar name.
3. **Cover `GrailsApplicationCommand` with a second precompiled command.**
The current fixture only implements `ApplicationCommand`, but the riskiest ABI
surface — the one that motivated this fixture — is `GrailsApplicationCommand`:
its `@Delegate`-generated forwarders to
`TemplateRenderer`/`FileSystemInteraction` and the `$Trait$FieldHelper`
accessors that the Groovy 4 compiler baked into the consumer class. A second
command in this same fixture whose `handle()` actually calls
`templateRenderer`/`file(...)`/render-style DSL and reads `applicationContext`
from inside the command would exercise the forwarder bytecode paths. Right now
that trait is only proven by the recompiled in-repo plugin, which is the gap
this fixture exists to close.
4. Minor: `handle()` here only touches `executionContext.baseDir` — even for
the plain `ApplicationCommand` case, reading a couple more contract members
from inside the precompiled code (e.g. `name`/`description` via the trait, the
`applicationContext` getter) widens the linked surface for free.
--
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]
