You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2018/05/21 18:49:20 UTC
[kudu-CR] thirdparty: clean up unused argument warnings
Hello Dan Burkert, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/10469
to review the following change.
Change subject: thirdparty: clean up unused argument warnings
......................................................................
thirdparty: clean up unused argument warnings
Commit ba3cdea53 revved breakpad to a hash that includes -Werror in its
build. This exposed an issue with non-ccache thirdparty builds: it turns out
that clang has been emitting a lot of warnings about unused arguments when
building TSAN dependencies, and those warnings now break breakpad when built
for TSAN. Builds that use ccache don't emit those warnings because the
build-support/ccache-clang scripts add -Qunused-arguments to the clang
command line.
We could fix this by patching out -Werror in breakpad, but that's not
particularly robust; whose to say that -Werror won't show up in another
dependency in the future? So instead, this patch tries to clean up those
unused argument warnings.
The core issue is the use of -stdlib=libc++ and -nostdinc++. Apparently the
former is intended to be a linker option and thus shouldn't be part of
CXXFLAGS, while the latter is a compiler option. So this patch moves
-stdlib=libc++ to LDFLAGS. We're also now passing LDFLAGS into
CMAKE_SHARED_LINKER_FLAGS and CMAKE_MODULE_LINKER_FLAGS; this allowed
LLVMHello.so to find libc++ in the LLVM build.
Sadly, I couldn't get rid of all the unused argument warnings. The problem
is that clang warns about -nostdinc++ when it's passed to the linker, and
setting it in CMAKE_CXX_FLAGS does just that. So I ended up adding
-Qunused-arguments anyway.
I tested this with a full thirdparty build without ccache on Ubuntu 16.04
as well as full builds on CentOS 6.6 and Ubuntu 18.04 with ccache.
Change-Id: Ie1f7a4b8d11dc3f820259bce9819755f68339854
---
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
2 files changed, 36 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/10469/1
--
To view, visit http://gerrit.cloudera.org:8080/10469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie1f7a4b8d11dc3f820259bce9819755f68339854
Gerrit-Change-Number: 10469
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] thirdparty: clean up unused argument warnings
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10469 )
Change subject: thirdparty: clean up unused argument warnings
......................................................................
Patch Set 1: Code-Review+2
LGTM. Tested on macos.
--
To view, visit http://gerrit.cloudera.org:8080/10469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1f7a4b8d11dc3f820259bce9819755f68339854
Gerrit-Change-Number: 10469
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 30 May 2018 22:23:01 +0000
Gerrit-HasComments: No
[kudu-CR] thirdparty: clean up unused argument warnings
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10469 )
Change subject: thirdparty: clean up unused argument warnings
......................................................................
thirdparty: clean up unused argument warnings
Commit ba3cdea53 revved breakpad to a hash that includes -Werror in its
build. This exposed an issue with non-ccache thirdparty builds: it turns out
that clang has been emitting a lot of warnings about unused arguments when
building TSAN dependencies, and those warnings now break breakpad when built
for TSAN. Builds that use ccache don't emit those warnings because the
build-support/ccache-clang scripts add -Qunused-arguments to the clang
command line.
We could fix this by patching out -Werror in breakpad, but that's not
particularly robust; whose to say that -Werror won't show up in another
dependency in the future? So instead, this patch tries to clean up those
unused argument warnings.
The core issue is the use of -stdlib=libc++ and -nostdinc++. Apparently the
former is intended to be a linker option and thus shouldn't be part of
CXXFLAGS, while the latter is a compiler option. So this patch moves
-stdlib=libc++ to LDFLAGS. We're also now passing LDFLAGS into
CMAKE_SHARED_LINKER_FLAGS and CMAKE_MODULE_LINKER_FLAGS; this allowed
LLVMHello.so to find libc++ in the LLVM build.
Sadly, I couldn't get rid of all the unused argument warnings. The problem
is that clang warns about -nostdinc++ when it's passed to the linker, and
setting it in CMAKE_CXX_FLAGS does just that. So I ended up adding
-Qunused-arguments anyway.
I tested this with a full thirdparty build without ccache on Ubuntu 16.04
as well as full builds on CentOS 6.6 and Ubuntu 18.04 with ccache.
Change-Id: Ie1f7a4b8d11dc3f820259bce9819755f68339854
Reviewed-on: http://gerrit.cloudera.org:8080/10469
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
2 files changed, 36 insertions(+), 18 deletions(-)
Approvals:
Kudu Jenkins: Verified
Dan Burkert: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie1f7a4b8d11dc3f820259bce9819755f68339854
Gerrit-Change-Number: 10469
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>