[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 2:

Filed https://issues.apache.org/jira/browse/IMPALA-7319 to track this.


--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 18 Jul 2018 23:34:38 +
Gerrit-HasComments: No

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 2:

Here are my thoughts on this CR:
1. I'm going to file a JIRA about clang tidy diff. Even if we don't pursue 
this, there should be a record for later. I will incorporate that JIRA on this 
CR.
2. Clang tidy diff is dramatically faster than run_clang_tidy.sh, but it is 
also not 100% correct. There are code changes that can introduce clang tidy 
issues in code that hasn't changed. Unless we are comfortable with that, clang 
tidy diff won't replace run_clang_tidy.sh.
3. For automated jobs (like clang-tidy-ub1604 and whatever happens for 
IMPALA-7317), speed only matters so much.
4. I think some improvements to run_clang_tidy.sh to be able to run clang tidy 
on a list of files would cut down on the cost to run locally.


--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 18 Jul 2018 22:19:03 +
Gerrit-HasComments: No

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Tim Armstrong (Code Review)]

Tim Armstrong has abandoned this change. ( http://gerrit.cloudera.org:8080/9751 
)

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Abandoned

I think this is now covered by IMPALA-7317 so I'll abandon for now. Restore if 
you disagree.
--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 2:

Now that the clang-tidy quiet patch is in, is this unblocked?


-- 
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 17 Jul 2018 15:31:38 +
Gerrit-HasComments: No

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 2:

Any updates on this?


--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 05 Jun 2018 21:54:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 2:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/9751/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9751/2//COMMIT_MSG@7
PS2, Line 7: experimental
Can you file a ticket for this, and post a link to this on there? That way, 
even if this patch doesn't land, they'll be a searchable record of this patch 
for the next person to play with.


http://gerrit.cloudera.org:8080/#/c/9751/2//COMMIT_MSG@29
PS2, Line 29: the clang-tidy diff
I don't think I understand this phrase. What thing includes the comments? The 
output of the tool, or some source code file in this patch?


http://gerrit.cloudera.org:8080/#/c/9751/2/bin/clang_tidy_diff.py
File bin/clang_tidy_diff.py:

http://gerrit.cloudera.org:8080/#/c/9751/2/bin/clang_tidy_diff.py@57
PS2, Line 57: def run_tidy(sha="HEAD", is_rev_range=False):
Could you add docstrings for functions in this file?


http://gerrit.cloudera.org:8080/#/c/9751/2/bin/clang_tidy_diff.py@91
PS2, Line 91: m = re.match(r"^(.+?):(\d+):\d+: ((?:warning|error): 
.+)$", w, re.MULTILINE | re.DOTALL)
nit: long line, here and elsewhere. You might find 
https://pypi.python.org/pypi/pep8 useful. I use 
https://pypi.python.org/pypi/autopep8 with emacs and

(defun pep8-region ( b e)
  (interactive "r")
  (call-process-region b e
   "/home/jbapple/.local/bin/autopep8" t t nil
   "--indent-size=2" "--max-line-length=90" "-a" "-a"
   "-a" "-a" "-a" "-a" "-a" "-a" "--experimental" "-"))


http://gerrit.cloudera.org:8080/#/c/9751/2/bin/clang_tidy_diff.py@139
PS2, Line 139: class TestClangTidyGerrit(unittest.TestCase):
Does anything exercise this?


http://gerrit.cloudera.org:8080/#/c/9751/2/bin/clang_tidy_diff.py@178
PS2, Line 178: help="Whether the revision specifies the 
'rev..' range")
What does that mean?


http://gerrit.cloudera.org:8080/#/c/9751/2/bin/tidy-diff.sh
File bin/tidy-diff.sh:

http://gerrit.cloudera.org:8080/#/c/9751/2/bin/tidy-diff.sh@28
PS2, Line 28: LAST_COMMITTED_REV=$($ROOT/bin/get-upstream-commit.sh)
Can you make it so this can be explicitly set in the CLI call to tidy-diff.sh?



--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sun, 01 Apr 2018 03:10:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG@22
PS1, Line 22: The main change is to find a way to get the
> I like your last suggestion as an alternative to doing #3 first. Maybe it w
Changed the names of the clang-tidy diff scripts and make targets to make the 
distinction clearer. Added some comments in clang_tidy_diff.py and 
run_clang_tidy.sh to say clearly what they are doing.

I'm looking into compilation flags we can enable. I will do a run with several 
flags disabled and see what pops.



--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 26 Mar 2018 23:40:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Joe McDonnell (Code Review)]

Hello Jim Apple, Philip Zeyliger, Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9751

