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. > > > > >> > >> > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > > > > > > > > > > > > > > > >
