jamesfredley commented on code in PR #16011: URL: https://github.com/apache/grails-core/pull/16011#discussion_r3610932474
########## 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: Follow-up on scope: the ABI-leak half of this is fixed (the fixture now compiles against `grails-core-cli-legacy`, not `grails-core-cli`). The precompiled-binary half I'm tracking as an explicit pre-release item under "Precompiled Grails 7 / Groovy 4 fixture" in the PR follow-ups rather than adding it in this change, and I want to be transparent about why. Building the genuine artifact you describe - a command plugin compiled by the Grails 7 / Groovy 4 toolchain, producing real `$Trait$Helper` / `$Trait$FieldHelper` / `@Delegate` forwarder bytecode - means standing up a Groovy 4 toolchain island that resolves `org.apache.grails:grails-core:7.1.1` inside a monorepo whose version management, BOM, and compilation are all Groovy 5 / Java 21. That fixture actively fights the current build architecture (pinning a second Groovy major for one subproject, against a different Grails major), so I don't think it belongs as a bolt-on inside this PR's build. I agree it's the right test to prove the headline claim, and that it should be green before the compat contract is relied on in the upgrade docs - which is exactly why I've flagged it as a pre-release blocker in the follow-ups rather than quietly strengthening it later. My plan is a dedicated fixture pair (a pinned 7.1.1/Groovy 4 "legacy" plugin binary + a Grails 8 "upgraded" app consuming it unchanged) done as its own change where the toolchain isolation can be set up properly, not wedged into this one. ########## 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: Follow-up on scope: I assessed whether to fix this in the current change and am deferring it deliberately, tracked under "Legacy per-command Gradle task discovery" in the PR follow-ups. Reasoning: the `afterEvaluate` config-time resolution isn't specific to the legacy scan - it's the existing shape of `configureApplicationCommands`. The non-legacy cli path resolves at configuration time too (`loadCommandNamesFromCliClasspath` resolves `grailsCliClasspath`, and the task classpath is built from `runtimeClasspath` + `grailsCliClasspath` in the same `afterEvaluate`). A genuinely resolution-order-independent fix - the marker / `withDependencies` approach you suggested - means plugins *advertising* their command names via a Gradle capability/attribute rather than the framework scanning jar `META-INF` at all, which is a discovery-protocol change that would have to cover both the cli and legacy paths to stay consistent, not a surgical edit to the legacy branch. That's larger than this PR and I don't want to bolt a half-version onto only the legacy path. For now the legacy scan mirrors the lenient, skip-on-failure handling the cli path already uses, and degrades to the always-present generic `runCommand` task, so no command becomes unrunnable. I'd rather do the deterministic marker-based discovery as its own change across both paths than leave them inconsistent - happy to take that on as a follow-up, and open to your steer on capability-advertisement vs. dropping per-command legacy tasks entirely in favor of documenting `runCommand -Pargs=...`. -- 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]
