impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Apple <jbap...@cloudera.com>
Subject Re: Tests that don't run impalad
Date Tue, 29 Nov 2016 19:21:53 GMT
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 <todd@cloudera.com> 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 <jbapple@cloudera.com> 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 <sailesh@cloudera.com>
>> wrote:
>> > On Tue, Nov 29, 2016 at 9:50 AM, Henry Robinson <henry@apache.org>
>> wrote:
>> >
>> >> On 29 November 2016 at 08:06, Jim Apple <jbapple@cloudera.com> 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

Mime
View raw message