Skip to content
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

Add incremental build support to gradle plugin #1165

Merged
merged 1 commit into from
Aug 14, 2019
Merged

Add incremental build support to gradle plugin #1165

merged 1 commit into from
Aug 14, 2019

Conversation

anatoliy-balakirev
Copy link
Contributor

@anatoliy-balakirev anatoliy-balakirev commented Aug 12, 2019

In order to make it working - I had to define clear input and output
params for each gradle task.
As part of this task I've also cleaned up some services to define clear
inputs instead of generic ContractVerifierConfigProperties.inputs instead of generic ContractVerifierConfigProperties.

Fixes gh-1133

@pivotal-issuemaster
Copy link

@anatoliy-balakirev Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@anatoliy-balakirev Thank you for signing the Contributor License Agreement!

@shanman190
Copy link
Contributor

So I'll admit that I've had similar issues while working through my refactor of this problem as well. Like you've seen the global mutable state either via the extension updates from within tasks or via the ext of the tasks.

This plugin really needs to have a pretty major refactoring to bring it up to what would be considered current standards. You've definitely gone a pretty substantial way there.

Some things that I'd like to note as pieces we should also consider:

  • Plugins are recommended not to automatically create publications unless they are very opinionated about the lifecycle. (Eg: java-gradle-plugin)
  • afterEvaluate in this plugins case shouldn't really be necessary. The one place I've found that isn't the case is when attaching as a depends on relationship with the compileTestJava or compileTestGroovy tasks.
  • Extension properties can take advantage of being lazy configuration which means that when the property is set by the user later on, then it Cascades through to the tasks that use it as well.
  • Could potentially thing forward a little further to build caching. This part I'm working through ideas in relationship to how the StubsDowloader downloads the contracts to a temporary directory outside of Gradle's lifecycle and is additionally managing this temp storage with the JVM shutdown cleanup in TempStorage. If we let this just be temp storage and copy to an output then it shouldn't be so bad.

These are just some of the things that are coming to mind currently. I'll try to get my branch that I'm working on cleaned up and pushed out for a compare and contrast if anybody is interested?

@anatoliy-balakirev
Copy link
Contributor Author

@shanman190 oh, so you worked on the same in parallel? You sould've mentioend this in the issue, could save us quite some time...

Regarding your points:

  1. Interesting, wasn't aware of this (I'm quite new to gradle plugin development; this is actually my first one :) ). But I'm not sure this is related to the incremental build feature, I worked on originally. Maybe worth to create a separate issue / PR for that?
  2. and 3. - Nice! Wasn't aware that lazy evaluation can be used with extension properties. Will try to adjust that. I used afterEvaluate to actually simulate this laziness in that particular part.
  3. Good idea! But again, maybe worth a separate issue / PR?

Copy link
Contributor

@marcingrzejszczak marcingrzejszczak left a comment

Choose a reason for hiding this comment

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

That's a lot of changes, so I've gone only through some of them.

Why did you delete ContractVerifierConfigProperties? I understand that we might not want to have it in the plugins, but we can just remove it from the plugins and not touch most of the code

@marcingrzejszczak
Copy link
Contributor

So I'll admit that I've had similar issues while working through my refactor of this problem as well. Like you've seen the global mutable state either via the extension updates from within tasks or via the ext of the tasks.

Yup, that's a problem.

Plugins are recommended not to automatically create publications unless they are very opinionated about the lifecycle. (Eg: java-gradle-plugin)

I guess we're opinionated. We want to have the stub jar publication

@anatoliy-balakirev
Copy link
Contributor Author

Why did you delete ContractVerifierConfigProperties? I understand that we might not want to have it in the plugins, but we can just remove it from the plugins and not touch most of the code

