[kudu-CR] java: enable error-prone for java builds

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/4425 )

Change subject: java: enable error-prone for java builds
..

java: enable error-prone for java builds

error-prone (http://errorprone.info/) is a compiler extension by Google
which checks for a bunch of common Java errors.

Fixes for the exposed issues are in follow-on commits.

Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Reviewed-on: http://gerrit.cloudera.org:8080/4425
Reviewed-by: Grant Henke 
Tested-by: Kudu Jenkins
---
M java/buildSrc/build.gradle
M java/gradle/dependencies.gradle
M java/gradle/quality.gradle
3 files changed, 19 insertions(+), 0 deletions(-)

Approvals:
  Grant Henke: Looks good to me, approved
  Kudu Jenkins: Verified

--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 8
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] java: enable error-prone for java builds

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/4425 )

Change subject: java: enable error-prone for java builds
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4425/4/java/gradle/quality.gradle
File java/gradle/quality.gradle:

http://gerrit.cloudera.org:8080/#/c/4425/4/java/gradle/quality.gradle@76
PS4, Line 76: tasks.withType(JavaCompile) {
> I like to have all the levers for control at the task level and often we us
Yea, that's why I think it's important to pin a specific version as I did here, 
so we don't have previously-working builds spontaneously break.

FWIW, Google uses error-prone by default instead of javac for all of their 
builds, so there's at least some evidence that it's stable and usable as a 
general purpose replacement. Open source bazel also does this -- java targets 
always use error-prone for javac.

Not sure about the build time overhead. It seemed to not be much slower as far 
as I could notice.



--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 26 Apr 2018 17:42:34 +
Gerrit-HasComments: Yes

[kudu-CR] java: enable error-prone for java builds

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/4425 )

Change subject: java: enable error-prone for java builds
..


Patch Set 7:

Fixed my accidental rebase back onto the old patch.


--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 26 Apr 2018 17:26:03 +
Gerrit-HasComments: No

[kudu-CR] java: enable error-prone for java builds

[Grant Henke (Code Review)]

