I'm not sure if there's any additional discussion coming on this thread, but I still feel pretty strongly that (1) "more tools is not better" and (2) we are basically at a tool selection phase with leanings toward one with multi-language support (which errorprone is not). I agree with Robert's assertion that folks can use whatever tools they want locally however, I don't think we want to encourage a mixed bag of "code cleanup" type pull requests either. If there is something important that needs fixup it should be isolated and likely presented here for discussion. Any other thoughts/opinions?
On Tue, Apr 3, 2018 at 2:08 PM, Robert Dale <robd...@gmail.com> wrote: > It's definitely hard enough to get one tool done right, as demonstrated by > the broken PR, let alone trying to get several tools done right. It makes > sense for our team to choose one tool that's most broadly applicable. We > can master that one tool and use it to its full extent. Then we can > evaluate what's not covered and decide on the right tool for that job. So, > right, we as a project need to decide on what we're willing and able to > support. However, this process does not impose any limitation on what an > individual can do. Any person can choose to run any preferred tool on a > local copy of the source code, report findings, fix, etc based on that > tool. In that case, the more the merrier! > > The error-prone PR could be saved if it removed the error-prone compiler, > made some statements on why certain changes were made, and built cleanly. > > Robert Dale > > On Tue, Apr 3, 2018 at 12:57 PM, Stephen Mallette <spmalle...@gmail.com> > wrote: > > > Sure, I think that I was the one to specifically say that I didn't agree > > that "running more tools was better". Sorry, I should have said more on > > that to explain why I felt that way. > > > > Shouldn't projects consider the cost to each new tool that gets added the > > build chain? It seems undesirable to me for a project to run findbugs and > > coverity and errorprone and whatever else just because there is some > chance > > that one might find something the other doesn't at the additional cost to > > build time (i don't what that might be - maybe not an issue?) and ongoing > > maintenance. In my mind there should be some added value for each new > thing > > we add to the build chain. I don't know that the added value needs to be > > significant (that would be nice), but it should at least be explainable > in > > a definitive way (e.g. "we need to add findbugs because it covers X, Y, Z > > and errorprone does not"). I think that I'd want that kind of > explanation > > if someone moved to add a second tool after we'd chosen a first before I > > would jump on-board with such a change. > > > > To continue with the issue of tool selection while I think I can agree > that > > one tool may or may not find "everything", right now, we have no tools in > > place for this at all. It is a green field. If we should introduce a tool > > it should be one that gets us the most bang for the buck and that would > be > > one that handles multiple languages. We would focus on getting that > > particular tool into play first and then go through the process of > turning > > on the various checks we want in that tool and, as we have multiple > > languages, we'd want to do that for each language we support. Once all > that > > is done, then we would determine if there are things that aren't covered > > and whether or not additional tools are helpful or redundant. > > > > Adding stuff (i.e. "tools" but also features or new dependencies) just > > because it is available or because it provides a small incremental value > > usually doesn't feel right to me. We've haphazardly done things like this > > in the past and it's always ended up burning us which is why I'm > advocating > > for a slow and logical approach to making this happen. For example, > > sometime back, we added an appveyor build to try to ensure the project > > built on windows. But after a while we came to realize that it was false > > failing on a fairly regular basis. The failed builds basically put red > "X"s > > on all the PRs even though they were fine which made it hard to quickly > > figure out what was truly passing and what wasn't. We were dealing with > all > > this extra effort just so that if someone on windows pulled the code they > > could build TinkerPop if they wanted. We learned that having appveyor in > > the mix was a small incremental value that wasn't really worth the effort > > expended since we generally found that no one was really building on > > windows in the first place. I can't even recall the last complaint about > > that issue. > > > > Anyway, I hope that explains where I'm coming from a bit. > > > > > > > > > > > > > > > > > > On Tue, Apr 3, 2018 at 11:24 AM, Roman Leventov <leventov...@gmail.com> > > wrote: > > > > > Could somebody explain how adding a multi-language tool makes adding a > > > single-language tool undesirable, given one doesn't strictly cover > > another? > > > > > > On Tue, 3 Apr 2018, 16:36 Stephen Mallette, <spmalle...@gmail.com> > > wrote: > > > > > > > Reviving this thread a bit - a quick summary of where things were > left: > > > > > > > > 1. no one seems specifically against adding a tool of this kind > (though > > > > Florian went so far as to be positive on doing so) > > > > 2. if we did add a tool it was a good suggestion to get the tool > > selected > > > > first and then slowly enable different checks. > > > > 3. having multi-language support in the tool was brought up as a > > possible > > > > requirement in tool selection > > > > > > > > the more i've thought about it the more that I think my preference > for > > > > putting code analysis in place would involve using a tool that > provided > > > > multi-language support. Therefore, something like sonarqube or > coverity > > > > would be more appealing to me than the original JVM only solution > that > > > was > > > > proposed. Is that basically the representative feeling on this for > > > > everyone? > > > > > > > > > > > > > > > > On Wed, Mar 21, 2018 at 10:03 AM, Robert Dale <robd...@gmail.com> > > wrote: > > > > > > > > > It has multi-language support - > > > > > https://community.synopsys.com/s/question/0D53400003WcsSiCAJ/what- > > > > > languages-are-supported-by-synopsys-static-analysis-coverity > > > > > > > > > > Robert Dale > > > > > > > > > > On Wed, Mar 21, 2018 at 10:01 AM, Robert Dale <robd...@gmail.com> > > > wrote: > > > > > > > > > > > Another option might be to use Coverity - > > > > https://scan.coverity.com/faq > > > > > - > > > > > > and run it in Travis CI > > > > > > > > > > > > Robert Dale > > > > > > > > > > > > On Wed, Mar 21, 2018 at 9:43 AM, Stephen Mallette < > > > > spmalle...@gmail.com> > > > > > > wrote: > > > > > > > > > > > >> > I think any proposed changes would be separate PRs and > reviewed > > > on > > > > a > > > > > >> case-by-case > > > > > >> basis. > > > > > >> > > > > > >> yeah - i think if we want something like this the goal should > > first > > > be > > > > > to > > > > > >> decide on a tool (i can't say that i agree with Roman that > > "running > > > > more > > > > > >> tools is always better" btw). note i'm still saying, "if we want > > > > > something > > > > > >> like this" because while i'm not against it I'd like to be > > convinced > > > > > that > > > > > >> we need to worry about this now at this stage of TinkerPop's > > > > > development. > > > > > >> i'd have been more inclined to deal with this early on in > > > TinkerPop's > > > > > >> development rather than now. And, yes, also agree that i would > > like > > > to > > > > > see > > > > > >> the tool in first and then each introduction of "rule" be a > > separate > > > > > >> discussion and PR. > > > > > >> > > > > > >> > As for making it the default compiler, I think this would be a > > > > > profile, > > > > > >> disabled by default, at best at least until a time it can be > > > massaged > > > > to > > > > > >> where it works for this project. > > > > > >> > > > > > >> yeah....that might be a smart way to go > > > > > >> > > > > > >> > whether other static analysis tools would find most of the > > > problems > > > > > >> that > > > > > >> such a compiler can find and also work with other languages than > > > Java. > > > > > >> > > > > > >> very good thought. while the bulk of our code is java, we are > far > > > > from a > > > > > >> single language code base. if there is one tool that can give us > > > some > > > > > nice > > > > > >> clean analysis across the entire code base, perhaps that's the > > best > > > > way > > > > > to > > > > > >> go (again, "if we want something like this"). > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> On Tue, Mar 20, 2018 at 8:41 AM, Florian Hockmann < > > > > > f...@florian-hockmann.de > > > > > >> > > > > > > >> wrote: > > > > > >> > > > > > >> > In general, I am +1 on adding a static analysis tool to our > tool > > > > > chain. > > > > > >> > I just wonder whether a Java compiler is the best choice for > us > > > > right > > > > > >> > now or whether other static analysis tools would find most of > > the > > > > > >> > problems that such a compiler can find and also work with > other > > > > > >> > languages than Java. SonarQube[1] for example looks pretty > nice. > > > It > > > > > also > > > > > >> > supports Python, C#, and JavaScript and we can probably just > > > > integrate > > > > > >> > it via GitHub to generate reports for the release branches and > > all > > > > PRs > > > > > >> > where it would then add issues in the form of inline comments. > > > > > >> > > > > > > >> > [1] https://www.sonarqube.org/ > > > > > >> > > > > > > >> > Am 20.03.2018 um 03:42 schrieb Robert Dale: > > > > > >> > > Some changes look good others look wrong evident by the > failed > > > > > >> builds. I > > > > > >> > > think any proposed changes would be separate PRs and > reviewed > > > on a > > > > > >> > > case-by-case basis. As for making it the default compiler, > I > > > > think > > > > > >> this > > > > > >> > > would be a profile, disabled by default, at best at least > > until > > > a > > > > > >> time it > > > > > >> > > can be massaged to where it works for this project. > > > > > >> > > > > > > > >> > > On Mon, Mar 19, 2018 at 14:59 Roman Leventov < > > > > leventov...@gmail.com > > > > > > > > > > > >> > wrote: > > > > > >> > > > > > > > >> > >> See https://github.com/apache/tinkerpop/pull/819. > > > > > >> > >> > > > > > >> > >> 1) Are there any objections to using error-prone compiler > > > instead > > > > > of > > > > > >> > >> OpenJDK's javac compiler? > > > > > >> > >> 2) Stephen raised the question, that Tinkerpop project may > > > better > > > > > use > > > > > >> > >> another static analysis tool instead of error-prone. I have > > to > > > > > answer > > > > > >> > that > > > > > >> > >> no static analysis tool covers 100% of the errors found by > > any > > > > > other > > > > > >> > tool, > > > > > >> > >> so running more tools is always better. > > > > > >> > >> 3) Stephen raised the question, what checks should be > > enabled. > > > I > > > > > >> believe > > > > > >> > >> this is out of scope of the PR ( > > > > > >> > >> https://github.com/apache/tinkerpop/pull/819), > > > > > >> > >> because it doesn't (and doesn't intent to) enable any more > > > > checks > > > > > >> > beyond > > > > > >> > >> the default (= minimal) set of error patterns. More checks > > may > > > or > > > > > may > > > > > >> > not > > > > > >> > >> be enabled in later PRs and that could be discussed > > separately. > > > > > >> > >> > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > >
