Split CLI dependencies from the runtime classpath#15948
Conversation
jdaugherty
force-pushed
the
fix/split-cli-dependencies
branch
from
a7d2537 to
01ddd85
Compare
|
These were resolving locally due to having maven local search on; I need to fix the dependency substitution to fix these resolution issues. |
|
Turns out there's a real build issue separate from this change, micronaut-singleton isn't including our functional test app so it was pulling from upstream instead of testing with the local copy. |
|
The published Gradle Module Metadata still contains capability-style dependencies (org.apache.grails:grails-core + requireCapability('org.apache.grails:grails-core-cli')) — violating the split's design rule that external consumers must see the plain -cli coordinate, never capabilities. When Gradle resolves that from a Maven repo, it selects grails-core's cliRuntimeElements variant, follows its available-at redirect to the standalone grails-core-cli module, and then sees both nodes providing the same capability — a self-conflict. I reproduced it with a one-dependency project against build/local-maven, and it fails with both timestamped and non-unique snapshots, so it's not a snapshot-timestamp issue. There are two distinct leak paths:
I'm still researching possible solutions for this one. |
|
This is now fixed. We have to support cases where the library is a regular library imported by another project that uses the requireCapbility - this is only when the projects are defined in the same build. I've created a dedicated plugin for this scenario. |
🚨 TestLens detected 1 failed test 🚨Here is what you can do:
Test SummaryCI - Groovy Joint Validation Build / build_grails > :grails-test-examples-gsp-sitemesh3:integrationTest
🏷️ Commit: c6739f4 Test FailuresEndToEndSpec > async multiple levels of layouts (:grails-test-examples-gsp-sitemesh3:integrationTest in CI - Groovy Joint Validation Build / build_grails)Muted TestsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app. |
codeconsole
left a comment
codeconsole
left a comment
There was a problem hiding this comment.
This is the right architecture for the CLI split, and most of the execution is excellent for a 260-file change: the renames are mechanically clean (h5/h7 command twins verified line-identical modulo package, no grails.dev.commands stragglers outside migration docs, license headers preserved), the BOM enumerates companion coordinates automatically via the lazy cliArtifactId sweep so there is nothing to keep in sync by hand, the AST transform ordering is verifiably correct with the ConfigReportCommand bootstrap problem cleanly solved, the docs match the implemented behavior including an honest upgrade note on the clean break, and the grailsCli configuration scoping is right (compile/test visible, absent from runtimeClasspath/bootJar/bootWar). The long-standing Groovy-conflict TODO in the dbmigration builds is genuinely resolved rather than papered over.
I left inline comments on the issues I think need resolution before merge — the two blocking ones are the project-dependency companion wiring in GrailsCliGradlePlugin and the 1.0.0-SNAPSHOT of grails-publish flowing into the published BOM. Details and suggested fixes are on the relevant lines.
A few minor items not worth their own threads:
GrailsApplicationContextCommandRunner's pre-existingCommand not found for name: ...message is now the symptom users will see when a companion was not provisioned; mentioning thegrailsCliconfiguration /-cliartifact in that message would save support traffic.FactoriesFileWriter.addToPropscarries over the old substring-based dedup check (existing.contains(classNodeName)), which miscounts when one FQCN is a prefix of another. Pre-existing, not a regression, but this refactor was the natural moment to fix it.- Four spring-security command files lost their
@author Puneet Behljavadoc tags during the rename (e.g.S2QuickstartCommand) — worth confirming that was intentional.
| if (componentIdentifier instanceof ProjectComponentIdentifier) { | ||
| Project target = project.rootProject.findProject(((ProjectComponentIdentifier) componentIdentifier).projectPath) | ||
| def cliArtifactId = target?.findProperty('cliArtifactId') | ||
| return cliArtifactId ? "${target.group}:${cliArtifactId}" as String : null |
There was a problem hiding this comment.
Blocking: companions advertised by a same-build project dependency are wired as unversioned external module dependencies, so auto-discovery is effectively broken for the plugin-development workflow.
Both branches of findAdvertisedCliArtifact return a bare group:artifactId string, and autoProvisionCliDependencies always adds it via dependencies.create(companion). For a ProjectComponentIdentifier result this means Gradle tries to resolve the companion from a repository instead of binding to the sibling project's cli feature variant. Concretely: a multi-project build with include 'app', 'my-plugin' where my-plugin applies grails-plugin-cli and is unpublished — resolving app's classpath fails (or silently picks up a stale previously-published artifact of the same coordinate).
The framework's own build corroborates the gap: root build.gradle disables auto-provisioning for every framework module, and gradle/functional-test-config.gradle adds a bespoke dependencySubstitution rule that performs exactly the project+capability wiring this method should produce itself.
Composite builds (includeBuild) hit a related edge: the included build's component is a ProjectComponentIdentifier that project.rootProject.findProject(projectPath) either fails to resolve (companion silently skipped) or resolves to a same-named project in the wrong build.
Suggested fix: when the component is a project of the current build, add project.dependencies.project(path) with capabilities { requireCapability(...) }; keep the coordinate path only for ModuleComponentIdentifier results. Alternatively, if in-build discovery is intentionally out of scope, document that and point at a substitution recipe.
| if (cliClasspath == null) { | ||
| return names | ||
| } | ||
| for (File file : cliClasspath.incoming.artifactView { it.lenient(true) }.files) { |
There was a problem hiding this comment.
This forces full resolution of grailsCliClasspath (and, transitively, the grailsCliDetect probe via withDependencies) inside project.afterEvaluate — i.e. at configuration time, on every invocation. ./gradlew help or an unrelated test run pays for resolving grails-core-cli, grails-console, and every discovered companion, and an offline/registry failure silently drops per-command task registration rather than failing loudly. It is also likely hostile to the configuration cache.
I realize task names must be known at configuration time to tasks.register(...), so some of this is a genuine design tension — but the resolution result should at least be cached/scoped so it is not repeated for builds that never touch a CLI task, and the lenient-swallow failure mode deserves a warning log at minimum.
|
|
||
| JavaPluginExtension java = project.extensions.getByType(JavaPluginExtension) | ||
| java.registerFeature(CLI_SOURCE_SET_NAME) { spec -> | ||
| // no explicit capability: the default (`${group}:${project.name}-cli:${version}`) |
There was a problem hiding this comment.
Latent contract mismatch: registerFeature here keeps the Gradle-computed default capability (${group}:${project.name}-cli), but CliArtifactExtension.artifactId is documented as customizable and renames the published jar and advertised coordinate. A consumer who sets cliArtifact { artifactId = 'custom-name' } and then requests requireCapability("$group:custom-name") in-build will fail to resolve, because the in-build capability never followed the extension.
Nothing in the framework customizes artifactId today, so this is unexercised — but either wire spec.capability(...) from the extension value or document the limitation on CliArtifactExtension.artifactId.
| 'gradle-groovy.version' : '4.0.32', | ||
| 'gradle-spock.version' : '2.4-groovy-4.0', | ||
| 'grails-publish-plugin.version' : '1.0.0-M1', | ||
| 'grails-publish-plugin.version' : '1.0.0-SNAPSHOT', |
There was a problem hiding this comment.
Blocking for release: this 1.0.0-SNAPSHOT flows into gradleBomDependencies and from there into the constraints { api ... } block of grails-bom/base, i.e. into the published <dependencyManagement> of org.apache.grails:grails-bom that every application consumes. A staged 8.0.x release would ship a BOM recommending a SNAPSHOT of org.apache.grails.gradle:grails-publish.
I understand this is pending apache/grails-gradle-publish#34 shipping — but the PR should be explicitly marked blocked on that release, with the version reverted to a GA coordinate before merge (or at latest before the first 8.0.x release candidate).
| def cliArtifactId = subproject.findProperty('cliArtifactId') | ||
| if (cliArtifactId) { | ||
| apiConfiguration.dependencyConstraints.add( | ||
| project.dependencies.constraints.create("${subproject.group}:${cliArtifactId}:${projectVersion}")) |
There was a problem hiding this comment.
Related to the grails-publish SNAPSHOT in dependencies.gradle: note that validateNoSnapshotDependencies (below in this file) only iterates config.allDependencies — constraints created here via dependencies.constraints.create(...) (and the constraints { api ... } block above) live in allDependencyConstraints, so the release guard has a blind spot for exactly this kind of leak and would let a SNAPSHOT constraint through a release build unflagged. Worth extending the guard to scan constraints as part of this PR, since the new lazy constraint mechanism widens that surface.
| * owner's cli variant and its {@code available-at} redirect target both provide the capability, | ||
| * so consumers fail with a capability self-conflict (the grails-forge generated-app failure). | ||
| */ | ||
| class CliCompanionPublishingSpec extends GradleSpecification { |
There was a problem hiding this comment.
The publishing side is well covered here (publish → parse .module/.pom → resolve from a fresh repo — nice), but the discovery/wiring half of the feature has no coverage at all: nothing tests autoProvisionCliDependencies/findAdvertisedCliArtifact, the guarantee that grailsCli never leaks into runtimeClasspath/bootJar/bootWar, per-command task registration, or the cliAutoProvision opt-out.
That gap is not hypothetical — the one scenario that would have caught the project-dependency wiring bug (an app resolving a companion advertised by a project dependency, with cliAutoProvision at its default) is never exercised anywhere; the framework build globally disables the feature for its own modules. A functional test with an in-build plugin + consuming app would cover both the bug and the regression.
| * ({@code src/cli/groovy}), which is not a standard project-source location — the transformation | ||
| * must still register them in {@code META-INF/grails-cli.factories}. | ||
| */ | ||
| class CommandFactoriesTransformationSpec extends Specification { |
There was a problem hiding this comment.
This spec only exercises the isCliSource(URL) helper. Nothing drives the transformation end-to-end: compiling a direct ApplicationCommand implementor, an indirect one via the GrailsApplicationCommand trait, and an abstract base class (which must be excluded), then asserting what lands in META-INF/grails-cli.factories. ApplicationContextCommandRegistry also has no spec pinning the intentional clean-break behavior — that a stale ApplicationCommand entry left in a legacy jar's grails.factories is silently ignored rather than loaded or erroring. Since the clean break is the headline breaking change, it deserves a test that locks it in.
2 participants
Description
Grails commands (
ApplicationCommandimplementations) have always shipped inside runtime pluginartifacts, dragging CLI-only dependencies onto application runtime classpaths. This causes real
failures:
grails-shell-clileaks a conflicting Groovy onto the app classpath (see thelong-standing exclusion + TODO in
grails-data-hibernate{5,7}/dbmigration/build.gradle), and aCLI-only Spring Boot
WebApplicationInitializerbreaks classic WAR deployment with an NPE(#15377) unless users hand-filter the
bootWarclasspath.(no classifiers or capability syntax for consumers), fully consumable by Maven as well as Gradle.
Framework changes
grails-core-cli: the command contract moves fromgrails.dev.commands.*toorg.apache.grails.core.cli.*. Command registrations move fromMETA-INF/grails.factoriesto adedicated
META-INF/grails-cli.factories, written by a newCommandFactoriesTransformationthat ships in the cli artifact (so it is active exactly when a command can compile). The default
grails-corejar contains no command code and no command contract.Automatic-Module-Name: dbmigration (h5/h7, 29dbm-*commands each), the hibernate plugins(
schema-export), scaffolding (9generate-*commands), web-url-mappings(
url-mappings-report), spring-security, and spring-security-oauth2.grails-shell-cliis gonefrom every runtime dependency graph.
Build/tooling changes
org.apache.grails.gradle.grails-cliplugin (applied automatically by the Grailsapplication/plugin plugins): registers the
grailsCliconfiguration — compile-visible forgrails-app/commandssources and on the command-runner and test classpaths, but never on themain
runtimeClasspath, so commands and their CLI-only dependencies are excluded frombootRun/bootJar/bootWar. It auto-provisionsgrails-core-cli+grails-console, anddiscovers plugin companions automatically: a command-bearing plugin advertises its companion
through a
Grails-Cli-Artifactmanifest attribute on its runtime jar, and the plugin walks theresolved dependency graph (including transitive plugins) and adds each advertised companion to
grailsCli. Per-command Gradle tasks (dbmUpdate, …) register from the discovered companions —no
buildscript { classpath }entry needed. Opt out withgrails { cliAutoProvision = false }.The
console/shelltasks draw from the same provisioned tier, so generated apps no longerdeclare
console "org.apache.grails:grails-console".org.apache.grails.gradle.grails-plugin-cliplugin for plugin authors (dogfooded by theframework's own modules): one
applysets up the cli source set as a feature variant, thecommand contract on its compile classpath, the manifest advertisement, and the companion
publication.
Support additional source sets and custom publishing grails-gradle-publish#34 (requires
grails-publish≥1.0.0-SNAPSHOT; the version isbumped in
dependencies.gradle).Consumer impact
Most applications upgrade with no build changes: declaring a command-bearing plugin is enough,
and its commands (and Gradle tasks) follow automatically. Application commands in
grails-app/commandsonly need the import rename (grails.dev.commands.*→8.0.x— breaking changes are part of the major release).Code Quality
suites, dbmigration integration tests, the config-report end-to-end integration test
exercising the full discovery → factories → runner chain, forge feature specs, and
grails-gradle plugin tests).
./gradlew build --rerun-tasks.aggregateViolationsreports zeroCheckstyle/CodeNarc/PMD/SpotBugs findings).
beyond the approved scope.
Licensing and Attribution
Documentation
Gradle plugins catalog,
create-commandreference).-cliartifacts, automatic discovery, and the new plugins.grails-cli.factoriesclean break, the
grailsCliconfiguration, and the third-party plugin migration path.