Lazy exact dependencies #2639

CRogers · 2023-10-10T16:05:36Z

Before this PR

Repo upgrades to Gradle 8 are failing because of errors when using baseline-java-versions and baseline-exact-dependencies together:

> Failed to apply plugin class ‘com.palantir.baseline.plugins.javaversions.BaselineJavaVersion’.
The value for property ‘languageVersion’ is final and cannot be changed any further.

The source of the problem is that in Gradle 8.3, the values of various Java toolchain related attributes are finalised for a Configuration the moment that Configuration is created. baseline-exact-dependencies was using getConfigurations().getByName(...) when configuring source sets, which means that it was forcing the creation of Configurations and so finalising the values of these attributes before baseline-java-versions had a chance to configure the languageVersion etc.

After this PR

==COMMIT_MSG==
baseline-exact-dependencies is now far more lazy around Configuration creation in order to support Gradle 8.
==COMMIT_MSG==

Possible downsides?

This reverts commit d740ca4.

changelog-app · 2023-10-10T16:05:40Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

baseline-exact-dependencies is now far more lazy around Configuration creation in order to support Gradle 8.

Check the box to generate changelog(s)

Generate changelog entry

CRogers · 2023-10-10T16:08:46Z

...e-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java

+                        //  \-- testCompile       extendsFrom(compile)
+                        // We therefore want to look at only the dependencies _directly_ declared in the implementation
+                        // and compile configurations (belonging to our source set)
+                        Configuration implCopy = project.getConfigurations()


I had originally tried to put this in a beforeResolve, where it makes most sense to live. But this causes GCV to have ConcurrentModificationExceptions, as it iterates over all the Configurations and then resolves them while this section of code creates more Configurations, meaning I had to keep it in afterEvaluate

CRogers · 2023-10-10T16:09:41Z

...e-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java

+                            Object value =
+                                    compileClasspath.get().getAttributes().getAttribute(attribute);
+                            conf.getAttributes().attribute((Attribute<Object>) attribute, value);
+                        });


There's no way to lazily add a Map of attributes (aka lazily conditionally add an attribute). So it too must unfortunately live in an afterEvaluate.

I think we can get around this particular usage of afterEvaluate since we don't actually need to copy the map, just the attribute from the compileClasspath => we know the attribute we want and can do use the attribute provider method for a single attribute key.

Additionally, I think this code is only really effective for gradle versions < 5.6? => if our min version is high enough, this bit of code is redundant as it's already covered by

gradle-baseline/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java

Lines 112 to 116 in 36e20de

if (GradleVersion.current().compareTo(GradleVersion.version("5.6")) >= 0) {

attributes.attribute(

LibraryElements.LIBRARY_ELEMENTS_ATTRIBUTE,

project.getObjects().named(LibraryElements.class, LibraryElements.CLASSES));

}

?

This was the first thing I tried and it didn't work. Sometimes the library elements attribute does not exist on source configuration. This then causes attribute(Attribute, Provider<T>) to throw as per the javadoc:

This method can NOT be used to discard the value of an property. Specifying a null provider will result in an IllegalArgumentException being thrown. When the provider has no value at finalization time, an IllegalStateException - regardless of whether or not a convention has been set.

felixdesouza

not everything is actionable, or necessarily for this PR, but would be interesting to see, as it could simplify the class a lot

felixdesouza · 2023-10-10T16:25:13Z

gradle-baseline-java/src/test/groovy/com/palantir/baseline/GradleTestVersions.java

