apache
ZOOKEEPER-3000: Use error-prone compiler
Also would you please elaborate a little bit more on reasoning in Jira's description for the records?
Hi @leventov, in the commits, there are test code changes, too. Are these related to the error prone compiler change, or these are just other bugs which you've fixed?
However we already integrated findbugs tool in our build process, don't you think it would be better to replace that instead of using two similar tools side-by-side?
@anmolnar no, static analysis tools complement each other. Bugs found by one tool are not found by another, and vice versa. So it's good to run them all.
Additionally I think it would be better for reviewers to split this PR into 1) integration of new static analyis tool, 2) test fixes.
Test problems are catched by the tool. If the tool is integrated first, the compilation will fail briefly. So I don't see the point of the separation. The whole PR is not that big, after all.
Could we add the error prone compiler as an optional tool, so that similarly to e.g. code coverage tools, the build could be run with or without the tool?
@mfenes I'm not proficient in Ant, I managed to run the tool the simplest way possible that I could figure out. If you could implement this so that the tool is optional, you could do this. I don't know how to do this.
in the commits, there are test code changes, too. Are these related to the error prone compiler change, or these are just other bugs which you've fixed?
All changes in this PR are imposed by error-prone only.
@leventov I've just verified your patch on my local machine.
Is Java8 a requirement for error-prone to work?
Because running ant clean test with java7 I got the following error message:
BUILD FAILED /Users/andor/git/my-zookeeper/build.xml:1417: The following error occurred while executing this line: /Users/andor/git/my-zookeeper/build.xml:490: Class com.google.errorprone.ErrorProneAntCompilerAdapter could not be loaded because of an invalid dependency.
If that's the case, I'm afraid we can only merge this patch to master, once we upgraded to java8 which is coming soon, but not there yet.
Additionally running with Java8 I got 75 warning messages. Is that accurate?
@anmolnar see http://errorprone.info/docs/installation, the first paragraph: "Please note that Error Prone must be run on JDK 8 or newer. It can be used to build Java 6 or 7 code by setting the appropriate -source / -target / -bootclasspath flags."
@leventov Got that, however my issue still stands: why ant test command doesn't compile with java7?
Is it just me who experience it? Have you tested the patch with java7?
@anmolnar if you try to run ant test while the test are not compiled yet (i. e. they have to be compiled), and Ant uses Java 7 which is installed on your machine for compilation, yes, this is expected that it will fail. You would need to run with Java 8.
However, I see that javac.source and javac.target in build.xml already set to 1.8, how do you test it with Java 7 anyway?
We just upgraded master and 3.5 branches to Java 8 a few days ago. I wrote my comment almost a month back. In which case your patch is targeted to master and 3.5 branches only and won't be merged to 3.4. Is that okay for you?
This is a good addition, thanks for bringing to our attention @leventov .
Unfortunately however we can't replace the std javac compiler with error_prone in the toolchain outright - we can have a separate option/target to do this, but it can't be the default.
I would recommend separating this into two jiras/pullrequests - one to change the build, the other to address the found issues.
@leventov you mentioned "I'm not proficient in Ant" - perhaps one of the other folks could help with this? @anmolnar you mentioned hbase is using it - anything we can pull from there wrt how they integrate it into their build?
ps. I like the idea of also getting this into 3.4 branch as well, which means it needs to be optional so that the user can run it when they have java8+, but otw not. Which matches my intent/statement above.
pps. looks like there is a new version of error_prone, might want to take this opportunity to update the dependency version. I notice the library is ASL2.0 which is also good (license is in the COPYING file currently)