@marcingrzejszczak
So, first of all, there are a couple of usage scenarios for that class:

  1. Many fields are used as an input to the TestGenerator (+ all sub services it calls) and not needed anywhere else. They all are input fields only, it never mutates any of those;
  2. In quite some places only a couple of fields were taken from that class and everything else was in fact not needed (the most extreme case is JsonToJsonPathsConverter.groovy, where we only need one field from this huge class, but there are a lot of other similar examples, where we only need some really small subset of the fields (e.g. 2-3));
  3. In some other places it's used as an output. I.e. nothing is taken from it, but something is modified in it for further usages (which is not always the case as we are not always sharing the same instance, which is making it even harder to reason about). The fact that this class is mutable, makes this scenario possible everywhere in the code and makes analysing the flow quite hard.

So I had a couple of options to proceed with my refactoring:

  1. Modify as much services as possible to be very explicit about what they need (and there are also some TODOs in the code, suggesting this, which actually encouraged me to proceed);
  2. Leave existing services as is (i.e. using that global variable) and only change gradle plugin to dynamically create a new instance of that class every time I need to interact with those services. I was considering this option as well as a bit easier to start with.

However, option #2 is very fragile:

  1. It is not clear what is actually needed by the service;
  2. It is not clear whether service is mutating something inside the ContractVerifierConfigProperties and that should be used as an output;
  3. Even if I figure out each of those points now - it's quite easy to change those services to start using some new field from the ContractVerifierConfigProperties (just because it's already there, why not taking it?) and that will silently break plugin (required input fields won't be passed / marked as an input or mutated state won't be passed further).

Based on this + based on the fact that some other contributors wanted to do this refactoring anyway (there are TODOs in the code) - I decided to go for option #1 as a more clear / robust solution.

So I now we have following setup:

  1. Services, which need some smaller number of arguments are defining them directly in their signatures;
  2. TestGenerator needs a lot of data, so I created dedicated class TestGeneratorInput, which is immutable and designed to be used only with that service. Again, we have a clearly defined input data;
  3. ContractVerifierConfigProperties is left only in the maven plugin and in a couple of tests, as otherwise my PR would be even bigger.

So we still need another follow up PR to remove ContractVerifierConfigProperties from maven plugin as well. I can do that later.

@marcingrzejszczak
Copy link
Contributor

In some other places it's used as an output. I.e. nothing is taken from it, but something is modified in it for further usages (which is not always the case as we are not always sharing the same instance, which is making it even harder to reason about). The fact that this class is mutable, makes this scenario possible everywhere in the code and makes analysing the flow quite hard.

I agree so we can convert the inputs and outputs separately from removing that property configuration.

Modify as much services as possible to be very explicit about what they need (and there are also some TODOs in the code, suggesting this, which actually encouraged me to proceed);

I dislike being extremely explicit cause afterwards, if there's one more field needed to be used I need to refactor a milion places. Also, often, I have to deprecate an existing public constructor and create a new one. Remember that if we have a public API then we can't just remove stuff. We need to deprecate it and remove it when we have version e.g. 3.0.0

Even if we decide to go that way, I'd prefer that to be done in a separate PR.

Leave existing services as is (i.e. using that global variable) and only change gradle plugin to dynamically create a new instance of that class every time I need to interact with those services. I was considering this option as well as a bit easier to start with.

Let's go with that if possible. If you think that option 2 makes more sense and won't interfere with the public API then let's go with option 2.

It is not clear whether service is mutating something inside the ContractVerifierConfigProperties and that should be used as an output;

We can try to find those points and first change the process somehow and only then change the API?

WDYT?

@anatoliy-balakirev
Copy link
Contributor Author

anatoliy-balakirev commented Aug 12, 2019

I dislike being extremely explicit cause afterwards, if there's one more field needed to be used I need to refactor a milion places.

Yes, there is always some tradeoff... It's either easier to add something without changing many places, but then harder to read / understand what we ended up having, or harder to modify, but easier to reason about each service's API. As at the end developers are spending much more time reading the code than actually writing it - I would still go for the approach with properly defined APIs, TBH.

Anyway, let me try to roll back my TestGeneratorInput and changes to public APIs to have smaller PR for now.

@codecov
Copy link