@@ -21,7 +21,7 @@

 public final class GradleTestVersions {
    public static final ImmutableList<String> VERSIONS =
-            ImmutableList.of(Baseline.MIN_GRADLE_VERSION.getVersion(), "7.1.1", "7.3");
+            ImmutableList.of(Baseline.MIN_GRADLE_VERSION.getVersion(), "7.6.2", "8.4");


do you want this to be 8.4 or 8.3?

Might as well use 8.4 as that is latest.

...e-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java

felixdesouza · 2023-10-10T16:27:14Z

gradle-baseline-java/build.gradle

@@ -61,6 +61,7 @@ tasks.test.dependsOn tasks.publishToMavenLocal
 test {
    environment 'CIRCLE_ARTIFACTS', "${buildDir}/artifacts"
    environment 'CIRCLE_TEST_REPORTS', "${buildDir}/circle-reports"
+    systemProperty 'ignoreDeprecations', 'true'


we discussed this internally some time ago - I actually want to set it on everything:

should we just ignoreDeprecations for every gradle project? Some reasons why:

Deprecations often come from other repos we end up running as part of tests. For example, in [internal-repo] there are some tests that run tasks from [other-internal-repo], that then fail with deprecations warnings due to [other-internal-repo]'s tasks. Since we can’t fix them in the [internal-repo], we end up adding --warning-mode=none to every test, losing deprecation warnings anyway. Or worse, people add System.setProperty('ignoreDeprecations', 'true') to individual tests as nebula suggests, making the build flake.

It’s impossible to see what caused deprecation warnings using nebula-test. The only way I found out to get good info is to add the buildscan plugin to the test.

We don’t tend to fix deprecations until a major version remove the feature anyway.

This leads to our gradle plugin repos being super out of date for their own gradle wrapper, meaning they get wrecked by stuff like not supporting jackson 2.15.2's java 20 code.

In this case, we get deprecation warnings about GCV not working with Gradle 9 when running in 8.3/8.4 - something we can't even fix in this repo.

...e-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java

felixdesouza · 2023-10-10T16:29:56Z

...e-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java

+                        // Specifically, we need to pick up the LIBRARY_ELEMENTS_ATTRIBUTE, which is being configured on
+                        // compileClasspath in JavaBasePlugin.defineConfigurationsForSourceSet, but we can't reference
+                        // it
+                        // directly because that would require us to depend on Gradle 5.6.


probably not for this PR, but surely we can raise the min gradle version? does the concern here still hold without it?

I suppose it just comes down to directly referencing the attribute like we did in line 117? instead of copying all the attributes?

We could probably aim to do some cleanup of minimum gradle versions (including the horrifying GradleWorkarounds files that exist in many repos) once we move to Gradle 8.

...e-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java

felixdesouza · 2023-10-10T16:58:02Z

...e-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java

+                            Object value =
+                                    compileClasspath.get().getAttributes().getAttribute(attribute);
+                            conf.getAttributes().attribute((Attribute<Object>) attribute, value);
+                        });


I think we can get around this particular usage of afterEvaluate since we don't actually need to copy the map, just the attribute from the compileClasspath => we know the attribute we want and can do use the attribute provider method for a single attribute key.

Additionally, I think this code is only really effective for gradle versions < 5.6? => if our min version is high enough, this bit of code is redundant as it's already covered by

gradle-baseline/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java

Lines 112 to 116 in 36e20de

if (GradleVersion.current().compareTo(GradleVersion.version("5.6")) >= 0) {

attributes.attribute(

LibraryElements.LIBRARY_ELEMENTS_ATTRIBUTE,

project.getObjects().named(LibraryElements.class, LibraryElements.CLASSES));

}

?

felixdesouza · 2023-10-10T16:59:41Z

...e-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java

+                        // For Gradle 6 and below, the compile configuration might still be used.
+                        maybeCompile.ifPresent(compile -> {
+                            Configuration compileCopy = compile.copy();
+                            // Ensure it's not resolvable, otherwise plugins that resolve all configurations might have
+                            // a bad time resolving this with GCV, if you have direct dependencies without corresponding
+                            // entries in
+                            // versions.props, but instead rely on getting a version for them from the lock file.
+                            compileCopy.setCanBeResolved(false);
+                            compileCopy.setCanBeConsumed(false);
+
+                            project.getConfigurations().add(compileCopy);
+
+                            conf.extendsFrom(compileCopy);
+                        });


redundant? our min version is effectively 7.0 if you apply all the plugins, I think it's 6.7 if you don't apply javaversions

Maybe, tbh I'd rather just leave it while we still have some repos on Gradle 6 (which I should burn down as part of this work).

felixdesouza · 2023-10-10T17:04:27Z

...e-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java

+                    AtomicBoolean projectsEvaluated = new AtomicBoolean();
+                    project.getGradle().projectsEvaluated(g -> projectsEvaluated.set(true));


an aside, do you want some sort of similar check in the flattener?

Honestly, it's a bit redundant given GCV does the same thing

felixdesouza · 2023-10-10T17:30:33Z

