You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2018/03/22 01:05:05 UTC
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
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 <jo...@cloudera.com>
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jul 2018 23:34:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 19:30:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 25 Mar 2018 20:25:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
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 (&optional 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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 01 Apr 2018 03:10:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 25 Mar 2018 16:56:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Jul 2018 15:31:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 26 Mar 2018 23:40:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jul 2018 22:19:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 26 Mar 2018 01:38:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 25 Mar 2018 01:06:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 26 Mar 2018 02:39:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 01:07:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Jun 2018 21:54:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 17:56:38 +0000
Gerrit-HasComments: Yes