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 2017/11/06 19:21:16 UTC
[kudu-CR] cmake: use MAKE C IDENTIFIER to generate valid target names
Hello Dan Burkert, Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8481
to review the following change.
Change subject: cmake: use MAKE_C_IDENTIFIER to generate valid target names
......................................................................
cmake: use MAKE_C_IDENTIFIER to generate valid target names
This fixes a build failure coming from the FindThrift cmake module in
the case that the source directory is in a path containing a character
which is not valid for CMake target names. For example, package version
numbers often include the '~' character which isn't allowed in a target
name. This would produce an error like:
09:42:16 CMake Error at cmake_modules/FindThrift.cmake:128 (add_custom_target):
09:42:16 The target name
09:42:16 "-data-jenkins-workspace-generic-package-ubuntu64-16-04-impala-CDH5-Packaging-Kudu-2017-11-06_08-48-50-kudu-1.6.0+cdh5.14.0+0-1.cdh5.14.0.p0.91~xenial-build-release-src-kudu-hms-hive_metastore.thrift"
09:42:16 is reserved or not valid for certain CMake features, such as generator
09:42:16 expressions, and may result in undefined behavior.
09:42:16 Call Stack (most recent call first):
09:42:16 src/kudu/hms/CMakeLists.txt:18 (THRIFT_GENERATE_CPP)
The fix is to use the MAKE_C_IDENTIFIER string function from CMake to ensure
that the generated target name is a valid target name.
Change-Id: I71f145c08d7fbb5627442b6527b3feebc05055bd
---
M cmake_modules/FindProtobuf.cmake
M cmake_modules/FindThrift.cmake
2 files changed, 2 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/8481/1
--
To view, visit http://gerrit.cloudera.org:8080/8481
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I71f145c08d7fbb5627442b6527b3feebc05055bd
Gerrit-Change-Number: 8481
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] cmake: use MAKE C IDENTIFIER to generate valid target names
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8481 )
Change subject: cmake: use MAKE_C_IDENTIFIER to generate valid target names
......................................................................
cmake: use MAKE_C_IDENTIFIER to generate valid target names
This fixes a build failure coming from the FindThrift cmake module in
the case that the source directory is in a path containing a character
which is not valid for CMake target names. For example, package version
numbers often include the '~' character which isn't allowed in a target
name. This would produce an error like:
09:42:16 CMake Error at cmake_modules/FindThrift.cmake:128 (add_custom_target):
09:42:16 The target name
09:42:16 "-data-jenkins-workspace-generic-package-ubuntu64-16-04-impala-CDH5-Packaging-Kudu-2017-11-06_08-48-50-kudu-1.6.0+cdh5.14.0+0-1.cdh5.14.0.p0.91~xenial-build-release-src-kudu-hms-hive_metastore.thrift"
09:42:16 is reserved or not valid for certain CMake features, such as generator
09:42:16 expressions, and may result in undefined behavior.
09:42:16 Call Stack (most recent call first):
09:42:16 src/kudu/hms/CMakeLists.txt:18 (THRIFT_GENERATE_CPP)
The fix is to use the MAKE_C_IDENTIFIER string function from CMake to ensure
that the generated target name is a valid target name.
Change-Id: I71f145c08d7fbb5627442b6527b3feebc05055bd
Reviewed-on: http://gerrit.cloudera.org:8080/8481
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Dan Burkert <da...@apache.org>
---
M cmake_modules/FindProtobuf.cmake
M cmake_modules/FindThrift.cmake
2 files changed, 2 insertions(+), 4 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/8481
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I71f145c08d7fbb5627442b6527b3feebc05055bd
Gerrit-Change-Number: 8481
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
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>
[kudu-CR] cmake: use MAKE C IDENTIFIER to generate valid target names
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has removed a vote on this change.
Change subject: cmake: use MAKE_C_IDENTIFIER to generate valid target names
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8481
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I71f145c08d7fbb5627442b6527b3feebc05055bd
Gerrit-Change-Number: 8481
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] cmake: use MAKE C IDENTIFIER to generate valid target names
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8481 )
Change subject: cmake: use MAKE_C_IDENTIFIER to generate valid target names
......................................................................
Patch Set 1:
flake
--
To view, visit http://gerrit.cloudera.org:8080/8481
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71f145c08d7fbb5627442b6527b3feebc05055bd
Gerrit-Change-Number: 8481
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 06 Nov 2017 20:49:54 +0000
Gerrit-HasComments: No
[kudu-CR] cmake: use MAKE C IDENTIFIER to generate valid target names
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8481 )
Change subject: cmake: use MAKE_C_IDENTIFIER to generate valid target names
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/8481
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71f145c08d7fbb5627442b6527b3feebc05055bd
Gerrit-Change-Number: 8481
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 06 Nov 2017 20:49:47 +0000
Gerrit-HasComments: No
[kudu-CR] cmake: use MAKE C IDENTIFIER to generate valid target names
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8481 )
Change subject: cmake: use MAKE_C_IDENTIFIER to generate valid target names
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8481
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71f145c08d7fbb5627442b6527b3feebc05055bd
Gerrit-Change-Number: 8481
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 06 Nov 2017 19:38:22 +0000
Gerrit-HasComments: No