Attempt to keep Kotlin totally off the classpath #1558
Conversation
There was a problem hiding this comment.
Would this change be a breaking change? If that is the case, can't we make this approach work like the one with contractTests, which means that the old approach is deprecated and we would remove it in 4.0.0.
BTW Have you run this against the samples?
Also, what do you think of actually making the current build.gradle setup in the samples a specific build-workshop.gradle and making the samples also a multimodule gradle build? Would you be interested in making such a change?
| @@ -0,0 +1,38 @@ | |||
| package org.springframework.cloud.contract.verifier.plugin; | |||
There was a problem hiding this comment.
We need to add the proper header to each file
/*
* Copyright 2013-2020 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/There was a problem hiding this comment.
Whoops missed one!
| @@ -97,6 +107,8 @@ public void apply(Project project) { | |||
| createGenerateTestsTask(extension, contractTestSourceSet, copyContracts); | |||
| createAndConfigurePublishStubsToScmTask(extension, generateClientStubs); | |||
|
|
|||
| project.getDependencies().add(CONTRACT_TEST_GENERATOR_RUNTIME_CLASSPATH_CONFIGURATION_NAME, "org.springframework.cloud:spring-cloud-contract-converters:" + SPRING_CLOUD_VERSION); | |||
There was a problem hiding this comment.
Why do we have to add this?
There was a problem hiding this comment.
So the reason for this is because end users who apply the sc-contract plugin don't typically have the converters module on the classpath and the RecursiveFilesConverter lives there.
While definitely could keep this on the classpath as it is a direct dependency (would probably need to add back it's transitives), it seemed like a good idea to keep everything isolated to ensure conflicts couldn't arise via the converters module either.
There was a problem hiding this comment.
But if it's required by the plugin in general then maybe we should add it to the plugin's classpath directly?
There was a problem hiding this comment.
So since this seems to still be a little confusing (understandably), let me try to illustrate it this way.
sc-contract-converters has direct dependencies on Groovy and sc-contract-spec. Since we know the Kotlin support is fairly isolated away in sc-contract-spec-kotlin, it's not such a big deal for the Kotlin aspect, but because it is in this same area it could be possible that at some point in the future Kotlin needs to be added as a direct dependency of sc-contract-spec. If that were to happen Kotlin DSL based Gradle builds would likely break again in EXACTLY the same way that they are broken today. The other facet of this relates to Groovy. While Spring Cloud version aligns with Spring Boot and furthermore Spring Framework when it comes to both Groovy and Kotlin versions, Gradle does not version align with any of those. So when Gradle inevitably upgrades to Groovy 3 for the Groovy DSL build.gradle scripts it is likely sc-contract will break from a Groovy standpoint instead of a Kotlin one as is the example today.
As you mentioned already, the plugin needs to have sc-contract-converters to perform it's job. With Gradle we declare what we need via configurations (generally dependencies). We do have options for how to declare those dependencies based on the needs that we have. The typical way for a plugin to perform it's activities is to include the necessary dependencies on its own implementation or api configurations as needed. If however we need greater isolation, we can create a configuration in the end users project model which we can either hide from them OR provide them the ability to contribute necessary dependencies to. Here we are performing the later because we need to become isolated away from Gradle itself, so that it's internal dependencies can not effect our classpath and our classpath can't conflict with Gradle's internal/parent classpath (that will contain Groovy and Kotlin + various other libraries Gradle uses).
It is good to note that a configuration that we create is by default isolated until you declare an extendsFrom. Once an extendsFrom exists that configuration now applies dependency version rules as normal across the full set of all dependencies of the current configuration plus all dependencies included with our configuration from transitive configurations. (Extended from configurations won't see our dependencies though, so our dependencies can't impact a user).
So in this specific case, we created the contractTestGeneratorRuntimeClasspath where we gain all of the test and contractTest dependencies and then ADD a dependency on sc-contract-converters, so that our plugin can use that total and isolated classpath for running it's JavaExec commands.
To give this a Maven parallel, it's very similar to how plugins allow you to add additional dependencies to themselves and Maven will fork the plugin execution to an independent Java process. Same concept here.
Hope that this clears some things up. :)
| @@ -71,6 +77,10 @@ | |||
|
|
|||
| private static final String CONTRACT_TEST_RUNTIME_ONLY_CONFIGURATION_NAME = "contractTestRuntimeOnly"; | |||
|
|
|||
| private static final String CONTRACT_TEST_RUNTIME_CLASSPATH_CONFIGURATION_NAME = "contractTestRuntimeClasspath"; | |||
There was a problem hiding this comment.
Shouldn't this extend from contractTest ?
There was a problem hiding this comment.
So contractTestRuntimeClasspath is automatically created and configured to extend all contractTest configurations (contractTestImplementation, etc), so we are using this here simply as a way to gain access to that resolvable configuration that will contain all of the end users runtime classpath elements so that we can extend it one level further to re-add sc-converters back for our purposes.
|
From a breaking change standpoint, I'm pretty sure that the answer is no. There is however some cleanup that end users should do to ensure a clean state long term. I did run through a chunk of the samples (all were passing), but did not get them all completely tested last night. Since I had the code done I wanted to get it out there for discussion as a main objective. Some of the samples don't play nicely with Windows, but I've come to expect that. A s far as the samples go, I have no problem with making them a multi-module build using current idiomatic Gradle configuration or staying aligned with what you all have already. Not sure I follow on the As to the changes and how they relate to a potential breaking change, or lack thereof, I'll break if down like this:
Hopefully that sheds some light on the impact of this from both the authoring and end-user perspectives. |
|
To additionally note, the only time that there could become a breaking change this way is if the end user is a consumer and is applying the sc-contract Gradle plugin erroneously. In this case sc-contract-verifier would not be on their Based on the documentation and samples, the user is attempted to be guided away from doing exactly this, but that doesn't prevent the end user from incorrectly using the plugin. We could also handle this case by adding a version aligned sc-contract-verifier module just like we did for sc-contract-converters. |
Good!
Sorry about that :(
So in the samples repo, current
Yup, that looks great, thanks so much!
We can't support all possible use cases... If someone uses the plugin in a wrong way then we can't do much about it.
Theoretically that way we would stop having the problem with Groovy version mismatch, right? If it's all about adding a line of code to add another library then maybe it's a good idea? |
Not quite sure what you mean with the Groovy version mismatch. Haha. I know we had one with Kotlin and test generation (producer). The previous quoted statement was more in line with preventing a build from bombing out because of a |
I mean that due to the fact that in this solution we're creating a separate classpath without kotlin for the java exec task, then we can automatically create one without groovy that comes from Gradle. Anyways, I don't like that you have to do it like this with Gradle, but I guess this is the way to go. |
|
@marcingrzejszczak, gotcha. I definitely agree that we shouldn't have to go to a fully forked process in order to gain the necessary isolation... Usually the worker api is enough to get the desired isolation, but this mechanic doesn't work for the Groovy or Kotlin versions. |
|
Ok, all of the in-repo samples are successful locally using both Gradle and Maven. @OlgaMaciaszek let me know that there are some issues with the other samples repository, so that'll be next on my chopping block. |
| dependencyManagement { | ||
| imports { | ||
| mavenBom "org.springframework.cloud:spring-cloud-dependencies:$BOM_VERSION" | ||
| mavenBom "org.springframework.cloud:spring-cloud-contract-dependencies:${project.findProperty('verifierVersion') ?: verifierVersion}" |
There was a problem hiding this comment.
Since I expect for this to come up probably... this elvis operator is actually redundant, so that's why I simplified it below.
If you pass a -P argument, it automatically overrides whatever is in your gradle.properties file with no further action necessary. So both findProperty('verifierVersion') and verifierVersion in this case are equivalent (and ironically, we can never hit the second case because verifierVersion is in gradle.properties). If verifierVersion did not exist in our gradle.properties file, we would want to solely use findProperty('verifierVersion') (again without the elvis), because this gives us null safe protection for Gradle properties. However, when the context of the property's usage is taken into account then null is an invalid value, thus verifierVersion is much simplier and provides a simplier, more readable codebase.
|
Ok, the current build failure appears to be related to versions in the pom.xml. Since I'm using Gradle's |
|
So we're waiting for this PR to get merged to deal with spring-cloud-samples/spring-cloud-contract-samples#177 (comment) ? |
|
@marcingrzejszczak, yes. I would have linked it, but I'm on mobile which is a little hard to do. Haha I'll be honest, that the error in that ticket seems to be a quirk possibly in the Kotlin JVM plugin due to the application not having Kotlin sources for that SourceSet. Once this is merged though, we'll be able to get a better picture of what's going on though. |
…throughout plugin and samples
|
Ok, so I've got everything running locally consistently following along exactly (minus tests because Windows) with the It does make me wonder if there happens to be a snapshot stored in the |
|
So with these changes have you managed to successfully run the Gradle build for Kotlin samples in |
|
@marcingrzejszczak, let me rephrase a little bit. For the NOTE: skipping tests here because some of them are sensitive on Windows as we've discussed previously and from the CI environment we can see all tests are passing. Building and testing the in repository samples is the last step of the CI process. And as further clarification, a number of the Kotlin samples contained within |
|
Sure, thanks a lot for the explanation :) Currently we have the biggest problems with |
|
@marcingrzejszczak, I'll make a pass through the Just and occurring thought, will the |
|
Great, thanks! Just check the spring cloud contract samples |
…ons less than 6.2
|
|
@marcingrzejszczak, yeah, I got fed up trying to tease out the issue, so just let stderr be printed directly (didn't want to use debug so as to not leak your all's sensitive credentials and such if there are any). I'm pretty sure this is as a result of the quoting of the command line arguments. I'm going to do some experimentation on this to try to expand on that idea. |
|
@marcingrzejszczak, so finally figured it out and it happens to be a "quirk" with Java itself... Basically on Windows double quotes are automatically removed unless they are escaped and on Linux they should be left alone and escaping them breaks the process. I did manage to stumble across this ticket in the Gradle GitHub issues and I noted it alongside the code change to hopefully fix this for good. Posting here as well: gradle/gradle#6072 NOTE: I am still going through the |
|
Still running through the samples, but it seems like I've got to add back aether to the mix, but that shouldn't be hard to accomplish and get sorted out. Will just need to get that sorted out before we merge this, so as to not let that aspect be dropped during this process. I've gotten through about 50% of the producers (with some issues related to Windows) and everything has been passing perfectly so far. |
|
@marcingrzejszczak, I've managed to complete all current producer and consumer samples contained within I'm going to start messing around specifically with the Kotlin samples at this time. NOTE: All Kotlin samples in |
This addresses the broken Kotlin support by keeping all things sc-contract in a totally isolated classloader (via process forking). This is the same pattern employed by the Kotlin plugin when attempting to perform Kotlin compilation (albeit much easier in our case).