to look at the new patch set (#2).

Change subject: [experimental] Clang Tidy Diff trial balloon
..

[experimental] Clang Tidy Diff trial balloon

Kudu has a nice script that does clang tidy on
a diff rather than the whole source code. Kudu
uses this for a Tidy Bot that comments on Gerrit
reviews if the code change introduces an issue.

This is an experimental change to bring those
scripts over and adapt them to Impala. The
Kudu scripts are mostly unchanged from the
Kudu versions. The changes are:
 - The scripts are in ${IMPALA_HOME}/bin,
   requiring corresponding path fixups.
 - clang_tidy_gerrit.py has been renamed to
   clang_tidy_diff.py to make a distinction
   between this clang-tidy functionality
   and the existing run_clang_tidy.sh. For the same
   reason, tidy.sh is now tidy-diff.sh and the
   Kudu compile target "make tidy" is now
   "make tidy-diff".
 - bin/clang_tidy_diff.py use Impala's locations
   for clang-tidy and clang-tidy-diff.py
 - All of the clang-tidy diff includes comments
   indicating it is experimental.

The main change is to find a way to get the
appropriate compilation flags for clang. This is
hard-coded in Kudu, but our build is more
complicated. This pulls the appropriate compile
flags from CMake's internal variables and uses
them to populate a templated python file
(bin/compile_flags.py.template) that can be
imported by clang_tidy_diff.py.

The script can be invoked directly, but this
change also adds a CMake target (stolen from
Kudu). A developer can run "make tidy-diff" and
the script will be invoked for changes since
the last upstream commit.

Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
---
M CMakeLists.txt
M be/CMakeLists.txt
A bin/clang_tidy_diff.py
A bin/compile_flags.py.template
A bin/get-upstream-commit.sh
M bin/run_clang_tidy.sh
A bin/tidy-diff.sh
M cmake_modules/FindKRPC.cmake
8 files changed, 366 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/9751/2
--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG@22
PS1, Line 22: The main change is to find a way to get the
> The backstory on this (which is somewhat redundant given Phil's comment) is
I like your last suggestion as an alternative to doing #3 first. Maybe it would 
be good to, in this patch, keep both systems but label them differently and 
maybe even preemptively add some more of those checks we'll eventually want but 
that we are not willing to apply to the whole codebase.



--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 26 Mar 2018 02:39:05 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG@22
PS1, Line 22: The main change is to find a way to get the
> I see - I always buildall, so I never noticed that myself.
The backstory on this (which is somewhat redundant given Phil's comment) is 
that I have had some changes that passed Gerrit review that then failed on GVO 
due to clang-tidy. The code changes passed the jenkins.impala.io's 
pre-review-test job (which doesn't do clang-tidy). My experience is that 
failure during GVO is the worst possible time.

There are a few observations:
A. I should have run clang-tidy.
B. pre-review-test should do clang-tidy
C. There should be an automated process that flags clang-tidy problems on 
Gerrit reviews.

"A" is true. I should have run clang-tidy. clang-tidy-1604 on jenkins.impala.io 
takes about 20 minutes (17-22 minutes or so). My guess is that this fits pretty 
well with how long it takes on my development machine. I would like "A" to be 
faster. Processing only the changed files speeds this up. On Kudu, 
clang-tidy-diff.py usually runs in a couple minutes. When something takes 
longer and ties up a development environment, developers get lazy.

We fixed "B" by adding a gerrit-verify-dryrun-external, which does exactly what 
GVO does without the +1 verify. This is great. I use it all the time.

I recently did some code changes on Kudu, and "C" is a very nice place to be. 
Quite a few clang-tidy issues won't be caught by reviewers. The clang-tidy 
issue that hit me on Kudu was this:
string long_string(INT_MAX, 'a');
Clang-tidy fails due to the string initialization being too big. I didn't know 
this check existed. "C" found it within an hour of posting to Gerrit without 
tying up my development system. It could have failed much later.

We should have "C" for Impala. I'm agnostic about how this happens. The script 
in this code change has logic to interpret the diff and post to Gerrit. We can 
use that or adapt it to the existing buildall.sh system (which doesn't sound 
too hard if the diff is similar).

Jim, you listed #3 before #1,#2. I want to make sure I'm not misinterpreting 
your preferences. What would you see as steps we need to take for #3?

I don't see very much cost to having two parallel systems, so long as they are 
labeled appropriately. Having some experience from actual code changes trumps 
any of my arm-chair analysis about which is better. That's why I'm in favor of 
merging this before figuring out whether it can fully replace the existing 
system. In other words, I put strong emphasis on the "if we can disable one" 
part of #3 when I put it last. I'm also generally ok with having both for an 
extended period of time (or forever).

Making the two systems distinct and clear to developers is important. A 
developer should know whether this is a diff-based clang-tidy or a full 
clang-tidy, and the scripts should be clear about what they do and how they are 
named.



--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 26 Mar 2018 01:38:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG@22
PS1, Line 22: The main change is to find a way to get the
> I don't want to speak with Joe, but there are two benefits here that I see:
I see - I always buildall, so I never noticed that myself.

As long as the codebase remains tidy-clean, run_clang_tidy will show the diff 
from upstream, since upstream would produce no tidy violations. That said, I'm 
now understanding Joe's point that "it might let us push things in the 
direction we want to go without rewriting".

Given how simplistic the Jenkins job is, I'd lean towards #3, #1, #2 for the 
ordering.



--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sun, 25 Mar 2018 20:25:53 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG@22
PS1, Line 22: The main change is to find a way to get the
> I don't think I quite understand what facts about the status quo make it so
I don't want to speak with Joe, but there are two benefits here that I see:

I ran run_clang_tidy.sh once and it blew away my CMake files, so I couldn't 
iterate with "make impalad" any more. I ended up having to run buildall to get 
myself out of a confusing state. This, um, discouraged me from running tidy 
ever again.

Kudu's stuff is also showing us the tidy diff from upstream, rather than all 
the tidy output. That would let us integrate it with Gerrit and such, and 
produce a smaller amount of output to sift through for developers.

In practice, I've seen plenty of reviews where fixing up the clang-tidy thing 
is the last thing that happens, because Gerrit-Verify failed. It's usually 
harmless, but anything we can do to speed up those fixes seems like a nice 
time-savings.



--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sun, 25 Mar 2018 16:56:50 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG@22
PS1, Line 22: The main change is to find a way to get the
> I agree. This is mostly a direct translation of what Kudu does, so I think
I don't think I quite understand what facts about the status quo make it so 
that the current tidy build is not usable. The Jenkins job is basically a 
wrapper around

bin/run_clang_tidy.sh &>"${TIDY_BUILD_OUT}"
! grep ']' "${TIDY_BUILD_OUT}"



--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sun, 25 Mar 2018 01:06:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG@22
PS1, Line 22: The main change is to find a way to get the
> We already have a "Tidy" build type in CMake. I guess the fundamental diffe
I agree. This is mostly a direct translation of what Kudu does, so I think 
there is still work to figure out how this fits in with the existing clang-tidy 
and make appropriate distinctions in our scripts and in CMake. Kudu does not 
have the full clang-tidy (AFAIK), so some of this may be redundant.

One rough thought on phasing this would go like this:

Phase 1: Check in something that individual developers can use. (Make a clear 
enough distinction between the two systems and how to use them.) Developers can 
play around with it and find issues. Start manually testing clang-tidy bot on 
the side.

Phase 2: Enable clang-tidy bot

Phase 3: Figure out how the two clang tidy systems relate and if we can disable 
one.

Hypothetically, I believe there can be a code change that could introduce clang 
tidy issues in code that hasn't changed, so I'm not sure the diff version alone 
catches everything. The existing clang-tidy job takes roughly 20 minutes, so 
maybe that wouldn't need to run for GVO if the diff version is active.

Some of the checks are disabled because existing code has a bunch of existing 
issues. If the diff based clang-tidy enables some checks that the full clang 
tidy does not, it might let us push things in the direction we want to go 
without rewriting the codebase immediately. Though I can also imagine this 
being super-annoying for some patches.



--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 22 Mar 2018 19:30:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 1:

(1 comment)

This seems like a good idea to me. My high-level question is about the 
relationship between this and our existing method of running clang-tidy.

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG@22
PS1, Line 22: The main change is to find a way to get the
We already have a "Tidy" build type in CMake. I guess the fundamental 
difference between that and this is that this makes it possible to run 
clang-tidy without regenerating all the CMakeFiles.

It does seem redundant to support both ways - it would be nice to remove the 
old way if this can displace it.



--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 22 Mar 2018 17:56:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG@14
PS1, Line 14: that
Typo: those


http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG@15
PS1, Line 15: adapt
Typo: adapts



--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 22 Mar 2018 01:07:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

[Joe McDonnell (Code Review)]

Joe McDonnell has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9751


Change subject: [experimental] Clang Tidy Diff trial balloon
..

[experimental] Clang Tidy Diff trial balloon

Kudu has a nice script that does clang tidy on
a diff rather than the whole source code. Kudu
uses this for a Tidy Bot that comments on Gerrit
reviews if the code change introduces an issue.

This is an experimental change to bring that
scripts over and adapt them to Impala. The
Kudu scripts are identical except:
 - The scripts are in ${IMPALA_HOME}/bin,
   requiring corresponding path fixups.
 - bin/clang_tidy_gerrit.py use Impala's locations
   for clang-tidy and clang-tidy-diff.py

The main change is to find a way to get the
appropriate compilation flags for clang. This is
hard-coded in Kudu, but our build is more
complicated. This pulls the appropriate compile
flags from CMake's internal variables and uses
them to populate a templated python file
(bin/compile_flags.py.template) that can be
imported by clang_tidy_gerrit.py.

The script can be invoked directly, but this
change also adds a CMake target (stolen from
Kudu). A developer can run "make tidy" and
the script will be invoked for changes since
the last upstream commit.

Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
---
M CMakeLists.txt
M be/CMakeLists.txt
A bin/clang_tidy_gerrit.py
A bin/compile_flags.py.template
A bin/get-upstream-commit.sh
A bin/tidy.sh
M cmake_modules/FindKRPC.cmake
7 files changed, 343 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/9751/1
--
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell