Skip to content

Skip extendsFrom(rootConfiguration) for consumable-only configurations #1614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
FinlayRJW wants to merge 9 commits into from
Closed

Skip extendsFrom(rootConfiguration) for consumable-only configurations #1614

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cc969f1
Skip `extendsFrom(rootConfiguration)` for consumable-only configurations
FinlayRJW Mar 10, 2026
1c78358
minimise diff
FinlayRJW Mar 10, 2026
3ab332b
no need for link
FinlayRJW Mar 10, 2026
c285c10
minimise diff
FinlayRJW Mar 10, 2026
162bdfd
test
FinlayRJW Mar 10, 2026
8d8148c
clearer message
FinlayRJW Mar 11, 2026
b8e9799
undo put in method
FinlayRJW Mar 11, 2026
8daecce
suppress CyclomaticComplexity
FinlayRJW Mar 11, 2026
567d839
improve comment
FinlayRJW Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 57 additions & 35 deletions src/main/java/com/palantir/gradle/versions/VersionsPropsPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,16 @@ private static void setupConfiguration(
return;
}

// 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.
Copy link
Contributor

@felixdesouza felixdesouza Mar 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Copy link
Contributor

@felixdesouza felixdesouza Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@FinlayRJW FinlayRJW Mar 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@FinlayRJW FinlayRJW Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@felixdesouza felixdesouza Mar 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@FinlayRJW FinlayRJW Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (conf.isCanBeConsumed() && !conf.isCanBeResolved()) {
log.debug("Not configuring consumable-only configuration: {}", conf);
return;
}

// Must do all this in a withDependencies block so that it's run lazily, so that
// `extension.shouldExcludeConfiguration` isn't queried too early (before the user had the change to configure).
// However, we must not make this lazy using an afterEvaluate.
Expand All @@ -184,46 +194,58 @@ private static void setupConfiguration(
// conf.getDependencies() is called.
AtomicBoolean wasConfigured = new AtomicBoolean();
conf.withDependencies(deps -> {
if (!wasConfigured.compareAndSet(false, true)) {
// We are configuring a copy of the original dependency, as they inherit the withDependenciesActions.
log.debug("Not configuring {} because it's a copy of an already configured configuration.", conf);
return;
}
if (extension.shouldExcludeConfiguration(conf.getName())) {
log.debug("Not configuring {} because it's excluded", conf);
return;
}
setupConfigurationDependencies(
subproject, extension, rootConfiguration, versionsProps, conf, deps, wasConfigured);
});
}

private static void setupConfigurationDependencies(
Copy link
Contributor

@felixdesouza felixdesouza Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a worthwhile change, please undo

Copy link
Contributor Author

@FinlayRJW FinlayRJW Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have undone - but now require @SuppressWarnings("checkstyle:CyclomaticComplexity") I assume thats okay?

Project subproject,
VersionRecommendationsExtension extension,
Configuration rootConfiguration,
VersionsProps versionsProps,
Configuration conf,
DependencySet deps,
AtomicBoolean wasConfigured) {
if (!wasConfigured.compareAndSet(false, true)) {
// We are configuring a copy of the original dependency, as they inherit the withDependenciesActions.
log.debug("Not configuring {} because it's a copy of an already configured configuration.", conf);
return;
}
if (extension.shouldExcludeConfiguration(conf.getName())) {
log.debug("Not configuring {} because it's excluded", conf);
return;
}

// This will ensure that dependencies declared in almost all configurations - including ancestors of
// published configurations (such as `compile`, `runtimeOnly`) - have a version if there only
// a star-constraint in versions.props that matches them.
provideVersionsFromStarDependencies(versionsProps, deps);
// This will ensure that dependencies declared in almost all configurations - including ancestors of
// published configurations (such as `compile`, `runtimeOnly`) - have a version if there only
// a star-constraint in versions.props that matches them.
provideVersionsFromStarDependencies(versionsProps, deps);

// But don't configure any _ancestors_ of our published configurations to extend rootConfiguration, as we
// explicitly DO NOT WANT to republish the constraints that come from it (that come from versions.props).
if (configurationWillAffectPublishedConstraints(subproject, conf)) {
log.debug("Not configuring published java configuration or its ancestor: {}", conf);
return;
}
// But don't configure any _ancestors_ of our published configurations to extend rootConfiguration, as we
// explicitly DO NOT WANT to republish the constraints that come from it (that come from versions.props).
if (configurationWillAffectPublishedConstraints(subproject, conf)) {
log.debug("Not configuring published java configuration or its ancestor: {}", conf);
return;
}

conf.extendsFrom(rootConfiguration);
conf.extendsFrom(rootConfiguration);

// We must allow unifiedClasspath to be resolved at configuration-time.
if (VersionsLockPlugin.UNIFIED_CLASSPATH_CONFIGURATION_NAME.equals(conf.getName())) {
return;
}
// We must allow unifiedClasspath to be resolved at configuration-time.
if (VersionsLockPlugin.UNIFIED_CLASSPATH_CONFIGURATION_NAME.equals(conf.getName())) {
return;
}

// Add fail-safe error reporting
conf.getIncoming().beforeResolve(_resolvableDependencies -> {
if (GradleWorkarounds.isConfiguring(subproject.getState())) {
throw new GradleException(String.format(
"Not allowed to resolve %s at "
+ "configuration time (https://guides.gradle.org/performance/"
+ "#don_t_resolve_dependencies_at_configuration_time). Please upgrade your "
+ "plugins and double-check your gradle scripts (see stacktrace)",
conf));
}
});
// Add fail-safe error reporting
conf.getIncoming().beforeResolve(_resolvableDependencies -> {
if (GradleWorkarounds.isConfiguring(subproject.getState())) {
throw new GradleException(String.format(
"Not allowed to resolve %s at "
+ "configuration time (https://guides.gradle.org/performance/"
+ "#don_t_resolve_dependencies_at_configuration_time). Please upgrade your "
+ "plugins and double-check your gradle scripts (see stacktrace)",
conf));
}
});
}

