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 <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to