Add gradle integration #566
Conversation
|
Thank you @vigoo, this looks great. I'll have a closer look on Monday (on vacation right now), and will fix the CI. It seems we're having some trouble with the Windows CI. |
There was a problem hiding this comment.
@vigoo Just finished adding some commits with some improvements to the code (the logic is mostly intact). Check the commit messages if you want to understand the rationale for each change and feel free to point out any incorrect thing I may have done!
Re testing: I would love to have some basic unit tests and one integration tests. For unit tests, we can extract the plugin testing API from the gradle jar that we download and add it as a test dependency. For the unit tests, I propose we replicate the same sbt configuration that with-tests (one of the projects that we use as an integration test) has in gradle. Then, what we would do is to check that the configuration file generate is the same to the one we receive from sbt (we may need to ignore order of fields like classpath because resolution is build-tool-dependent).
Another step would be to test kafka, which uses gradle. I think this integration test would complement the unit tests well because it will confirm that we can reuse most of the scala configuration in kafka's build without any user change.
| ) | ||
| ) | ||
|
|
||
| private val specsFramework = Config.TestFramework( |
There was a problem hiding this comment.
What do you think of sharing these default frameworks with MojoImplementation by adding them to the jsonConfig module? The idea would be to have a value called DefaultTestFrameworks that can be invoked by both integrations.
Another thing that we better do now here is to have a function def detectTestFrameworks(artifacts: List[Config.Artifact]): List[Config.TestFramework] that would add these only if the classpath contains the artifact associated with every framework (scalatest, specs, junit, etc).
(This last point is minor, if you don't feel like taking on it we can add a ticket so that another contributor does it 😄)
| for (sourceSet <- otherSourceSets) { | ||
| val projectName = converter.getProjectName(project, sourceSet) | ||
| // Hardcore an implicit dependency for every source set to the main source set (compile) | ||
| generateBloopConfiguration(projectName, Set(mainProjectName), sourceSet, targetDir) |
There was a problem hiding this comment.
I'm surprised that this is not present in the project configuration of gradle. Would you mind confirming that adding this dependency is necessary?
There was a problem hiding this comment.
What I found so far is:
- The
testsource set's compile classpath is extended with themainsource set's compilation output: https://github.com/gradle/gradle/blob/master/subprojects/plugins/src/main/java/org/gradle/api/plugins/JavaPlugin.java#L277 - The
testconfiguration "extends from" themainconfiguration: https://github.com/gradle/gradle/blob/master/subprojects/plugins/src/main/java/org/gradle/api/plugins/JavaPlugin.java#L427
So it's not as high level as I would expect but hopefully can be somehow used to figure out the depenencies. Will experiment with it.
There was a problem hiding this comment.
After thinking more about this, we do need to add this implicit dependency as a project dependency. As you have figured out, Gradle adds the main output dir to the test classpath, but at the project level we still need to add this dependency so that Bloop can properly schedule their compilation sequentially (if there was no dependency, Bloop would run the compilation of main and test in parallel, which would fail because test depends on main).
There was a problem hiding this comment.
Yes that was my orgiginal intention too. But it would be nice figuring out the dependencies between the Gradle source sets instead of hardcoding that all non-main depends on main.
There was a problem hiding this comment.
Yeah. That’s most likely not supported by Gradle, though... Maybe exposing a way to configure that in the gradle config would make sense.
project/BuildPlugin.scala
Outdated
| }, | ||
| ) | ||
|
|
||
| lazy val unmanagedJarsWithGradleApi: Def.Initialize[Task[Keys.Classpath]] = Def.taskDyn { |
There was a problem hiding this comment.
This is a trick I did so that it fetchGradleApi is done exactly before it's required (this is important so that IntelliJ and other tools using the output of library dependencies of a project can see the lib jar if it's not yet downlaoded).
project/Dependencies.scala
Outdated
| @@ -18,6 +18,8 @@ object Dependencies { | |||
| val mavenApiVersion = "3.5.2" | |||
| val mavenAnnotationsVersion = "3.5" | |||
| val mavenScalaPluginVersion = "3.2.2" | |||
| val gradleVersion = "4.8.1" | |||
There was a problem hiding this comment.
I took the liberty of updating this to 4.8.1, 3.0.0 is very outdated.
There was a problem hiding this comment.
I chose 3.0 because that is the oldest version compatible. By compiling it with 3.0 it is compatible with all versions above 3 including the latest 4.8.1 (I tried it). I also asked a friend who works for Gradle about this and he recommended to choose the oldest one we would like to support.
There was a problem hiding this comment.
Oh interesting! So did your friend recommend you 3.0 for testing the gradle API or for testing the plugin itself? I'm fine leaving this for 3.0, but I'd like to test the plugin with the latest gradle version so that we are closer to the versions that our users will be at.
There was a problem hiding this comment.
I think the best would be to compile with the oldest version possible to cover as much versions as we can, but always test it with the latest (or with both).
My fear of people not having the latest Gradle version may be a bit subjective as we at Prezi are still stuck with 2.13 :) But covering more versions without extra effort would no hurt, I think.
There was a problem hiding this comment.
Great, I'm happy with that. This means that we'll need to use the gradle api 3.0.0 but use the plugin unit test infrastrcuture from 4.8.1 (which means we'll need to download two gradles, unfortunately). I think it's worth it though 😄
|
Thanks for the review, I will go through all your comments tomorrow. |
|
The last support adds the required sbt magic to be able to access the project base directory for |
jvican
added
the
community
|
Whenever you add changes to the pull request, remember to rebase so that CI is faster 😉 |
| * } | ||
| * }}} | ||
| */ | ||
| final class BloopParameters(project: Project) { |
There was a problem hiding this comment.
Seems like it cannot be final, because of some unexpected Gradle magic :)
Caused by: org.gradle.api.GradleException: Could not generate a proxy class for class bloop.integrations.gradle.BloopParameters.
at org.gradle.api.internal.AbstractClassGenerator.generateUnderLock(AbstractClassGenerator.java:201)
at org.gradle.api.internal.AbstractClassGenerator.generate(AbstractClassGenerator.java:64)
at org.gradle.api.internal.ClassGeneratorBackedInstantiator.newInstance(ClassGeneratorBackedInstantiator.java:36)
at org.gradle.api.internal.plugins.DefaultConvention.create(DefaultConvention.java:103)
at bloop.integrations.gradle.syntax$ProjectExtension.createExtension(syntax.scala:42)
at bloop.integrations.gradle.BloopPlugin.apply(BloopPlugin.scala:20)
at bloop.integrations.gradle.BloopPlugin.apply(BloopPlugin.scala:17)
at org.gradle.api.internal.plugins.ImperativeOnlyPluginApplicator.applyImperative(ImperativeOnlyPluginApplicator.java:35)
at org.gradle.api.internal.plugins.RuleBasedPluginApplicator.applyImperative(RuleBasedPluginApplicator.java:43)
at org.gradle.api.internal.plugins.DefaultPluginManager.doApply(DefaultPluginManager.java:137)
... 124 more
Caused by: java.lang.VerifyError: Cannot inherit from final class
at java.lang.ClassLoader.defineClass1(Native Method)
at java.lang.ClassLoader.defineClass(ClassLoader.java:763)
at java.lang.ClassLoader.defineClass(ClassLoader.java:642)
at sun.reflect.GeneratedMethodAccessor8.invoke(Unknown Source)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.gradle.internal.reflect.JavaMethod.invoke(JavaMethod.java:75)
at org.gradle.api.internal.AsmBackedClassGenerator$ClassBuilderImpl.generate(AsmBackedClassGenerator.java:908)
at org.gradle.api.internal.AbstractClassGenerator.generateUnderLock(AbstractClassGenerator.java:199)```
|
FYI: I was playing with manually trying out the plugin with Kafka, does not work yet, will continue investigating sometime in the next days |
|
Added |
.drone.yml
Outdated
| "jsonConfig210/test" \ | ||
| "jsonConfig212/test" \ | ||
| "millBloop/compile" \ | ||
| "mavenBloop/compile" \ | ||
| "gradleBloop/compile" \ |
There was a problem hiding this comment.
Can we change this to gradleBloop/test to test the new gradle plugins in the CI?
|
@vigoo Is this working on your gradle builds? If it is, maybe we can hold off from adding the kafka integration test and merge this in 1.1.0 and label it as beta. What do you think? |
|
@jvican sorry I did not have time to work on this since my last comment but would like to continue this week. Last time I checked it worked for some example projects which are not added yet as tests, and did not work with Kafka yet. What is the planned date for 1.1.0? |
|
@vigoo 👍 We're flexible on the date, but I have the hope of releasing it in two/three weeks. We release on merge though, so as soon as we merge this Gradle users have a version they can use to test the integration and report bugs. That can be useful to improve the quality of the integration for 1.1.0+, and catch things that our tests won't. |
|
Made some progress in applying the plugin for Kafka, still not finished though.
|
This most likely has to do with Gradle class loading Scala 2.11, and then it's only normal that class loading the plugin fails, good catch.
Yes, and with the latest commit that I've added and had staged in another branch, the scala instance for an empty scala config will always be the instance for the scala version that Bloop server uses. The previous approach wasn't quite working and I did it because I kind of predicted this would be a problem for the authors of integrations 😄. Note that this is not problematic with the rest of the scala versions of your project because the scala compiler will never be run (there are no scala sources in the project). Unfortunately, because of design limitations of Zinc, we're forced to always pass in an object of
In all the other integrations, we can compile Java sources without a problem. This is, actually, mandatory to some extent because Java sources and Scala sources can have both way dependencies. Let me know if I've clarified everything! I just rebased your PR and added the commit I'm talking you about. I had to force push to rebase, so make sure you fully update your branch. Most likely, |
|
@vigoo Feel free to have a look at my latest change in the config API and the bloop configuration, this should make the CI pass again. Note that I've done a change important for semantics: the classpath and all the artifact-related variables are lists instead of sets. The reason for this is that we need to respect the order of the classpath that Gradle gives us, because the order of the classpath modifies the semantics of compilation (and Bloop wants to compile projects as similar as possible to Gradle). |
* Use logger instead of `println` * Change some of the user-facing messages and add some comments * Other minor stylistic changes (apply formatting) * Fetch the jar when doing `bloopGenerate` so that `bloop compile gradleBloop` doesn't fail if the gradle api hasn't been downloaded
1. Improve documentation: add more docs and comments. 2. Stylistic changes: mostly formatting and grouping related code. 3. Clearer names for variables (`projectDependencies` is an example). 4. Use shorter output when generating bloop configuration files. The improvements are minimal and do not touch any of the core logic. However, they change semantics for the case of `strictProjectDependencies` which should always take precedence over the normal project dependencies because in the classpath order matters.
Let's trigger the gradle API resolver whenever we ask for `unmanagedJars` so that even IntelliJ triggers the gradle API resolution and detects its dependency in `gradleBloop` (this avoids red squigglies in the gradle plugin the first time it's imported and the gradle api is not there yet).
Add the sbt infrastructure required to clone all the required integration projects that are not sbt and therefore are not handled by sbt automatically whenever the project is referenced from the build graph. This approach will clone the integration projects at the beginning of the sbt shell if they haven't been cloned. The logic to do so re-uses the one that sbt provides us via `Resolvers.git`.
…mpty and java-only source sets
Our previous approach was bogus: scala jars had always path-dependent URIs. What we need to do is to get the URI of the running bloop server so that we can create a Scala instance from the jars required for compilation.
This commit does the following things: 1. Turns `gradleBloop211` into `gradleBloop` for the moment. It seems there is no need to publish gradle-bloop 2.12? We need more investigation here, but for now relying on the version that works in the kafka build is reasonable. 2. Make bloop-config_2.11 compile and use the derivation logic from 2.12 instead of 2.11 (it's faster to compile). 3. Adjust the plugin installation to use the latest config API and to have a little bit more clear error reporting in the handling of the scala config. I've also added some more docs. Test for `gradleBloop` are passing, so this should pass the CI too.
|
I rebased everything on top of latest master that adds much faster CI turnarounds (the caching wasn't working because of a regression and was killing me). I just realized that I may have misunderstood the reason why you and I think that a 2.11 version of gradle is required. You seem to think that this is the case for Gradle projects that use Scala 2.11, and that's why you left Gradle for 2.12, and I think this is due to gradle's scala support being written in Scala 2.11 and class loaded (this would make sense because they use a very old version of think from three/four years ago). Based on this, I intuited that gradle 2.12 wasn't required and therefore removed it. @vigoo What do you think? Please let me know if gradle 2.12 is indeed required, or feel free to just add it back. By the way, try to have a look at my two commits and review them whenever you've got some time 😉 |
There was a problem hiding this comment.
I think you are right about the 2.11/2.12 question. I added two test cases to verify that, the plugin compiled with 2.11 works with Gradle projects targeting both 2.11 and 2.12.
Your other changes seems reasonable :)
About Kafka, now the plugin generates bloop project files for all subprojects, but the compilation does not work yet, I don't have more time right now to investigate.
|
@jvican I finally had some time to see what fails in Kafka, and it is still the Java-only projects, it now fails with " |
Thanks for confirming what the bug is Daniel. I'll have a look on Monday, this error may be related to a change I introduced in this branch to improve the reliability of the scala class loaders instantiation. |
There was a problem hiding this comment.
@vigoo I finally fixed the issue you reported and invested some time (and commits) in improving our previous infrastructure. Now, gradle unit tests make sure that certain invariants are met, and then we invoke bloop on the generated bloop configuration files and check for compilation.
This plugin could be improved in several ways, and I imagine that there are still some rough edges to polish. However, I think it's already more useful than having none. Help developing/improving the plugin is very much appreciated btw, given that these days I'm mostly focused on improving compilation performance.
Thanks for this contribution Daniel 👍
There was a problem hiding this comment.
@vigoo I finally fixed the issue you reported and invested some time (and commits) in improving our previous infrastructure. Now, gradle unit tests make sure that certain invariants are met, and then we invoke bloop on the generated bloop configuration files and check for compilation.
This plugin could be improved in several ways, and I imagine that there are still some rough edges to polish. However, I think it's already more useful than having none. Help developing/improving the plugin is very much appreciated btw, given that these days I'm mostly focused on improving compilation performance.
Thanks for this contribution Daniel 👍
|
I'm merging this together with some docs, feel free to improve/refine them! |
|
By the way, I didn't say anything but I got |
In that pull request, I added a new source directory scala-2.11-2.12 that was only to be used by these versions, however I forgot to update this in the sbt code that generated these files for the community build.
|
I couldn't find your handle in Twitter @vigoo, so I linked to your GH profile: |
|
Thanks for finishing and merging this, I was on vacation so could not follow :) |
This is a first stage gradle implementation (#152).
Featuring:
bloopInstallgradle task that, similarly to thesbtintegration, generates Bloop's configuration filesbloopproject extension where some customizations can be done from the Gradle build scriptmainis treated special, others always depend on itNOTE: A custom sbt task downloads Gradle and runs a dummy Gradle build compile time to generate the gradle-api artifact
I was not sure about:
so these are not included (yet).