bharathv commented on a change in pull request #2: URL: https://github.com/apache/hbase-native-client/pull/2#discussion_r429485955
########## File path: cmake/DownloadFolly.cmake ########## @@ -0,0 +1,39 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +## Download facebook's folly library. +## SOURCE_DIR is typically the cmake source directory +## BINARY_DIR is the build directory, typically 'build' + +function(download_folly SOURCE_DIR BINARY_DIR) + + + ExternalProject_Add( + facebook-folly-proj + GIT_REPOSITORY "https://github.com/facebook/folly.git"; + GIT_TAG "v2020.05.18.00" + SOURCE_DIR "${BINARY_DIR}/dependencies/facebook-folly-proj-src" + CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS} Review comment: I had libfmt-dev too but I probably messed up something. I tried again and I could get past that, but then I ran into this.. ``` In file included from hbase-native-client/build/dependencies/facebook-folly-proj-src/folly/logging/Logger.h:22:0, from hbase-native-client/build/dependencies/facebook-folly-proj-src/folly/logging/BridgeFromGoogleLogging.cpp:20: hbase-native-client/build/dependencies/facebook-folly-proj-src/folly/logging/LogStreamProcessor.h:19:10: fatal error: fmt/core.h: No such file or directory #include <fmt/core.h> ^~~~~~~~~~~~ ``` Turns out the problem is this https://github.com/facebook/folly/pull/1263. libfmt-dev in ubuntu 18.04 doesn't install the right headers. If we end up dockerizing this on ubuntu, we may run into this problem in which case we may have to install fmt dependency like other dependencies, just FYI. ########## File path: cmake/DownloadFolly.cmake ########## @@ -0,0 +1,39 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +## Download facebook's folly library. +## SOURCE_DIR is typically the cmake source directory +## BINARY_DIR is the build directory, typically 'build' + +function(download_folly SOURCE_DIR BINARY_DIR) + + + ExternalProject_Add( + facebook-folly-proj + GIT_REPOSITORY "https://github.com/facebook/folly.git"; + GIT_TAG "v2020.05.18.00" + SOURCE_DIR "${BINARY_DIR}/dependencies/facebook-folly-proj-src" + CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS} Review comment: Next problem is the "version.h"... ``` cp: cannot stat '../bin/../../hbase-common/target/generated-sources/native/utils//*': No such file or directory CMakeFiles/copy_version_h.dir/build.make:57: recipe for target 'CMakeFiles/copy_version_h' failed make[2]: *** [CMakeFiles/copy_version_h] Error 1 CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/copy_version_h.dir/all' failed make[1]: *** [CMakeFiles/copy_version_h.dir/all] Error 2 Makefile:140: recipe for target 'all' failed make: *** [all] Error 2 ``` I don't see hbase-common in the hbase repo generating a native header file. It only generates a Version.java file.. the following code looks sketchy to me. How did get the version.h in your case? ``` # Copy the version.h generated from hbase-common/src/saveVersion.sh script via the mvn build BIN_DIR=$(dirname "$0") VERSION_SOURCE_DIR="${BIN_DIR}/../../hbase-common/target/generated-sources/native/utils/" VERSION_DEST_DIR="${BIN_DIR}/../include/hbase/utils/" cp $VERSION_SOURCE_DIR/* $VERSION_DEST_DIR/ ``` (I looked at the git history for that file and it looks like it never generated a header file, I don't know what this is referring to, also the directory paths look wrong). ########## File path: cmake/DownloadFolly.cmake ########## @@ -0,0 +1,39 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +## Download facebook's folly library. +## SOURCE_DIR is typically the cmake source directory +## BINARY_DIR is the build directory, typically 'build' + +function(download_folly SOURCE_DIR BINARY_DIR) + + + ExternalProject_Add( + facebook-folly-proj + GIT_REPOSITORY "https://github.com/facebook/folly.git"; + GIT_TAG "v2020.05.18.00" Review comment: Ya, I was more talking about the scope creep and a reviewing perspective. I'm totally up for bumping the version but I think it'd be nice to do it in a follow up patch _ if it is not too much work for you_. I'll let @joshelser weigh in, he has more context here than I do. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
