> 1. bin/run_clang_tidy.sh - should we force our code to always be clang-tidy? I think so (at least if we have a subset of rules that the codebase is already clean for). It's hard to avoid noise creeping in otherwise. Kudu's approach would also work here. > 2. bin/check-rat-report.py - uses Apache RAT to check that our code has proper license headers Makes sense - we have no reason to check in code that doesn't pass RAT. > 3. Other buildall.sh options - in the past we have broken -asan, -release, or -so without breaking the pre-commit test. I'm going to echo other people's comments - I think this would be great aside from the build latency problem. If we do this, we could also enable -Werror for release and ASAN, which would cut down on warning noise. Can we fork off separate jenkins jobs to compile the code under different settings? Or somehow use ccache? > 4. Docs build It would be great to get John's input on what the docs review/merge process should look like and whether pre-commit testing for doc changes make sense. I suspect something lighter-weight than the code-review/test process would be appropriate here, and, as others have said, we could have a separate, faster, doc build rather than bundling it all into a mega-build.
On Tue, Nov 29, 2016 at 11:21 AM, Jim Apple <[email protected]> wrote: > At the moment, we have not added anything to our .clang-tidy config > file that doesn't apply to the whole codebase. That is to say, we are > clang-tidy green (but for a recent errant semicolon after a member fn > decl). > > On Tue, Nov 29, 2016 at 11:18 AM, Todd Lipcon <[email protected]> wrote: > > On Kudu we've hooked it up as a precommit, but without any "vote". That > is > > to say, it will add a gerrit comment with any warnings/diagnostics, but > > doesn't add a '-1' which prevents merge. This has been useful for > pointing > > out style issues or missed optimizations, but we've been able to adopt it > > gradually without a big-bang "tidy everything" kind of commit which can > be > > disruptive to in-flight patches. > > > > Typically we expect patch contributors to address the review comments > from > > clang-tidy just like they'd address review comments from a human reviewer > > (which might include ignoring the comment if it's just some code that was > > moved from one place to another rather than new code) > > > > Here's an example of the type of comments it leaves: > > https://gerrit.cloudera.org/#/c/5252/1/src/kudu/master/ > catalog_manager.cc@3296 > > > > -Todd > > > > On Tue, Nov 29, 2016 at 10:06 AM, Jim Apple <[email protected]> > wrote: > > > >> Also, individual warning can be suppressed with "// NOLINT" (or with > >> "#pragma clang diagnostic ignored" for tidy checks that are also > >> compiler warnings) > >> > >> On Tue, Nov 29, 2016 at 10:01 AM, Sailesh Mukil <[email protected]> > >> wrote: > >> > On Tue, Nov 29, 2016 at 9:50 AM, Henry Robinson <[email protected]> > >> wrote: > >> > > >> >> On 29 November 2016 at 08:06, Jim Apple <[email protected]> > wrote: > >> >> > >> >> > Should we add to our pre-merge testing (aka GVM, aka GVO) some > tests > >> >> > that don't run impalad, but only build it or check for correctness? > >> >> > > >> >> > For instance: > >> >> > > >> >> > 1. bin/run_clang_tidy.sh - should we force our code to always be > >> >> > clang-tidy? > >> >> > > >> >> > >> >> I don't have enough experience of the tool to know if this likely to > be > >> a > >> >> help or hindrance. > >> >> > >> >> > >> >> > >> > +1 for this. My opinion is unless we foresee some patches that would > fail > >> > clang-tidy but still be considered a valid patch by us, we should have > >> this > >> > as a pre-commit test. > >> > > >> >> > >> >> > 2. bin/check-rat-report.py - uses Apache RAT to check that our code > >> >> > has proper license headers > >> >> > > >> >> > >> >> +1 > >> >> > >> >> > >> >> > > >> >> > 3. Other buildall.sh options - in the past we have broken -asan, > >> >> > -release, or -so without breaking the pre-commit test. > >> >> > > >> >> > >> >> If all can be tested for 'free' wrt to wall-clock-time, then sure. > But > >> if > >> >> that's not possible, I'd only consider building -release, and maybe > not > >> >> even that. -asan failures are infrequent enough that I don't expect > it > >> to > >> >> be worth the extra time it would add to the pre-commit build. > >> >> > >> >> -so is less important to me. > >> >> > >> >> > >> >> > > >> >> > 4. Docs build > >> >> > > >> >> > I think I can do these without increasing the end-to-end time it > takes > >> >> > to run the tests, by doing them in parallel. One issue I see is > that > >> >> > committers who see their change as minor and merge it manually, > >> >> > without pre-merge testing, might break clang-tidy or RAT tests. > >> >> > > >> >> > >> >> For that reason, perhaps a separate docs build makes the most sense. > >> >> > >> > > > > > > > > -- > > Todd Lipcon > > Software Engineer, Cloudera >