Grant Henke has uploaded a new patch set (#7) to the change originally created 
by Todd Lipcon. ( http://gerrit.cloudera.org:8080/4425 )

Change subject: java: enable error-prone for java builds
..

java: enable error-prone for java builds

error-prone (http://errorprone.info/) is a compiler extension by Google
which checks for a bunch of common Java errors.

Fixes for the exposed issues are in follow-on commits.

Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
---
M java/buildSrc/build.gradle
M java/gradle/dependencies.gradle
M java/gradle/quality.gradle
3 files changed, 19 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/4425/7
--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] java: enable error-prone for java builds

[Grant Henke (Code Review)]

Grant Henke has uploaded a new patch set (#6) to the change originally created 
by Todd Lipcon. ( http://gerrit.cloudera.org:8080/4425 )

Change subject: java: enable error-prone for java builds
..

java: enable error-prone for java builds

error-prone (http://errorprone.info/) is a compiler extension by Google
which checks for a bunch of common Java errors.

Fixes for the exposed issues are in follow-on commits.

Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
---
M java/build.gradle
M java/gradle/dependencies.gradle
M java/gradle/quality.gradle
3 files changed, 24 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/4425/6
--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] java: enable error-prone for java builds

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/4425 )

Change subject: java: enable error-prone for java builds
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4425/4/java/gradle/quality.gradle
File java/gradle/quality.gradle:

http://gerrit.cloudera.org:8080/#/c/4425/4/java/gradle/quality.gradle@76
PS4, Line 76: tasks.withType(JavaCompile) {
> that's sort of the design of error-prone. It's actually a subclass of the J
I like to have all the levers for control at the task level and often we use 
certain tasks for "testing" vs "packaging".

My concern is if the static code analysis complicates or breaks longer running, 
assembly only, packaging type jobs. For those we tend to use `gradle assemble` 
instead of `gradle check` or `gradle build`. If it does't break things, it 
likely makes the build longer.



--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 26 Apr 2018 01:19:48 +
Gerrit-HasComments: Yes

[kudu-CR] java: enable error-prone for java builds

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/4425 )

Change subject: java: enable error-prone for java builds
..


Patch Set 5: Code-Review+1

Looks fine, thanks


--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 25 Apr 2018 22:30:18 +
Gerrit-HasComments: No

[kudu-CR] java: enable error-prone for java builds

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/4425 )

Change subject: java: enable error-prone for java builds
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4425/4/java/gradle/quality.gradle
File java/gradle/quality.gradle:

http://gerrit.cloudera.org:8080/#/c/4425/4/java/gradle/quality.gradle@76
PS4, Line 76:   signature 'org.codehaus.mojo.signature:java17:1.0@signature'
> I am not a fan that this needs to run all the time when compiling as oppose
that's sort of the design of error-prone. It's actually a subclass of the Java 
compiler that hooks into the compilation process itself, rather than doing 
post-compilation bytecode analysis.

I suppose it could be an optional flag, but I kind of like that it will break 
your compile for certain classes of errors



--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 25 Apr 2018 22:29:01 +
Gerrit-HasComments: Yes

[kudu-CR] java: enable error-prone for java builds

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/4425 )

Change subject: java: enable error-prone for java builds
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4425/3/java/build.gradle
File java/build.gradle:

http://gerrit.cloudera.org:8080/#/c/4425/3/java/build.gradle@24
PS3, Line 24:   id 'net.ltgt.errorprone' version '0.0.13' apply false
> unfortunately I couldn't move the 'plugins' definition anywhere but here. I
This is just adding the dependencies to the build. I have been doing that in 
the /buildsrc repo to prevent this issue. I will post an update to this patch 
showing that.


http://gerrit.cloudera.org:8080/#/c/4425/4/java/gradle/quality.gradle
File java/gradle/quality.gradle:

http://gerrit.cloudera.org:8080/#/c/4425/4/java/gradle/quality.gradle@76
PS4, Line 76: tasks.withType(JavaCompile) {
I am not a fan that this needs to run all the time when compiling as opposed to 
as a separate task like findbugs, pmd, etc. I am not sure that is avoidable and 
can follow up and improve this if it is.



--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 25 Apr 2018 22:27:02 +
Gerrit-HasComments: Yes

[kudu-CR] java: enable error-prone for java builds

[Grant Henke (Code Review)]

Grant Henke has uploaded a new patch set (#5) to the change originally created 
by Todd Lipcon. ( http://gerrit.cloudera.org:8080/4425 )

Change subject: java: enable error-prone for java builds
..

java: enable error-prone for java builds

error-prone (http://errorprone.info/) is a compiler extension by Google
which checks for a bunch of common Java errors.

Fixes for the exposed issues are in follow-on commits.

Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
---
M java/buildSrc/build.gradle
M java/gradle/dependencies.gradle
M java/gradle/quality.gradle
3 files changed, 19 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/4425/5
--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] java: enable error-prone for java builds

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/4425 )

Change subject: java: enable error-prone for java builds
..


Patch Set 3:

(3 comments)

As for error-prone vs spotbugs, I think they are likely complementary with a 
different set of checks. I personally like that error-prone produces its output 
as warnings in the normal compile output in the normal flow of development

http://gerrit.cloudera.org:8080/#/c/4425/3/java/build.gradle
File java/build.gradle:

http://gerrit.cloudera.org:8080/#/c/4425/3/java/build.gradle@24
PS3, Line 24:   id 'net.ltgt.errorprone' version '0.0.13' apply false
> I have been putting all of these static analysis checks into quality.gradle
unfortunately I couldn't move the 'plugins' definition anywhere but here. It 
has to be the first thing in the build file apparently


http://gerrit.cloudera.org:8080/#/c/4425/3/java/build.gradle@48
PS3, Line 48:   apply plugin: 'java'
> Any reason you changed this to single quotes?
mistake, reverted


http://gerrit.cloudera.org:8080/#/c/4425/3/java/build.gradle@51
PS3, Line 51:   apply from: "$rootDir/gradle/errorprone.gradle"
> Did you add this file? I have been putting all of these static analysis che
moved the contents to quality.gradle



--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 25 Apr 2018 22:06:24 +
Gerrit-HasComments: Yes

[kudu-CR] java: enable error-prone for java builds

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Jean-Daniel Cryans, Kudu Jenkins, Grant Henke,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/4425

to look at the new patch set (#4).

Change subject: java: enable error-prone for java builds
..

java: enable error-prone for java builds

error-prone (http://errorprone.info/) is a compiler extension by Google
which checks for a bunch of common Java errors.

Fixes for the exposed issues are in follow-on commits.

Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
---
M java/build.gradle
M java/gradle/dependencies.gradle
M java/gradle/quality.gradle
3 files changed, 24 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/4425/4
--
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-Change-Number: 4425
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] java: enable error-prone for java builds

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: java: enable error-prone for java builds
..


Patch Set 2:

> 23:08:57 [ERROR] Failed to execute goal 
> org.apache.maven.plugins:maven-compiler-plugin:3.3:compile (default-compile) 
> on project interface-annotations: Execution default-compile of goal 
> org.apache.maven.plugins:maven-compiler-plugin:3.3:compile failed: An API 
> incompatibility was encountered while executing 
> org.apache.maven.plugins:maven-compiler-plugin:3.3:compile: 
> java.lang.UnsupportedClassVersionError: javax/tools/DiagnosticListener : 
> Unsupported major.minor version 52.0

Something was compiled targeting JDK 8, but it's hard to tell exactly what.

-- 
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] java: enable error-prone for java builds

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: java: enable error-prone for java builds
..


Patch Set 2: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No