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
+42 −1
Closed

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

Show file tree
Hide file tree
Changes from 5 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
91 changes: 56 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,15 @@ private static void setupConfiguration(
return;
}

// Consumable-only configurations are published variants of a project — they don't resolve
// dependencies, so version constraints from rootConfiguration are useless on them. Extending
// rootConfiguration also creates a variant model cycle when rootConfiguration contains a
// ProjectDependency on the root project
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 +193,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