codecov bot commented Aug 12, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ca72481). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1165   +/-   ##
========================================
  Coverage          ?   2.27%           
  Complexity        ?      18           
========================================
  Files             ?      60           
  Lines             ?    2158           
  Branches          ?     134           
========================================
  Hits              ?      49           
  Misses            ?    2101           
  Partials          ?       8

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca72481...7ec1a54. Read the comment docs.

@anatoliy-balakirev
Copy link
Contributor Author

anatoliy-balakirev commented Aug 12, 2019

@marcingrzejszczak I've reverted all my changes related to TestGeneratorInput. Still, I left my other changes to a couple of services, switching them to more fine grained input. When services are public - left an old signature as well, deprecating it. Can you take a look pls? Is it better now or you would rather do those changes in the separate PR as well? (they seem to be quite not big, so decided to leave)

@shanman190
Copy link
Contributor

Plugins are recommended not to automatically create publications unless they are very opinionated about the lifecycle. (Eg: java-gradle-plugin)

I guess we're opinionated. We want to have the stub jar publication

So on this one, I've got a story for you. In the case that I have, I'd really honestly go down the path of deprecating this and forcing the end user to specifically add the stubsJar as an artifact that they publish themselves.

Story time...

In our environment (my workplace), we actually have to have every project disable the publications as a result of an oddity we encountered and have just had time to workaround, but not really discuss until now. Basically, our build server is only allowed to push artifacts into our Artifact repository, but not allowed to delete. In several cases, we have teams that are using SCC in the form of CDC as part of the service application Gradle module. This module publishes a Spring Boot jar, source jar, etc. In our case, the stubsJar publication actually conflicts with our normal maven-publish publication which had our repository not been configured for write once, then it would have overwritten the root publication. In our case, we just disabled the built-in publishing and added the stubsJar as an artifact to our publication which achieves the same goal without messing up the original pom file.

publishing {
    publications {
        maven(MavenPublication) {
            from components.java

            // other artifacts, etc
            artifact sourceJar

            artifact stubsJar
        }
    }
}

Now this definitely doesn't need to be a part of this pull request in order to achieve task caching, but I think that it's something to talk about. @marcingrzejszczak, I can write up an issue if you'd prefer to handle it that way or to keep it as a reminder?

@anatoliy-balakirev
Copy link
Contributor Author

anatoliy-balakirev commented Aug 12, 2019

