You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/08/04 22:31:07 UTC
[kudu-CR] WIP [thirdparty]: added include-what-you-use
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7593
Change subject: WIP [thirdparty]: added include-what-you-use
......................................................................
WIP [thirdparty]: added include-what-you-use
Build the include-what-you-use utility along with the LLVM toolchain.
The project URL:
https://include-what-you-use.org/
WIP: it's necessary to remove custom prefix once the tarball is uploaded
into the cloudfront bucket.
Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-add-iwyu.patch
M thirdparty/vars.sh
3 files changed, 34 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/7593/1
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [thirdparty]: added include-what-you-use
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [thirdparty]: added include-what-you-use
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7593/3/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
PS3, Line 264: fetch_and_expand include-what-you-use-${IWYU_VERSION}.src.tar.gz \
: ${LLVM_SOURCE}/tools/clang/tools
Right now updating LLVM involves extracting the LLVM tarball, adding clang and a bunch of other stuff, retarring it, and uploading the result. The new EXTRACT_SUBDIR functionality you added to fetch_and_expand means we could perhaps get away with uploading the various LLVM and clang tarballs to S3 and piecing the directory tree together on the client machine, like you did for IWYU.
In any case, adding IWYU like this represents an unusual middle ground: the LLVM tarball is not fully preprocessed, nor is it fully postprocessed. Could you either adhere to the status quo and upload a new LLVM tarball to S3 containing IWYU, or redo this to fully componentize LLVM and piece the directory tree together locally?
Which approach is preferred may depend on whether IWYU needs to be patched/updated with every LLVM release. Is there some semblance of compatibility? Or is there a 1-1 relationship between each release?
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP [thirdparty]: added include-what-you-use
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7593
to look at the new patch set (#2).
Change subject: WIP [thirdparty]: added include-what-you-use
......................................................................
WIP [thirdparty]: added include-what-you-use
Build the include-what-you-use utility along with the LLVM toolchain.
The project URL:
https://include-what-you-use.org/
WIP: it's necessary to remove custom prefix once the tarball is uploaded
into the cloudfront bucket.
Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-add-iwyu.patch
M thirdparty/vars.sh
3 files changed, 29 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/7593/2
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [thirdparty]: added include-what-you-use
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [thirdparty]: added include-what-you-use
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [thirdparty]: added include-what-you-use
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7593
to look at the new patch set (#4).
Change subject: [thirdparty]: added include-what-you-use
......................................................................
[thirdparty]: added include-what-you-use
Build the include-what-you-use utility along with the LLVM toolchain.
The project URL:
https://include-what-you-use.org/
Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-add-iwyu.patch
A thirdparty/patches/llvm-iwyu-include-picker.patch
A thirdparty/patches/llvm-iwyu-nocurses.patch
M thirdparty/vars.sh
5 files changed, 47 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/7593/4
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [thirdparty]: added include-what-you-use
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [thirdparty]: added include-what-you-use
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7593/3/thirdparty/patches/llvm-iwyu-nocurses.patch
File thirdparty/patches/llvm-iwyu-nocurses.patch:
What does IWYU use curses for? What is the effect of removing it?
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty]: added include-what-you-use
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7593
to look at the new patch set (#3).
Change subject: [thirdparty]: added include-what-you-use
......................................................................
[thirdparty]: added include-what-you-use
Build the include-what-you-use utility along with the LLVM toolchain.
The project URL:
https://include-what-you-use.org/
Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-add-iwyu.patch
A thirdparty/patches/llvm-iwyu-nocurses.patch
M thirdparty/vars.sh
4 files changed, 44 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/7593/3
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [thirdparty]: added include-what-you-use
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [thirdparty]: added include-what-you-use
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7593/3/thirdparty/patches/llvm-iwyu-nocurses.patch
File thirdparty/patches/llvm-iwyu-nocurses.patch:
> What does IWYU use curses for? What is the effect of removing it?
Some of our build machines used by Jenkins do not have ncurses libraries and headers files installed. On those the IWYU tool could not be built.
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty]: added include-what-you-use
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [thirdparty]: added include-what-you-use
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7593/3/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
PS3, Line 55: local URL_PREFIX=$3
: if [ -z "$URL_PREFIX" ]; then
: URL_PREFIX="${CLOUDFRONT_URL_PREFIX}"
: fi
> is this used?
It's no longer used: that's a remnant from the version where the source file was not uploaded into the cloudsource repo.
I'll remove it.
PS3, Line 264: fetch_and_expand include-what-you-use-${IWYU_VERSION}.src.tar.gz \
: ${LLVM_SOURCE}/tools/clang/tools
> Right now updating LLVM involves extracting the LLVM tarball, adding clang
Thank you for pointing that out. Indeed -- this is kind of hackish way to achieve what I needed.
I think the former option (uploading a new LLVM tarball which would include IWYU) is my best bet in the short term. The include-what-you-use source does not need to be updated every release of LLVM, it just needs to be compiled along with it.
http://gerrit.cloudera.org:8080/#/c/7593/3/thirdparty/patches/llvm-iwyu-nocurses.patch
File thirdparty/patches/llvm-iwyu-nocurses.patch:
> What does IWYU use curses for? What is the effect of removing it?
You could find more details at https://github.com/include-what-you-use/include-what-you-use/pull/465
It seems somehow they compile LlvmSupport lib with ncurses support, and when linking the include-what-you-use binary (which links LlvmSupport lib), it's necessary to add ncurses library for some symbols in LlvmSupport library itself.
However, I found that in our environment we don't need that (probably, because we don't use all bells and whistles).
But I might be some bug -- I haven't looked at that yet. I will need to do that to continue with that pull request I submitted, though.
http://gerrit.cloudera.org:8080/#/c/7593/3/thirdparty/vars.sh
File thirdparty/vars.sh:
PS3, Line 148: IWYU_NAME=llvm-$LLVM_VERSION.src
: IWYU_SOURCE=$TP_SOURCE_DIR/$IWYU_NAME
> are these actually used, then?
Nope, it's not. It seems it's not needed.
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty]: added include-what-you-use
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [thirdparty]: added include-what-you-use
......................................................................
[thirdparty]: added include-what-you-use
Build the include-what-you-use utility along with the LLVM toolchain.
The project URL:
https://include-what-you-use.org/
Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Reviewed-on: http://gerrit.cloudera.org:8080/7593
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-add-iwyu.patch
A thirdparty/patches/llvm-iwyu-include-picker.patch
A thirdparty/patches/llvm-iwyu-nocurses.patch
M thirdparty/vars.sh
5 files changed, 39 insertions(+), 2 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [thirdparty]: added include-what-you-use
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [thirdparty]: added include-what-you-use
......................................................................
Patch Set 6: Verified+1
Unrelated flake in org.apache.kudu.client.TestScannerMultiTablet.org.apache.kudu.client.TestScannerMultiTablet
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [thirdparty]: added include-what-you-use
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7593
to look at the new patch set (#6).
Change subject: [thirdparty]: added include-what-you-use
......................................................................
[thirdparty]: added include-what-you-use
Build the include-what-you-use utility along with the LLVM toolchain.
The project URL:
https://include-what-you-use.org/
Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-add-iwyu.patch
A thirdparty/patches/llvm-iwyu-include-picker.patch
A thirdparty/patches/llvm-iwyu-nocurses.patch
M thirdparty/vars.sh
5 files changed, 39 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/7593/6
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [thirdparty]: added include-what-you-use
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [thirdparty]: added include-what-you-use
......................................................................
Patch Set 3:
(2 comments)
any idea what the incremental compilation time cost is for thirdparty on this?
http://gerrit.cloudera.org:8080/#/c/7593/3/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
PS3, Line 55: local URL_PREFIX=$3
: if [ -z "$URL_PREFIX" ]; then
: URL_PREFIX="${CLOUDFRONT_URL_PREFIX}"
: fi
is this used?
http://gerrit.cloudera.org:8080/#/c/7593/3/thirdparty/vars.sh
File thirdparty/vars.sh:
PS3, Line 148: IWYU_NAME=llvm-$LLVM_VERSION.src
: IWYU_SOURCE=$TP_SOURCE_DIR/$IWYU_NAME
are these actually used, then?
--
To view, visit http://gerrit.cloudera.org:8080/7593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes