phrocker commented on a change in pull request #2: URL: https://github.com/apache/hbase-native-client/pull/2#discussion_r430331425
########## 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: Yep. I'll take a look. I updated folly ( which resulted in a lot of other changes ) because of this ` https://github.com/facebook/folly/blob/v2017.09.04.00/CMakeLists.txt#L24` My preference was to use CMake if and when possible but it definitely becomes difficult as we aim to control the dependency tree. ########## 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. 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 classpath information to be valid. libfmt issues are one I think I need to address. In the case of gtest did you use libgtest-dev? My hope is to control these ( like I started doing with Sodium ). The ultimate issue I would hope to avoid is not being in control of the dependency tree. I think having a build mode for libhabaseclient.so that allows for static linking ( with perhaps GLIB as the only dependency ) will allow better control over not only the dependency tree, but also allow the client to be used across a variety of systems without regard to dependencies on those machines. This would also allow python bindings to eventually be a breeze and they can be distributed via pypi with relative ease. 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. I think for the non-draft PR I would probably make the dependency reliance (BUILD_FB_DEPENDENCIES, BUILD_ZOOKEEPER) off by default. That would help to shrink the PR some as the need to download all dependencies and solve every problem is minimized. [1] https://github.com/apache/hbase-native-client/blob/master/bin/copy-version.sh ---------------------------------------------------------------- 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: us...@infra.apache.org