Expand Down
27 changes: 26 additions & 1 deletion src/test/java/com/palantir/gradle/versions/VersionsPropsPluginIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty;
import com.palantir.gradle.testing.execution.GradleInvoker;
import com.palantir.gradle.testing.execution.InvocationResult;
import com.palantir.gradle.testing.junit.AdditionallyRunWithGradle;
import com.palantir.gradle.testing.junit.DisabledConfigurationCache;
import com.palantir.gradle.testing.junit.GradlePluginTests;
import com.palantir.gradle.testing.maven.MavenArtifact;
Expand Down Expand Up @@ -83,7 +84,7 @@ void setup(MavenRepo repo, RootProject rootProject) {

allprojects {
repositories {
maven { url "file:///%s" }
maven { url uri("%s") }
Copy link
Contributor Author

@FinlayRJW FinlayRJW Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@felixdesouza felixdesouza Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is said reason?

Copy link
Contributor Author

@FinlayRJW FinlayRJW Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@FinlayRJW FinlayRJW Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe gradle 9 is stricter on having spaces in url strings?

Copy link
Contributor Author

@FinlayRJW FinlayRJW Mar 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah see: https://docs.gradle.org/9.4.0/userguide/upgrading_version_8.html#deprecated_invalid_url_decoding

Copy link
Contributor

@felixdesouza felixdesouza Mar 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk, you tell me! can you go and verify that please? thank you! what do you think that means for some of our excavators?

Copy link
Contributor Author

@FinlayRJW FinlayRJW Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

https://github.com/palantir/gradle-plugin-testing/blob/develop/docs/junit-migration-excavator-instructions.md

Copy link
Contributor

@felixdesouza felixdesouza Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in build.gradle test files, use uri("%s") instead as it is deprecated

Copy link
Contributor

@felixdesouza felixdesouza Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@FinlayRJW FinlayRJW Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet - see palantir/gradle-plugin-testing#416

}
}

Expand Down Expand Up @@ -298,6 +299,30 @@ void creates_rootconfiguration_even_if_versions_props_file_missing(GradleInvoker
gradle.withArgs().buildsSuccessfully();
}

@Test
@AdditionallyRunWithGradle("9.4.0")
Copy link
Contributor Author

@FinlayRJW FinlayRJW Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when all the tests are moved over we can start testing everything on 9.4.0

void consumable_only_configuration_on_root_project_does_not_cause_variant_model_cycle(
GradleInvoker gradle, RootProject rootProject) {
rootProject.propertiesFile("versions.props").setProperty("org.slf4j:*", "1.7.24");

rootProject.buildGradle().plugins().add("java");

rootProject.buildGradle().append("""
configurations.consumable('customOutgoing') {
attributes {
attribute(Usage.USAGE_ATTRIBUTE, objects.named(Usage, 'custom-usage'))
}
outgoing.artifacts(provider {
configurations.compileClasspath.incoming.artifactView {
attributes { attribute(Attribute.of('artifactType', String), 'jar') }
}.files.files
})
}
""");

gradle.withArgs("resolveConfigurations").buildsSuccessfully();
}

@Test
void build_succeeds_without_versions_props_or_versions_lock(GradleInvoker gradle, SubProject foo) {
foo.buildGradle().plugins().add("java");
Expand Down