bharathv commented on a change in pull request #2: URL: https://github.com/apache/hbase-native-client/pull/2#discussion_r430720072
########## 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: Agreed. ########## 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: > Yeah version is an issue I'd like to address with cmake getting a known tag. I manually created a version.h for the time being. I started working on a solution for this, but then the size of the PR would grow even more. > Finally, RE the copy_version [1] agree it's reliant on a relative path that probably won't exist for most people. A more sustainable solution would be to have a variable specifying an hbase release target, and we generate the version file from the tag. Thanks. To keep things simple, IMHO it is also reasonable to pull the version.h from hbase source in the parent directory (basically the current approach). Just that mvn compilation in hbase project is not generating that header file. Let me know if you think this is a reasonable approach and I can fix it in a separate patch and you can integrate it with your change (that'll keep your changes simple for now). > My hope is to leverage an internal java project that runs a mini cluster and relies on maven to build a jar we can run for integration tests. Right now there is a relative path expecting class-path information to be valid. Agreed. > libfmt issues are one I think I need to address. In the case of gtest did you use libgtest-dev? Yes, I was using the dev library. I think this is a known problem, see the second answer on this page. https://stackoverflow.com/questions/13513905/how-to-set-up-googletest-as-a-shared-library-on-linux > but also allow the client to be used across a variety of systems without regard to dependencies on those machines. +1. I think an ideal end state would be a prebuilt toolchain for various commonly used platforms we just download them on the fly during compilation. ---------------------------------------------------------------- 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]
