Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18103 )
Change subject: [thirdparty] Add jwt-cpp to thirdparty build ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/18103/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18103/1//COMMIT_MSG@9 PS1, Line 9: require nit: required http://gerrit.cloudera.org:8080/#/c/18103/1/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: http://gerrit.cloudera.org:8080/#/c/18103/1/thirdparty/build-definitions.sh@1138 PS1, Line 1138: "$OPENSSL_ROOT_DIR" \ This looks a bit strange: why not to pass all what's necessary in this context via EXTRA_CXXFLAGS? http://gerrit.cloudera.org:8080/#/c/18103/1/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/18103/1/thirdparty/build-thirdparty.sh@181 PS1, Line 181: OPENSSL_ROOT_DIR="-DOPENSSL_ROOT_DIR=$homebrew_openssl_dir" This isn't a good naming for a variable: at least, OPENSSL_ROOT_DIR is supposed to point to a location, not be a flag or something (at least that's how it's used in the top-level CMakeLists.txt). Why not to add corresponding -D... into the EXTRA_CXXFLAGS? http://gerrit.cloudera.org:8080/#/c/18103/1/thirdparty/patches/jwt-cpp-openssl-1.0.1.patch File thirdparty/patches/jwt-cpp-openssl-1.0.1.patch: http://gerrit.cloudera.org:8080/#/c/18103/1/thirdparty/patches/jwt-cpp-openssl-1.0.1.patch@18 PS1, Line 18: +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11") : + It turns out this piece isn't necessary if building with C++11-cabaple compiler. At least, that worked for me fine without this line if using devtoolset on CentOS6.6 -- To view, visit http://gerrit.cloudera.org:8080/18103 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idab887e1e0c44e9ebe6897defec8238c020cccbb Gerrit-Change-Number: 18103 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 15 Dec 2021 22:54:35 +0000 Gerrit-HasComments: Yes