...e-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineExactDependencies.java


        TaskProvider<CheckUnusedDependenciesTask> sourceSetUnusedDependencies = project.getTasks()
                .register(
                        checkUnusedDependenciesNameForSourceSet(sourceSet), CheckUnusedDependenciesTask.class, task -> {
                            task.dependsOn(sourceSet.getClassesTaskName());
                            task.setSourceClasses(sourceSet.getOutput().getClassesDirs());
-                            task.dependenciesConfiguration(explicitCompile);
+                            task.getDependenciesConfigurations().add(explicitCompile);


wacky idea, not actually sure, but can't we just use the getHierarchy() method of the configuration we're initially copying? and then take the first one? do we actually need/want a copy, or did we only take a copy because that was the only way to get the first level without the other dependencies from super configurations?

because with that, you can just do a conf.map(c -> Iterables.getFirst(c.getHierarchy())), it's also a LinkedSet populated in DFS manner so in theory it should have what we want assuming we don't want/need a separate copy

Not really sure I follow here, the first time I saw this plugin was this PR. I'd like to focus on getting this to work at all with Gradle 8 first, if we want to improve it/change it in some way it seems less risky to spend time on that intentionally in another change.

felixdesouza · 2023-10-12T11:38:32Z

👍

svc-autorelease · 2023-10-12T11:38:48Z

Released 5.24.0

This reverts commit f6e4309.

Revert "Lazy exact dependencies"

CRogers added 8 commits October 3, 2023 15:57

Time to be maximally lazy

58ba350

Stupid dependencies thing

d740ca4

Revert "Stupid dependencies thing"

916ee8b

This reverts commit d740ca4.

The horrors of afterEvaluate

cc98420

Better versions to test against?

85dd89e

Ignore deprecations

b042de4

just return to afterEvaluate

276fa33

8.4

41e8456

CRogers requested a review from felixdesouza October 10, 2023 16:05

CRogers commented Oct 10, 2023

View reviewed changes

Add generated changelog entries

36e20de

felixdesouza reviewed Oct 10, 2023

View reviewed changes

CRogers added 2 commits October 11, 2023 15:44

fix comment wrapping

8c820e6

@code javadoc

3a4906a

CRogers added merge when ready autorelease labels Oct 11, 2023

bulldozer-bot bot merged commit f6e4309 into develop Oct 12, 2023

bulldozer-bot bot deleted the callumr/lazy-exact-dependencies branch October 12, 2023 11:38

CRogers added a commit that referenced this pull request Oct 13, 2023

Revert "Lazy exact dependencies (#2639)"

306ce35

This reverts commit f6e4309.

bulldozer-bot bot pushed a commit that referenced this pull request Oct 13, 2023

Revert "Lazy exact dependencies (#2639)" (#2642)

bee51c4

Revert "Lazy exact dependencies"

This was referenced Oct 16, 2023

Lazy exact dependencies - take two #2644

Merged

Lazy exact deps - take three #2645

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy exact dependencies #2639

Lazy exact dependencies #2639

CRogers commented Oct 10, 2023

changelog-app bot commented Oct 10, 2023 •

edited by CRogers

Loading

CRogers Oct 10, 2023 •

edited

Loading

CRogers Oct 10, 2023

felixdesouza Oct 10, 2023

CRogers Oct 11, 2023

felixdesouza left a comment

felixdesouza Oct 10, 2023

CRogers Oct 11, 2023

felixdesouza Oct 10, 2023

CRogers Oct 11, 2023 •

edited

Loading

felixdesouza Oct 10, 2023

felixdesouza Oct 10, 2023

CRogers Oct 11, 2023

felixdesouza Oct 10, 2023

felixdesouza Oct 10, 2023

CRogers Oct 11, 2023

felixdesouza Oct 10, 2023

CRogers Oct 11, 2023

felixdesouza Oct 10, 2023

felixdesouza Oct 10, 2023

CRogers Oct 11, 2023

felixdesouza commented Oct 12, 2023

svc-autorelease commented Oct 12, 2023

	if (GradleVersion.current().compareTo(GradleVersion.version("5.6")) >= 0) {
	attributes.attribute(
	LibraryElements.LIBRARY_ELEMENTS_ATTRIBUTE,
	project.getObjects().named(LibraryElements.class, LibraryElements.CLASSES));
	}

		AtomicBoolean projectsEvaluated = new AtomicBoolean();
		project.getGradle().projectsEvaluated(g -> projectsEvaluated.set(true));

Lazy exact dependencies #2639

Lazy exact dependencies #2639

Conversation

CRogers commented Oct 10, 2023

Before this PR

After this PR

Possible downsides?

changelog-app bot commented Oct 10, 2023 • edited by CRogers Loading

Generate changelog in changelog/@unreleased

CRogers Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixdesouza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CRogers Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixdesouza commented Oct 12, 2023

svc-autorelease commented Oct 12, 2023

changelog-app bot commented Oct 10, 2023 •

edited by CRogers

Loading

Generate changelog in `changelog/@unreleased`

CRogers Oct 10, 2023 •

edited

Loading

CRogers Oct 11, 2023 •

edited

Loading