HBASE-23105: Download lib double conversion, fizz, update folly #6
Conversation
bharathv
left a comment
bharathv
left a comment
There was a problem hiding this comment.
Let me know when this is ready for review, I'll take a look, thanks.
|
@bharathv Thanks. making some changes now to download double conversion and gsasl2. will mark as ready for review after. thanks! |
phrocker
force-pushed
the
HBASE-23105
branch
2 times, most recently
from
62da5ee to
acf5029
Compare
bharathv
left a comment
bharathv
left a comment
There was a problem hiding this comment.
Nice.. I skimmed through the PR, cmake side changes seem fine, I'll take a look at the code changes later this week.
bharathv
left a comment
bharathv
left a comment
There was a problem hiding this comment.
Initial round of comments, yet to try out the patch, might have a few more, thanks for your patience.
CMakeLists.txt
Outdated
| include_directories("${JAVA_HBASE_DIR}/hbase-common/target/generated-sources/native/") | ||
| ############ | ||
| ## Validate that we have C++ 14 support | ||
| ## Validate that we have C++ 17 support |
There was a problem hiding this comment.
Is this for std::optional?
There was a problem hiding this comment.
Yes my hope was to move forward as it would support optional. In retrospect after a nice break from computers during a week off I don't want to push for this change without a lot more testing. I will revert this part of the change. I've grown fond of c++17, but I think you'd agree it's an unnecessary change at this time. Thanks for your patience and help!
There was a problem hiding this comment.
Ya, preferred to keep at 14 unless there is a lot of upside IMO. Like I mentioned in my other patch, we are going to have a precommit soon, so we will have good test coverage in a clean environment once we get #10 in.
CMakeLists.txt
Outdated
|
|
||
| if (DOWNLOAD_DEPENDENCIES) | ||
| download_doubleconversion(${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR}) | ||
| download_fizz(${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR}) |
There was a problem hiding this comment.
This needs corresponding changes in Dockerfile?
There was a problem hiding this comment.
Oh sorry I have these changes staged. Will roll them in and sanity check them. Thanks!
There was a problem hiding this comment.
I pushed a commit but will ping you after I've made some additional changes regarding this. I'm going to re-run through some tests. thanks!
cmake/DownloadCyrusSasl.cmake
Outdated
| BINARY_DIR ${BUILD_DIR}/dependencies/cyrussasl-src/ | ||
| CONFIGURE_COMMAND ./configure --enable-static --with-pic --prefix=${BUILD_DIR}/dependencies/cyrussasl-install | ||
| "CFLAGS=-fPIC" | ||
| "CXXFLAGS=${CMAKE_CXX_FLAGS} -fPIC" |
There was a problem hiding this comment.
nit: unnecessary line break
cmake/DownloadCyrusSasl.cmake
Outdated
| "CFLAGS=-fPIC" | ||
| "CXXFLAGS=${CMAKE_CXX_FLAGS} -fPIC" | ||
| ) | ||
| add_library(sasl2 STATIC IMPORTED) |
There was a problem hiding this comment.
Move this to CMakeLists like other dependencies?
cmake/DownloadDoubleConversion.cmake
Outdated
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| ## Download facebook's folly library. |
There was a problem hiding this comment.
Download "double conv"... ? Looks like a copy-paste mistake.
There was a problem hiding this comment.
yes I didn't go through this PR to look for things like this. sorry for the careless mistakes.
cmake/patches/zookeeper.3.4.14.buf
Outdated
| @@ -0,0 +1,4 @@ | |||
| 3480c3480 | |||
| < static char buf[128]; | |||
There was a problem hiding this comment.
Thanks! I had updated zk but decided not to. You are correct thanks!
cmake/patches/zookeeper.3.4.14.cli
Outdated
| @@ -0,0 +1,4 @@ | |||
| 559c559 | |||
| < strncpy(cmd, argv[2]+4, sizeof(cmd)); | |||
|
|
||
| namespace hbase { | ||
|
|
||
| template <class T> |
| GroupAndSend(actions_, 1); | ||
| return collectAll(action2futures_); | ||
| // use the executor to convert he SemiFuture to a Future. | ||
| return std::move(collectAll(action2futures_)).via(cpu_pool_.get()); |
There was a problem hiding this comment.
whats the need for move() everywhere? Isn't a copy elided with RVO?
There was a problem hiding this comment.
yeah I'm surprised to see that there too. This is something I'm usually good at picking up in reviews of others' code, so it's kind of embarrassing that I didn't notice this myself, but I got pretty careless when I pulled this commit out from the original PR. Removed!
src/test/hbase-configuration-test.cc
Outdated
| HBaseConfigurationLoader loader; | ||
| hbase::optional<Configuration> conf = loader.LoadDefaultResources(); | ||
| ASSERT_TRUE(conf != none) << "No configuration object present."; | ||
| ASSERT_TRUE(conf ) << "No configuration object present."; |
There was a problem hiding this comment.
trailing space every where, missed it in a sed command? :-D
bharathv
left a comment
bharathv
left a comment
There was a problem hiding this comment.
Had a chance to apply your patch locally and try it out, running into this weird linker issue.
dependencies/facebook-folly-proj-install/lib/libfolly.a(AtFork.cpp.o): In function
folly::detail::(anonymous namespace)::AtForkList::AtForkList()': /hbase-native-client2/build/dependencies/facebook-folly-proj-src/folly/detail/AtFork.cpp:95: undefined reference topthread_atfork'
collect2: error: ld returned 1 exit status
CMakeFiles/simple-client.dir/build.make:139: recipe for target 'bin/examples/simple-client' failed
make[2]: *** [bin/examples/simple-client] Error 1
CMakeFiles/Makefile2:554: recipe for target 'CMakeFiles/simple-client.dir/all' failed
make[1]: *** [CMakeFiles/simple-client.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 96%] Building CXX object CMakeFiles/delete-test.dir/src/test/d
You don't run into this? I don't see anything the patch touches related to this.. I thought find_package(Threads) should transparently handle this and set CMAKE_THREAD_LIBS_INIT and I see it is being added to simple-client.. or may be I messed something up locally.
I'm back from vacation so I'll take a look with a fresh build environment. Thanks! |
I did not notice any linker issues. I noticed a few things that were poorly extracted from my rebase ( and pulling commits from the original PR ), so it could be related to that. I have only tried on U20, so I will try across distros and docker now. |
@bharathv I just checked my docker builds and they failed due to c++17. I anticipated this so I'll verify that and fix accordingly. |
bharathv
left a comment
bharathv
left a comment
There was a problem hiding this comment.
@phrocker I integrated the precommit now and you should see an update from the bot here shortly for this PR. I'd expect it to fail though (seems like something I broke in the master). So, just a heads up. I'll fix it soon.
Also, let me know once this ready for review, can take another pass.
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
phrocker
force-pushed
the
HBASE-23105
branch
2 times, most recently
from
6c7c22e to
62dc042
Compare
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
@bharathv Sorry for the delay. I'm rebasing and rebuilding. ran into some issues with boost so I'll correct those then push them up . Thanks! |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
@bharathv I relied on CI to run docker for me. that appears to be the issue. xenial doesn't have libfmt-dev -- so I'll have to download that manually or add it to the build to get it to work, which should be pretty easy. hopefully will check the pre-commit after or run yetus manually. thanks for the info on how to run it! |
bharathv
left a comment
bharathv
left a comment
There was a problem hiding this comment.
I'm running into this when compiling with the patch locally.
In file included from hbase-native-client/build/dependencies/facebook-folly-proj-install/include/folly/functional/ApplyTuple.h:25:0,
from hbase-native-client/build/dependencies/facebook-folly-proj-install/include/folly/hash/Hash.h:31,
from hbase-native-client/build/dependencies/facebook-folly-proj-install/include/folly/FBString.h:45,
from hbase-native-client/build/dependencies/facebook-folly-proj-install/include/folly/io/IOBuf.h:30,
from hbase-native-client/build/dependencies/facebook-fizz-proj-src/fizz/crypto/exchange/KeyExchange.h:12,
from hbase-native-client/build/dependencies/facebook-fizz-proj-src/fizz/crypto/exchange/X25519.h:11,
from hbase-native-client/build/dependencies/facebook-fizz-proj-src/fizz/crypto/exchange/X25519.cpp:9:
hbase-native-client/build/dependencies/facebook-folly-proj-install/include/folly/functional/Invoke.h:22:10: fatal error: boost/preprocessor/control/expr_iif.hpp: No such file or directory
#include <boost/preprocessor/control/expr_iif.hpp>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Looks like fizz compilation is not able to locate boost includes in a non-standard location needed by folly. Are you not running into this? Perhaps you have a boost OS package ?
| 46a47,54 | ||
| > | ||
| > if(NOT APPLE) | ||
| > if(NOT WIN32) |
There was a problem hiding this comment.
No worries, was just curious. ya, if (LINUX) would be simpler.
| target_link_libraries(simple-client ${KRB5_LIBRARIES}) | ||
| target_link_libraries(simple-client ${ZOOKEEPER_LIBRARIES}) | ||
| target_link_libraries(simple-client hbaseclient-static ${CMAKE_THREAD_LIBS_INIT}) | ||
| target_link_libraries(simple-client ${WHOLE_ARCHIVE} ${Boost_context} ${NO_WHOLE_ARCHIVE}) |
There was a problem hiding this comment.
Cool, fine to do a follow on patch.
| SOURCE_DIR "${BUILD_DIR}/dependencies/facebook-fizz-proj-src" | ||
| PATCH_COMMAND ${CMAKE_COMMAND} -E copy "${CMAKE_CURRENT_SOURCE_DIR}/cmake/folly/local/FindFolly.cmake" ${BUILD_DIR}/dependencies/facebook-fizz-proj-src/fizz/cmake/ | ||
| COMMAND ${CMAKE_COMMAND} -E copy "${CMAKE_CURRENT_SOURCE_DIR}/cmake/doubleconversion/local/FindDoubleConversion.cmake" ${BUILD_DIR}/dependencies/facebook-fizz-proj-src/fizz/cmake | ||
| COMMAND ${CMAKE_COMMAND} -E copy "${CMAKE_CURRENT_SOURCE_DIR}/cmake/boost/local/FindBoost.cmake" ${BUILD_DIR}/dependencies/facebook-fizz-proj-src/fizz/cmake |
There was a problem hiding this comment.
Doesn't look like FindBoost is being used anywhere AFAICT.
bharathv
left a comment
bharathv
left a comment
There was a problem hiding this comment.
I relied on CI to run docker for me. that appears to be the issue. xenial doesn't have libfmt-dev -- so I'll have to download that manually or add it to the build to get it to work, which should be pretty easy. hopefully will check the pre-commit after or run yetus manually. thanks for the info on how to run it!?.
Cool. Let me know know if you run into any issues with local docker run, happy to help.
bharathv
left a comment
bharathv
left a comment
There was a problem hiding this comment.
@phrocker I finally had a chance to apply this patch locally. I fixed some issues with folly/fizz etc compiling with the downloaded version of boost and then there was an issue with cyrussasl.
One last issue I'm running into is with boost linking of simple-client/load-client. Seems like a real head scratcher. Linker is not able to resolve boost symbols while 'nm' of the hbase client shows they are in the archive .. just wanted to share the update while I try out a few more things.
|
I think your following change also is related to the issue I'm running into, so perhaps you already know what the problem is
|
|
I figured out the issue, submitted a new PR with my patch on top #14. A quick summary of my patch addition
Lets see how the precommit goes. There was an issue with libfmt-dev not available for ubuntu 16.04 but that looks optional, so removed it (not sure though). @phrocker Feel free to apply my patch on top of your PR, I don't wan't to take credit for your work with one small final patch :-). |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
HBASE-24622: Download DoubleConversion and CyrusSasl hbase-24621: Update to C++17 ( and fix build failures as a result of this )
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
This PR had to be rebased as a part of HBASE-26737. Please shout if you need help getting your local checkout updated. |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
4 participants
and wangle and use std c++17