Adar Dembo has posted comments on this change. Change subject: KUDU-2066. Add experimental Gradle build support ......................................................................
Patch Set 16: (14 comments) http://gerrit.cloudera.org:8080/#/c/7258/16/java/build.gradle File java/build.gradle: File-level comment for the purpose of this file ("This is the entry-point for the gradle build, yada yada yada")? http://gerrit.cloudera.org:8080/#/c/7258/16/java/gradle.properties File java/gradle.properties: File-level comment here. http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/compile.gradle File java/gradle/compile.gradle: Line 24: options.incremental = true // enable incremental compilation. > Done I didn't mean to suggest it should be removed. I just didn't know whether it's good or bad; there was no information to help me understand why it's a good thing. Line 31: } > I am not sure I know how to do this in the maven build. Mainly the ability You'd have to add a Maven profile. See https://stackoverflow.com/a/16743137. http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/dependencies.gradle File java/gradle/dependencies.gradle: PS14, Line 60: . > Perhaps add some checking just beforehand to ensure that the scalaVersions Missed this? Line 64: versions["scalaBase"] = versions.scala.substring(0, versions.scala.lastIndexOf(".")) > I thought this made the below logic more explicit and readable. No problem, I can buy that. http://gerrit.cloudera.org:8080/#/c/7258/16/java/gradle/publishing.gradle File java/gradle/publishing.gradle: Line 37: // To test locally, replace mavenUrl in gradle.properties to file://localhost/tmp/myRepo/ Now that mavenUrl uses propertyWithDefault(), is this guidance necessary? I think -D or -P is better than modifying gradle.properties. http://gerrit.cloudera.org:8080/#/c/7258/16/java/gradle/quality.gradle File java/gradle/quality.gradle: PS16, Line 22: fro for http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/wrapper.gradle File java/gradle/wrapper.gradle: Line 28: // This allows us to avoid checking in the jar to our repository. > Otherwise this code is run during project configuration and not when the ta Could you add a comment explaining that? http://gerrit.cloudera.org:8080/#/c/7258/16/java/gradle/wrapper.gradle File java/gradle/wrapper.gradle: PS16, Line 27: doesn"t Single quote was better here. http://gerrit.cloudera.org:8080/#/c/7258/14/java/kudu-mapreduce/build.gradle File java/kudu-mapreduce/build.gradle: Line 17: > It could be a bug, but in the maven build we don't shade these jars. Happy Hmm, ask Dan? Line 23: } > There isn't a super clean way to do this. What I did instead was create a p Nice. Can you add a link to the pull request in each place where you apply this pattern? That way we'll be more likely to notice and remove it once async merges the pull request. http://gerrit.cloudera.org:8080/#/c/7258/16/java/kudu-spark/pom.xml File java/kudu-spark/pom.xml: PS16, Line 104: <args> : <arg>-unchecked</arg> : <arg>-deprecation</arg> : <arg>-explaintypes</arg> : </args> I didn't see this in the gradle build. Is it built-in to gradle's scala plugin? http://gerrit.cloudera.org:8080/#/c/7258/16/java/settings.gradle File java/settings.gradle: File-level comment. -- To view, visit http://gerrit.cloudera.org:8080/7258 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib257cdd019a1f383c886b9238bb47d96576c4421 Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
