Skip to content

Conversation

@dansanduleac
Copy link
Contributor

Before this PR

checkUnusedDependencies would break on gradle 6.4, because it tried to find the jars of other subprojects rather than the classes directories, but the plugin didn't wire up task dependencies onto the jars.

After this PR

==COMMIT_MSG==
Fix task dependencies onto other subprojects for com.palantir.baseline-exact-dependencies tasks (checkUnusedDependencies, checkImplicitDependencies), so they work with gradle 6.4.
==COMMIT_MSG==

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented May 13, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Fix task dependencies onto other subprojects for com.palantir.baseline-exact-dependencies tasks (checkUnusedDependencies, checkImplicitDependencies), so they work with gradle 6.4.

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from ferozco May 13, 2020 16:41
@dansanduleac
Copy link
Contributor Author

It's just markdown failing on linked URLs it can't resolve... this is really frustrating, I wish we could override it with a check or something.
unit-test-11 was flaking before due to

Caused by: org.gradle.internal.resource.transport.http.HttpErrorStatusCodeException: Could not GET 'https://jcenter.bintray.com/com/google/googlejavaformat/google-java-format/1.7/google-java-format-1.7.pom'. Received status code 502 from server: Bad Gateway

}

@Input
@InputFiles
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is really the key, if you don't set this then gradle doesn't wire up the tasks that produce various files inside these configurations as inputs...
I think we were just getting lucky before.

Comment on lines +112 to +117
if (GradleVersion.current().compareTo(GradleVersion.version("5.6")) >= 0) {
attributes.attribute(
LibraryElements.LIBRARY_ELEMENTS_ATTRIBUTE,
project.getObjects().named(LibraryElements.class, LibraryElements.CLASSES));
}
});
Copy link
Contributor Author

@dansanduleac dansanduleac May 13, 2020

Choose a reason for hiding this comment

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

This is mostly an optimisation so we don't force the production of jars since we can use the classes dirs.
Gradle 6.4 has made it so that it sometimes forces jars when resolving compileClasspath in order to be able to confidently extract the Automatic-Module-Name if there is one set in the manifest.
However I think we can always just use the classes dir for this case, as it won't change the behaviour.

}

@Input
@InputFiles
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be @Classpath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, yep that will fix any ordering issues!

Copy link
Contributor

@ferozco ferozco left a comment

Choose a reason for hiding this comment

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

Could we put together some sort of test for this?

@dansanduleac
Copy link
Contributor Author

so there was a test specifically for this, but it was accidentally running checkUnusedDependencies instead of :checkUnusedDependencies, and thus hiding the fact that the task dependency wasn't there as expected. Fixed now.

@ferozco
Copy link
Contributor

ferozco commented May 14, 2020

👍 Awesome, thanks @dansanduleac

@bulldozer-bot bulldozer-bot bot merged commit 80f15af into develop May 14, 2020
@bulldozer-bot bulldozer-bot bot deleted the ds/fix-unused-dependencies-gradle-64 branch May 14, 2020 13:21
@svc-autorelease
Copy link
Collaborator

Released 3.12.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants