You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/04/23 18:57:56 UTC
[kudu-CR] iwyu: add proper dependency on thrift generated code, check for iwyu errors
Hello Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/10161
to review the following change.
Change subject: iwyu: add proper dependency on thrift generated code, check for iwyu errors
......................................................................
iwyu: add proper dependency on thrift generated code, check for iwyu errors
IWYU was giving incorrect results on the HMS modules when running on
Jenkins. This was due to the 'iwyu' CMake target not depending on the
thrift code generation. IWYU was continuing to run despite having
'missing include' errors.
This patch adds the appropriate CMake dependency and also makes our
IWYU wrapper check for such errors in the IWYU output.
I also changed the IWYU wrapper to explicitly build the IWYU
dependencies before running so that even if a user invokes it directly,
they won't hit this issue.
Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde
---
M CMakeLists.txt
M build-support/iwyu.py
2 files changed, 18 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/10161/1
--
To view, visit http://gerrit.cloudera.org:8080/10161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde
Gerrit-Change-Number: 10161
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] iwyu: add proper dependency on thrift generated code, check for iwyu errors
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10161 )
Change subject: iwyu: add proper dependency on thrift generated code, check for iwyu errors
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10161/1/build-support/iwyu.py
File build-support/iwyu.py:
http://gerrit.cloudera.org:8080/#/c/10161/1/build-support/iwyu.py@52
PS1, Line 52: r'^.+?:\d+:\d+:\s*'
> the FATAL_ERROR case is already handled explicitly in _run_iwyu_tool. I add
Ah, yes -- I found that yesterday as well after reviewing that patch.
Thank you for addressing this tiny nit.
--
To view, visit http://gerrit.cloudera.org:8080/10161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde
Gerrit-Change-Number: 10161
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Apr 2018 19:59:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] iwyu: add proper dependency on thrift generated code, check for iwyu errors
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10161 )
Change subject: iwyu: add proper dependency on thrift generated code, check for iwyu errors
......................................................................
iwyu: add proper dependency on thrift generated code, check for iwyu errors
IWYU was giving incorrect results on the HMS modules when running on
Jenkins. This was due to the 'iwyu' CMake target not depending on the
thrift code generation. IWYU was continuing to run despite having
'missing include' errors.
This patch adds the appropriate CMake dependency and also makes our
IWYU wrapper check for such errors in the IWYU output.
I also changed the IWYU wrapper to explicitly build the IWYU
dependencies before running so that even if a user invokes it directly,
they won't hit this issue.
Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde
Reviewed-on: http://gerrit.cloudera.org:8080/10161
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M CMakeLists.txt
M build-support/iwyu.py
2 files changed, 19 insertions(+), 5 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde
Gerrit-Change-Number: 10161
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] iwyu: add proper dependency on thrift generated code, check for iwyu errors
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10161 )
Change subject: iwyu: add proper dependency on thrift generated code, check for iwyu errors
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10161/1/build-support/iwyu.py
File build-support/iwyu.py:
http://gerrit.cloudera.org:8080/#/c/10161/1/build-support/iwyu.py@52
PS1, Line 52: r'^.+?:\d+:\d+: (fatal )?error:'
> In $IWYU_SRC_ROOT/iwyu_test_util.py there is pattern for expected error/war
the FATAL_ERROR case is already handled explicitly in _run_iwyu_tool. I added the \s* instead of ' '
--
To view, visit http://gerrit.cloudera.org:8080/10161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde
Gerrit-Change-Number: 10161
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Apr 2018 18:03:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] iwyu: add proper dependency on thrift generated code, check for iwyu errors
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10161
to look at the new patch set (#2).
Change subject: iwyu: add proper dependency on thrift generated code, check for iwyu errors
......................................................................
iwyu: add proper dependency on thrift generated code, check for iwyu errors
IWYU was giving incorrect results on the HMS modules when running on
Jenkins. This was due to the 'iwyu' CMake target not depending on the
thrift code generation. IWYU was continuing to run despite having
'missing include' errors.
This patch adds the appropriate CMake dependency and also makes our
IWYU wrapper check for such errors in the IWYU output.
I also changed the IWYU wrapper to explicitly build the IWYU
dependencies before running so that even if a user invokes it directly,
they won't hit this issue.
Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde
---
M CMakeLists.txt
M build-support/iwyu.py
2 files changed, 19 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/10161/2
--
To view, visit http://gerrit.cloudera.org:8080/10161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde
Gerrit-Change-Number: 10161
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] iwyu: add proper dependency on thrift generated code, check for iwyu errors
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10161 )
Change subject: iwyu: add proper dependency on thrift generated code, check for iwyu errors
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10161/1/build-support/iwyu.py
File build-support/iwyu.py:
http://gerrit.cloudera.org:8080/#/c/10161/1/build-support/iwyu.py@52
PS1, Line 52: r'^.+?:\d+:\d+: (fatal )?error:'
In $IWYU_SRC_ROOT/iwyu_test_util.py there is pattern for expected error/warning/diag output:
_ACTUAL_DIAGNOSTICS_RE = re.compile(r'^(.*?):(\d+):\d+:\s*'
r'(?:warning|error|fatal error):\s*(.*)$')
Maybe, allow any number of spaces before '(fatal )?error' as well?
Also, it seems IWYU itself can output messages like 'FATAL ERROR: ...' in case of wrong usage, etc.. Do we need to catch those here as well?
--
To view, visit http://gerrit.cloudera.org:8080/10161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde
Gerrit-Change-Number: 10161
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 24 Apr 2018 03:01:57 +0000
Gerrit-HasComments: Yes