[GitHub] [hbase-native-client] bharathv commented on a change in pull request #2: HBASE-24400: Fixup cmake infrastructure to allow dependencies to be built locally

[GitBox <gi...@apache.org>]

bharathv commented on a change in pull request #2:
URL: https://github.com/apache/hbase-native-client/pull/2#discussion_r429061722



##########
File path: .gitignore
##########
@@ -3,6 +3,9 @@
 *.lo
 *.o
 
+# Proto files
+src/hbase/if/*.proto

Review comment:
       copying of protos can be rolled into the cmake script instead of manually invoking copy-protos script?

##########
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:
       Any reason you bumped up the versions from the master branch? Probably better to use the same versions and upgrade them in a separate commit to limit the scope of changes?

##########
File path: cmake/DownloadWangle.cmake
##########
@@ -0,0 +1,41 @@
+# 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 wangle library. 
+## SOURCE_DIR is typically the cmake source directory
+## BINARY_DIR is the build directory, typically 'build'
+
+function(download_wangle SOURCE_DIR BUILD_DIR)
+
+	
+	ExternalProject_Add(
+		facebook-wangle-proj
+		GIT_REPOSITORY "https://github.com/facebook/wangle.git"
+		GIT_TAG "v2020.05.18.00"

Review comment:
       same as above.

##########
File path: cmake/DownloadFizz.cmake
##########
@@ -0,0 +1,41 @@
+# 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 fizz library. 
+## SOURCE_DIR is typically the cmake source directory
+## BINARY_DIR is the build directory, typically 'build'
+
+function(download_fizz SOURCE_DIR BUILD_DIR)
+
+	
+	ExternalProject_Add(
+		facebook-fizz-proj
+		GIT_REPOSITORY "https://github.com/facebookincubator/fizz.git"
+		GIT_TAG "v2020.05.18.00"

Review comment:
       nit: Define all the thirdparty version variables in one place in the CMakeLists? Easier to make changes in the future if someone wants to try out a specific git tag..

##########
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:
       Interestingly I'm not able to build this git tag, tried it separately too, didn't you run into this? (file name FindFmt vs Findfmt)
   
   ```
   CMake Error at CMake/folly-deps.cmake:227 (find_package):
     No "Findfmt.cmake" found in CMAKE_MODULE_PATH.
   Call Stack (most recent call first):
     CMakeLists.txt:113 (include)
   
   
   CMake Warning (dev) at CMake/folly-deps.cmake:227 (find_package):
     Findfmt.cmake must either be part of this project itself, in this case
     adjust CMAKE_MODULE_PATH so that it points to the correct location inside
     its source tree.
   
     Or it must be installed by a package which has already been found via
     find_package().  In this case make sure that package has indeed been found
     and adjust CMAKE_MODULE_PATH to contain the location where that package has
     installed Findfmt.cmake.  This must be a location provided by that package.
     This error in general means that the buildsystem of this project is relying
     on a Find-module without ensuring that it is actually available.
   
   Call Stack (most recent call first):
     CMakeLists.txt:113 (include)
   This warning is for project developers.  Use -Wno-dev to suppress it.
   
   
   ```




----------------------------------------------------------------
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:
users@infra.apache.org