-
Notifications
You must be signed in to change notification settings - Fork 21
[improvement] Stop using lambda in doFirst to improve build caching #116
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.
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
[improvement] Stop using lambda in doFirst to improve build caching #116
Conversation
|
@lycarter Any chance you can write a test that fails without this, and passes with the fix? That way we can be confident we don't reintroduce this break. We tests for similar problems, see |
|
@cakofony this task is already in that test: https://github.com/palantir/gradle-conjure/blob/develop/gradle-conjure/src/test/groovy/com/palantir/gradle/conjure/ConjurePluginTest.groovy#L241 The test passes on Gradle 4.10.2 (what this repo is on), and I believe will fail on Gradle >= 5.0, per some of the comments on that linked gradle issue. I tried testing that out to confirm, but a large pile of other tests fail when I tried upgrading to 5.1 :/ I can dig into upgrading to upgrading to >=5.0, but it'll probably take a little while |
|
Dug in a bit further on the issues with upgrading to Gradle 5.0: they all seem to be because of methods that were deprecated in Gradle 5.0, and there's a check to ensure no deprecated methods are used. Most of these methods are pulled in from |
|
lame, a Gradle 5 upgrade for this repo (while deprecation checks are enabled) is blocked on palantir/gradle-baseline#493 :( |
|
@cakofony this PR can't be tested because it's fixing an issue with a future version of Gradle (and, once you upgrade to that future version, this will be tested by existing tests). In the interim, the caching problem is making |
|
I defer to the other service infnra folks who have stronger gradle-fu than myself :-) The change looks reasonable to me though! |
iamdanfox
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!-- PR title should start with '[fix]', '[improvement]' or '[break]' if this PR would cause a patch, minor or major SemVer bump. Omit the prefix if this PR doesn't warrant a standalone release. --> ## Before this PR Caching checkstyle results isn't possible due to same issue as in palantir/gradle-conjure#116 I can share an internal build scan pointing to this as the culprit. ## After this PR Caching checkstyleMain results should be possible. <!-- Reference any existing GitHub issues, e.g. 'fixes #000' or 'relevant to #000' -->
Before this PR
Copying conjure sources was not cacheable because gradle does not (currently) support Java lambdas: gradle/gradle#5510
bluf of the issue: Java lambdas do not reproducibly generate the same ID internally, so gradle is unable to determine if the lambda has changed. The easy fix is to just use an anonymous function until gradle fixes this upstream.
After this PR
Copying conjure sources is now cacheable
relevant to gradle/gradle#5510