phrocker commented on a change in pull request #2: URL: https://github.com/apache/hbase-native-client/pull/2#discussion_r429177939
########## 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: I believe the master branch had 2017.09.04, which gave me concern due to its age. This was one of two reasons why I submitted this as a draft ( the other being that not all tests passed ). It allows me to get feedback on this very item so thanks for pointing this out. 1) On one hand I can keep the versions as-is from master ( thus reducing or eliminating the code changes ). It would be a smaller PR, less to review, and if I were the reviewer I'd probably prefer that :) +1482 -382 is a pain to review IMO and the version bumps cause scope creep. 2) On the other hand bumping versions after such an age gap did require some code resolutions, which I think when the are combined in this PR give perspective. The other consideration for this when I approached this I'm on a much newer toolchain and am building outside of docker, so managing the numerous dependencies of folly/wangle caused compatibility concerns. I haven't fully validated that this will be a problem so going with #1 and splitting up this PR would make total sense to me. I can extract the code changes required for the version bump very easily, and ultimately this is why I posted it as a draft-- so I can get this feedback early and get opinions on this. Would appreciate @joshelser 's take too RE version bumping. Look forward to your responses! Thanks! ---------------------------------------------------------------- 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]
