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/06/21 21:23:20 UTC
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10788
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch llvm to fix run-clang-tidy.py output
Clang's run-clang-tidy.py has some behaviors
that make it hard for Impala to get good output.
Specifically:
1. It does not synchronize output to stdout.
Output for different files can be interleaved
with each other and with the script's own output.
2. It does not suppress its own output when the
-quiet argument is specified. This output is hard
to separate from the actual output of clang-tidy.
3. It can silently skips files when clang-tidy
fails with an error code. This does not happen
with the existing set of clang-tidy checks, but
it could cause loss of coverage if a new check
caused clang-tidy to encounter an error.
This adds a patch to llvm that fixes these issues
in run-clang-tidy.py by synchronizing output,
respecting -quiet, and allowing an error from
clang-tidy to abort the run.
Since run-clang-tidy.py is not in the main llvm
source tree (it is in the extra package), our
existing method could not patch it. This changes
llvm to extract all of the archives into the
appropriate places before applying a unified
patch that can touch any of the source files from
any of the archives.
Testing:
- Ran run-clang-tidy.py on normal Impala
checkout without issues.
- Added clang tidy checks that Impala
currently doesn't pass and verified
that the output with -quiet is
sensible.
Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
---
M buildall.sh
M functions.sh
M source/llvm/build-source-tarball.sh
A source/llvm/llvm-5.0.1-patches/0001-PATCH-Fix-run-clang-tidy.py-s-output.patch
4 files changed, 98 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/88/10788/2
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10788
to look at the new patch set (#4).
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch llvm to fix run-clang-tidy.py output
Clang's run-clang-tidy.py has some behaviors
that make it hard for Impala to get good output.
Specifically:
1. It does not synchronize output to stdout.
Output for different files can be interleaved
with each other and with the script's own output.
2. It does not suppress its own output when the
-quiet argument is specified. This output is hard
to separate from the actual output of clang-tidy.
3. It can silently skips files when clang-tidy
fails with an error code. This does not happen
with the existing set of clang-tidy checks, but
it could cause loss of coverage if a new check
caused clang-tidy to encounter an error.
This adds a patch to llvm that fixes these issues
in run-clang-tidy.py by synchronizing output,
respecting -quiet, and allowing an error from
clang-tidy to abort the run.
Since run-clang-tidy.py is not in the main llvm
source tree (it is in the extra package), our
existing method could not patch it. This changes
llvm to extract all of the archives into the
appropriate places before applying a unified
patch that can touch any of the source files from
any of the archives.
Testing:
- Ran run-clang-tidy.py on normal Impala
checkout without issues.
- Added clang tidy checks that Impala
currently doesn't pass and verified
that the output with -quiet is
sensible.
Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
---
M buildall.sh
M functions.sh
M source/llvm/build-source-tarball.sh
A source/llvm/llvm-5.0.1-patches/0001-PATCH-Fix-run-clang-tidy.py-s-output.patch
4 files changed, 109 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/88/10788/4
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/10788 )
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10788/2/functions.sh
File functions.sh:
http://gerrit.cloudera.org:8080/#/c/10788/2/functions.sh@210
PS2, Line 210: function setup_extracted_package_build() {
> A brief comment would be good (maybe explain how it differes from setup_pac
Added comment here and modified the comment for setup_package_build to note that it is a wrapper around this function.
http://gerrit.cloudera.org:8080/#/c/10788/2/source/llvm/llvm-5.0.1-patches/0001-PATCH-Fix-run-clang-tidy.py-s-output.patch
File source/llvm/llvm-5.0.1-patches/0001-PATCH-Fix-run-clang-tidy.py-s-output.patch:
PS2:
> Would be nice to get this upstream so we don't have to carry it around with
I'm looking at the upstream stuff and it seems like the fixes would still apply. I will look into it.
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Jun 2018 23:13:34 +0000
Gerrit-HasComments: Yes
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10788
to look at the new patch set (#3).
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch llvm to fix run-clang-tidy.py output
Clang's run-clang-tidy.py has some behaviors
that make it hard for Impala to get good output.
Specifically:
1. It does not synchronize output to stdout.
Output for different files can be interleaved
with each other and with the script's own output.
2. It does not suppress its own output when the
-quiet argument is specified. This output is hard
to separate from the actual output of clang-tidy.
3. It can silently skips files when clang-tidy
fails with an error code. This does not happen
with the existing set of clang-tidy checks, but
it could cause loss of coverage if a new check
caused clang-tidy to encounter an error.
This adds a patch to llvm that fixes these issues
in run-clang-tidy.py by synchronizing output,
respecting -quiet, and allowing an error from
clang-tidy to abort the run.
Since run-clang-tidy.py is not in the main llvm
source tree (it is in the extra package), our
existing method could not patch it. This changes
llvm to extract all of the archives into the
appropriate places before applying a unified
patch that can touch any of the source files from
any of the archives.
Testing:
- Ran run-clang-tidy.py on normal Impala
checkout without issues.
- Added clang tidy checks that Impala
currently doesn't pass and verified
that the output with -quiet is
sensible.
Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
---
M buildall.sh
M functions.sh
M source/llvm/build-source-tarball.sh
A source/llvm/llvm-5.0.1-patches/0001-PATCH-Fix-run-clang-tidy.py-s-output.patch
4 files changed, 109 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/88/10788/3
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/10788 )
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10788/4/functions.sh
File functions.sh:
http://gerrit.cloudera.org:8080/#/c/10788/4/functions.sh@218
PS4, Line 218: setup_package_build
> setup_extracted_package_build
Fixed
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Jun 2018 18:48:52 +0000
Gerrit-HasComments: Yes
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10788
to look at the new patch set (#5).
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch llvm to fix run-clang-tidy.py output
Clang's run-clang-tidy.py has some behaviors
that make it hard for Impala to get good output.
Specifically:
1. It does not synchronize output to stdout.
Output for different files can be interleaved
with each other and with the script's own output.
2. It does not suppress its own output when the
-quiet argument is specified. This output is hard
to separate from the actual output of clang-tidy.
3. It can silently skips files when clang-tidy
fails with an error code. This does not happen
with the existing set of clang-tidy checks, but
it could cause loss of coverage if a new check
caused clang-tidy to encounter an error.
This adds a patch to llvm that fixes these issues
in run-clang-tidy.py by synchronizing output,
respecting -quiet, and allowing an error from
clang-tidy to abort the run.
Since run-clang-tidy.py is not in the main llvm
source tree (it is in the extra package), our
existing method could not patch it. This changes
llvm to extract all of the archives into the
appropriate places before applying a unified
patch that can touch any of the source files from
any of the archives.
Testing:
- Ran run-clang-tidy.py on normal Impala
checkout without issues.
- Added clang tidy checks that Impala
currently doesn't pass and verified
that the output with -quiet is
sensible.
Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
---
M buildall.sh
M functions.sh
M source/llvm/build-source-tarball.sh
A source/llvm/llvm-5.0.1-patches/0001-PATCH-Fix-run-clang-tidy.py-s-output.patch
4 files changed, 110 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/88/10788/5
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10788 )
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10788/4/functions.sh
File functions.sh:
http://gerrit.cloudera.org:8080/#/c/10788/4/functions.sh@218
PS4, Line 218: setup_package_build
setup_extracted_package_build
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Jun 2018 00:08:12 +0000
Gerrit-HasComments: Yes
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/10788 )
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch Set 2:
Let me know if there are other appropriate reviewers.
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Jun 2018 21:55:20 +0000
Gerrit-HasComments: No
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/10788 )
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch Set 5: Verified+1
Toolchain package build succeeded on all platforms, +1 verified
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Jun 2018 19:43:12 +0000
Gerrit-HasComments: No
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/10788 )
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch Set 5: Code-Review+2
Carry +2
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Jun 2018 18:49:27 +0000
Gerrit-HasComments: No
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10788 )
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch llvm to fix run-clang-tidy.py output
Clang's run-clang-tidy.py has some behaviors
that make it hard for Impala to get good output.
Specifically:
1. It does not synchronize output to stdout.
Output for different files can be interleaved
with each other and with the script's own output.
2. It does not suppress its own output when the
-quiet argument is specified. This output is hard
to separate from the actual output of clang-tidy.
3. It can silently skips files when clang-tidy
fails with an error code. This does not happen
with the existing set of clang-tidy checks, but
it could cause loss of coverage if a new check
caused clang-tidy to encounter an error.
This adds a patch to llvm that fixes these issues
in run-clang-tidy.py by synchronizing output,
respecting -quiet, and allowing an error from
clang-tidy to abort the run.
Since run-clang-tidy.py is not in the main llvm
source tree (it is in the extra package), our
existing method could not patch it. This changes
llvm to extract all of the archives into the
appropriate places before applying a unified
patch that can touch any of the source files from
any of the archives.
Testing:
- Ran run-clang-tidy.py on normal Impala
checkout without issues.
- Added clang tidy checks that Impala
currently doesn't pass and verified
that the output with -quiet is
sensible.
Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Reviewed-on: http://gerrit.cloudera.org:8080/10788
Reviewed-by: Joe McDonnell <jo...@cloudera.com>
Tested-by: Joe McDonnell <jo...@cloudera.com>
---
M buildall.sh
M functions.sh
M source/llvm/build-source-tarball.sh
A source/llvm/llvm-5.0.1-patches/0001-PATCH-Fix-run-clang-tidy.py-s-output.patch
4 files changed, 110 insertions(+), 20 deletions(-)
Approvals:
Joe McDonnell: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[native-toolchain-CR] Patch llvm to fix run-clang-tidy.py output
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10788 )
Change subject: Patch llvm to fix run-clang-tidy.py output
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10788/2/functions.sh
File functions.sh:
http://gerrit.cloudera.org:8080/#/c/10788/2/functions.sh@210
PS2, Line 210: function setup_extracted_package_build() {
A brief comment would be good (maybe explain how it differes from setup_package_build).
http://gerrit.cloudera.org:8080/#/c/10788/2/source/llvm/llvm-5.0.1-patches/0001-PATCH-Fix-run-clang-tidy.py-s-output.patch
File source/llvm/llvm-5.0.1-patches/0001-PATCH-Fix-run-clang-tidy.py-s-output.patch:
PS2:
Would be nice to get this upstream so we don't have to carry it around with us, but that shouldn't block progress here.
--
To view, visit http://gerrit.cloudera.org:8080/10788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9125cb0a908fd7005ee68aafb41f3afe93522632
Gerrit-Change-Number: 10788
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Jun 2018 22:28:46 +0000
Gerrit-HasComments: Yes