Something weird is going on with the build after I synced with original repo...
On the CI (https://circleci.com/gh/spring-cloud/spring-cloud-contract/8633?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link)
Build is constantly failing with a lot of errors:

[ERROR] Errors: 
[ERROR]   JavaContractConverterSpec.should convert Java DSL with REST to DSL for [#contractFile]:68 NoSuchElement
[ERROR]   JavaContractConverterSpec.should convert Java DSL with REST to DSL for [#contractFile]:68 NoSuchElement
[INFO] 
[ERROR] Tests run: 1093, Failures: 5, Errors: 2, Skipped: 0
[INFO] 

While locally it all looks good:

[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 1093, Failures: 0, Errors: 0, Skipped: 0
[INFO] 

Moreover, reported failures are in the tests I didn't even touch.

@marcingrzejszczak I see those are new tests added by you today. Any idea what's going on there?

@shanman190
Copy link
Contributor

@anatoliy-balakirev, here's my work in progress if you want to use some of it for inspiration. Mine is going way left field in terms of changes, so yours will make more sense in the short term.

shanman190@34bf9af

@anatoliy-balakirev
Copy link
Contributor Author

@shanman190 Nice, will take a look. Thanks!

@marcingrzejszczak
Copy link
Contributor

Plugins are recommended not to automatically create publications unless they are very opinionated about the lifecycle. (Eg: java-gradle-plugin) ...

@shanman190 hey thanks for the story :)

Let's create a separate issue. Removing that default would be a breaking change :/

@marcingrzejszczak
Copy link
Contributor

Something weird is going on with the build after I synced with original repo... ...

@anatoliy-balakirev eee no idea. Our builds work fine in jenkins. Are your sure you synced with the latest changes?

@anatoliy-balakirev
Copy link
Contributor Author

anatoliy-balakirev commented Aug 13, 2019

@anatoliy-balakirev eee no idea. Our builds work fine in jenkins. Are your sure you synced with the latest changes?

@marcingrzejszczak I created another small PR to fix a couple of NPEs and it fails with the same:
https://circleci.com/gh/spring-cloud/spring-cloud-contract/8646?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

PR is here: #1170

Both that one and this are based on the latest master, so there is definitely some issue with it / its config for CI:
Screen Shot 2019-08-13 at 10 44 30

Also it started failing when I rebased it yesterday.

@marcingrzejszczak
Copy link
Contributor

Can you rebase against master and try to build it again with the new base image for circleci?

@marcingrzejszczak
Copy link
Contributor

Jacoco was the culprit. Please rebase and it should work fine.

@anatoliy-balakirev
Copy link
Contributor Author

@marcingrzejszczak that helped, thanks!

@anatoliy-balakirev
Copy link
Contributor Author

@marcingrzejszczak @shanman190 I've updated PR with all the latest changes. Switched extension to use lazy properties as well. Take a look pls, I believe it should be the final version.

In order to make it working - I had to define clear input and output
params for each gradle task.
As part of this task I've also cleaned up some services to define clear
inputs instead of generic `ContractVerifierConfigProperties`.

Fixes gh-1133
@anatoliy-balakirev
Copy link
Contributor Author

The last rebase was to restart CI tests as they seem to be flaky.

@marcingrzejszczak
Copy link
Contributor

Congrats @anatoliy-balakirev and @shanman190 on this fantastic PR. Great work!

@anatoliy-balakirev
Copy link
Contributor Author

Thanks! :)

@anatoliy-balakirev
Copy link
Contributor Author

@marcingrzejszczak I just realised that this PR introduced some kind of backward incompatible change in terms of configuring plugin in the build.gradle.kts (from here: https://docs.gradle.org/current/userguide/lazy_configuration.html):

Note that Gradle Groovy DSL will generate setter methods for each Property-typed property in a task implementation. These setter methods allow you to configure the property using the assignment (=) operator as a convenience.

Kotlin DSL conveniences will be added in a future release.

So when using Kotlin DSL, instead of writing this:

contracts {
    packageWithBaseClasses = "com.numbrs.referral.controller.contract"
    basePackageForTests = packageWithBaseClasses
    testMode = TestMode.WEBTESTCLIENT
    testFramework = TestFramework.JUNIT5
}

We'll have to do this (note .set() instead of =):

contracts {
    packageWithBaseClasses.set( "com.numbrs.referral.controller.contract")
    basePackageForTests.set(packageWithBaseClasses)
    testMode.set(TestMode.WEBTESTCLIENT)
    testFramework.set(TestFramework.JUNIT5)
}

Groovy DSL is not affected.
I can create a new PR to add explicit getters if that's an issue.

@marcingrzejszczak
Copy link
Contributor

Yeah that's exactly what we had to do here 3c4772b

It's not a breaking change though since there was no support whatsover for Kotlin. However having consistent behavior would be great. Sure, file another PR :)

@anatoliy-balakirev
Copy link
Contributor Author

I checked this further and looks like currently it's not really possible to do, as Kotlin doesn't support properties with overloaded setters. Here is an original discussion:
gradle/kotlin-dsl-samples#597

And a couple more related links:
gradle/kotlin-dsl-samples#380
https://youtrack.jetbrains.com/issue/KT-24831
https://youtrack.jetbrains.com/issue/KT-4075

So for now Kotlin users will have to write property.set("value") instead of property = "value".
So you can adjust it here as well: 3c4772b

That big assignment (packageWithBaseClasses = objects.property(String::class).value("com.example.fraud")) is not needed and can / should be replaced with:
packageWithBaseClasses.set("com.example.fraud")

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.

Support Up-to-date checks (AKA Incremental Build) in Gradle plugin
5 participants