Skip extendsFrom(rootConfiguration) for consumable-only configurations#1614
Skip extendsFrom(rootConfiguration) for consumable-only configurations#1614
extendsFrom(rootConfiguration) for consumable-only configurations#1614Conversation
Consumable-only configurations (canBeConsumed=true, canBeResolved=false) are published variants that never resolve dependencies, so inheriting version constraints via extendsFrom is useless. It also creates a variant model cycle when rootConfiguration contains a ProjectDependency on the root project, which became a build failure in Gradle 9.4.0 due to gradle/gradle#36245.
Generate changelog in
|
| allprojects { | ||
| repositories { | ||
| maven { url "file:///%s" } | ||
| maven { url uri("%s") } |
There was a problem hiding this comment.
for some reason fails on gradle 9.4.0 without this?
Caused by: java.net.URISyntaxException: Illegal character in path at index 121: file:////Volumes/git/gradle-consistent-versions/build/gradle-plugin-testing/VersionsPropsPluginIntegrationTest/consumable only configuration on root project does not cause variant model cycle/9.4.0/build/mavenrepo/localMavenRepository
at org.gradle.api.internal.file.FileNotationConverter.convert(FileNotationConverter.java:96)
There was a problem hiding this comment.
well gradle 8.14.3 -> gradle 9.4.0 gives:
file:////Volumes/git/gradle-consistent-versions/build/gradle-plugin-testing/VersionsPropsPluginIntegrationTest/consumable only configuration on root project does not cause variant model cycle/8.14.3/build/mavenrepo/localMavenRepository
vs
file:////Volumes/git/gradle-consistent-versions/build/gradle-plugin-testing/VersionsPropsPluginIntegrationTest/consumable only configuration on root project does not cause variant model cycle/9.4.0/build/mavenrepo/localMavenRepository
Whats weird is that the illegal character the error references to is a space which is present in both
There was a problem hiding this comment.
maybe gradle 9 is stricter on having spaces in url strings?
There was a problem hiding this comment.
There was a problem hiding this comment.
idk, you tell me! can you go and verify that please? thank you! what do you think that means for some of our excavators?
There was a problem hiding this comment.
should be okay we have an explicit callout to use uri I added a bit ago:
// ALSO CORRECT - allprojects block when subprojects also need the repo
rootProject.buildGradle().append("""
allprojects {
repositories {
maven { url uri("%s") }
}
}
""", repo.path());There was a problem hiding this comment.
I would not say that it's explicit, it's just an example of adding repositories. If we're seeing this come up again, we should have an explicit callout along the line of:
- don't use
file:///...when inbuild.gradletest files, useuri("%s")instead as it is deprecated
There was a problem hiding this comment.
one could argue, it's a separate concern as it's gradle 9 stuff, but I think it's far easier to nip it in the bud now whilst we're doing the migrations and it migrates to the proper thing, that's forward compatible, as we probably won't run a similar style AI assisted migration for gradle 9 stuffs.
There was a problem hiding this comment.
sweet - see palantir/gradle-plugin-testing#416
✅ Successfully generated changelog entry!Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. 📋Changelog Preview💡 Improvements
|
| } | ||
|
|
||
| @Test | ||
| @AdditionallyRunWithGradle("9.4.0") |
There was a problem hiding this comment.
when all the tests are moved over we can start testing everything on 9.4.0
| }); | ||
| } | ||
|
|
||
| private static void setupConfigurationDependencies( |
There was a problem hiding this comment.
this is not a worthwhile change, please undo
There was a problem hiding this comment.
have undone - but now require @SuppressWarnings("checkstyle:CyclomaticComplexity") I assume thats okay?
| // Consumable-only configurations are published variants of a project — they never resolve | ||
| // dependencies, so version constraints from rootConfiguration are useless on them. | ||
| // Additionally, extending rootConfiguration here creates a variant model cycle on | ||
| // Gradle 9.4.0+. If a consumable config's outgoing artifacts provider resolves another config (e.g. | ||
| // compileClasspath) that also extends rootConfiguration. |
There was a problem hiding this comment.
-
when you say "published" what do you mean?
-
so version constraints from rootConfiguration are useless on them.
I'm not sure I buy this, can you explain your reasoning a bit more please?
What happens to configurations that are all three types? this happens in a lot of places.Will they be broken when they get to 9.x? Probably, but I'm concerned about existing behaviour changing in subtle ways for 8.x. This is also for all consumable-only configurations, not just the root project as that was your initial problem.
There was a problem hiding this comment.
If a consumable config's outgoing artifacts provider resolves another config (e.g. compileClasspath) that also extends rootConfiguration.
I can't parse this sentence, what are you trying to say here
There was a problem hiding this comment.
when you say "published" what do you mean?
sorry, I mean consumable-only configurations are outgoing variants — they expose artifacts to consumers but never resolve dependencies themselves. Since they never resolve, version constraints inherited from rootConfiguration surely will have no effect on them
I can't parse this sentence, what are you trying to say here
Sorry I'm not 100% sure I fully understand but my best guess is extending rootConfiguration on a consumable-only config creates a variant model cycle on Gradle 9.4.0+ when that configurations outgoing artifacts resolve a configuration (e.g. compileClasspath) that also extends rootConfiguration. rootConfiguration ends up reachable from both sides — Gradle includes it in the variant model through the consumable config, and also needs to resolve it through compileClasspath. That circular dependency between variant discovery and resolution is what Gradle 9.4.0+ now rejects.
There was a problem hiding this comment.
This is also for all consumable-only configurations
True I'll add a test to see if this can happen only within a subproject or if this is only an issue with root and tighten the check accordingly
There was a problem hiding this comment.
I guess my qualm with this approach is that, underlying behaviour has changed in gradle 9.4, this change happens to avoid this new behaviour you don't want. However, what I don't get from this PR or its description is what the underlying problem is for this change to be deemed the correct fix. It just feels like because it solves the problem that it's then good to go, which I fundamentally disagree with.
FWIW, this might actually be the right fix, but it's hard to know that given what I'm seeing here, especially considering it may change behaviour for non gradle 9 use cases.
There was a problem hiding this comment.
yeah thats fair - I'll give it some more thought I think I need to more fully understand how this has changed - will close this PR for now / convert to draft and try and figure out exactly what has changed and what the best fix is
Before this PR
GCV applies
conf.extendsFrom(rootConfiguration)to all configurations on every project.rootConfigurationcontains aProjectDependencyon the root project. This creates a variant model cycle when two conditions are met on the root project:gradle-idea-language-injector)compileClasspath)Both configurations extend
rootConfiguration(via GCV). On Gradle 9.4.0, gradle/gradle#36245 changedextendsFromfrom eagerly storing parent configs in aSet<Configuration>to lazily storing them asProvider<Configuration>. When Gradle computes the root project's variant model, it now iterates consumable configs and evaluates their lazyextendsFromchains. FollowingrootConfiguration→ProjectDependency(root)→ re-enters variant model computation on root → cycle.The real-world trigger is
gradle-idea-language-injector, which creates a consumable config on the root project whose outgoing artifacts resolvecompileClasspathto scan jars for annotations. See palantir/gradle-idea-language-injector#149 for a downstream workaround.After this PR
Skips
extendsFrom(rootConfiguration)for consumable-only configurations (canBeConsumed=true,canBeResolved=false). These are published variants that never resolve dependencies, so inheriting version constraints serves no purpose.No behavior change for existing configs:
implementation,api, etc. —canBeConsumed=false→ still getsextendsFromcompileClasspath,runtimeClasspath—canBeResolved=true→ still getsextendsFromapiElements,runtimeElements— already handled byJAVA_PUBLISHED_CONFIGURATION_NAMESPossible downsides?
We are no longer extending from rootConfiguration for any consumable configuration - this could have some unseen side effect, but I don't think it should as we don't care about version constraints from the rootConfiguration on a consumable as they are never resolved
Testing