You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/05/09 09:10:10 UTC
[GitHub] [incubator-kvrocks] PragmaTwice opened a new pull request, #564: Use FetchContent instead of ExternalProject and git submodules in CMake
PragmaTwice opened a new pull request, #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564
Check https://github.com/apache/incubator-kvrocks/issues/562 and https://github.com/apache/incubator-kvrocks/issues/561 for more details.
@tisonkun
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123707713
OK, many thanks for @tisonkun help, I'm very glad to merge and summary.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123160640
I feel that updating snappy from 1.1.7 to 1.1.9 is another good solution to this problem, which removes the need for us to add `FindGTest.cmake`. What do you think is better to do? @git-hulk
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123562228
I just tested the build process in cmake 3.11, and error occurred because a feature [here](https://cmake.org/cmake/help/latest/command/install.html) in cmake 3.13 is used.
> New in version 3.13: [install(TARGETS)](https://cmake.org/cmake/help/latest/command/install.html#install-targets) can install targets that were created in other directories. When using such cross-directory install rules, running make install (or similar) from a subdirectory will not guarantee that targets from other directories are up-to-date. You can use [target_link_libraries()](https://cmake.org/cmake/help/latest/command/target_link_libraries.html#command:target_link_libraries) or [add_dependencies()](https://cmake.org/cmake/help/latest/command/add_dependencies.html#command:add_dependencies) to ensure that such out-of-directory targets are built before the subdirectory-specific install rules are run.
And cmake 3.13 works well. I will change the required cmake version from 3.16 to 3.13.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123165219
> I feel that updating snappy from 1.1.7 to 1.1.9 is another good solution to this problem, which removes the need for us to add `FindGTest.cmake`. What do you think is better to do? @git-hulk
it's good to me
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r869130333
##########
cmake/rocksdb.cmake:
##########
@@ -15,67 +15,34 @@
# specific language governing permissions and limitations
# under the License.
-if (NOT __ROCKSDB_INCLUDED)
- set(__ROCKSDB_INCLUDED TRUE)
+include_guard()
- find_package(Threads)
- set(CMAKE_INSTALL_LIBDIR lib)
- # build directory
- set(rocksdb_PREFIX ${CMAKE_BUILD_DIRECTORY}/external/rocksdb-prefix)
- # install directory
- set(rocksdb_INSTALL ${CMAKE_BUILD_DIRECTORY}/external/rocksdb-install)
- set(COMPILE_WITH_JEMALLOC ON)
+set(COMPILE_WITH_JEMALLOC ON)
- if (UNIX)
- set(ROCKSDB_EXTRA_COMPILER_FLAGS "-fPIC")
- endif()
+if (DISABLE_JEMALLOC)
+ set(COMPILE_WITH_JEMALLOC OFF)
+endif()
- set(ROCKSDB_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${ROCKSDB_EXTRA_COMPILER_FLAGS}")
- set(ROCKSDB_C_FLAGS "${CMAKE_C_FLAGS} ${ROCKSDB_EXTRA_COMPILER_FLAGS}")
- if (NOT DISABLE_JEMALLOC)
- list(APPEND rocksdb_DEPENDS jemalloc)
- list(APPEND rocksdb_DEPENDS snappy)
- set(JEMALLOC_ROOT_DIR ${jemalloc_INSTALL})
- else()
- set(rocksdb_DEPENDS "snappy")
- set(COMPILE_WITH_JEMALLOC OFF)
- endif()
- ExternalProject_Add(rocksdb
- DEPENDS ${rocksdb_DEPENDS}
- PREFIX ${rocksdb_PREFIX}
- #GIT_REPOSITORY "https://github.com/facebook/rocksdb"
- #GIT_TAG "v5.15.10"
- SOURCE_DIR ${PROJECT_SOURCE_DIR}/external/rocksdb
- INSTALL_DIR ${rocksdb_INSTALL}
- CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
- -DCMAKE_INSTALL_LIBDIR=${CMAKE_INSTALL_LIBDIR}
- -DCMAKE_INSTALL_PREFIX=${rocksdb_INSTALL}
- -DBUILD_SHARED_LIBS=OFF
- -DBUILD_STATIC_LIBS=ON
- -DBUILD_PACKAGING=OFF
- -DBUILD_TESTING=OFF
- -DBUILD_NC_TESTS=OFF
- -DBUILD_CONFIG_TESTS=OFF
- -DINSTALL_HEADERS=ON
- -DCMAKE_C_FLAGS=${ROCKSDB_C_FLAGS}
- -DCMAKE_CXX_FLAGS=${ROCKSDB_CXX_FLAGS}
- -DCMAKE_PREFIX_PATH=${snappy_INSTALL}
- -DJEMALLOC_ROOT_DIR=${JEMALLOC_ROOT_DIR}
- -DFAIL_ON_WARNINGS=OFF
- -DWITH_TESTS=OFF
- -DWITH_SNAPPY=ON
- -DWITH_TOOLS=OFF
- -DWITH_GFLAGS=OFF
- -DUSE_RTTI=ON
- -DWITH_JEMALLOC=${COMPILE_WITH_JEMALLOC}
- LOG_DOWNLOAD 1
- LOG_CONFIGURE 1
- LOG_INSTALL 1
- )
+include(FetchContent)
- include(GNUInstallDirs)
- set(rocksdb_FOUND TRUE)
- set(rocksdb_INCLUDE_DIRS ${rocksdb_INSTALL}/include)
- set(rocksdb_LIBRARIES ${rocksdb_INSTALL}/${CMAKE_INSTALL_LIBDIR}/librocksdb.a)
-endif()
+FetchContent_Declare(rocksdb
+ GIT_REPOSITORY https://github.com/facebook/rocksdb
+ GIT_TAG v6.29.5
+)
+
+include(cmake/utils.cmake)
+
+FetchContent_MakeAvailableWithArgs(rocksdb
Review Comment:
`FetchContent` commands will be executed on configuration stage, not the build stage, so there is no `DEPENDS` parameter (and the FetchContent declared names are not cmake targets actually, so no `add_dependencies`). The execution order plays the role of `DEPENDS`, you can check [here](https://cmake.org/cmake/help/latest/module/FetchContent.html#examples) to find details.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r869059575
##########
CMakeLists.txt:
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-cmake_minimum_required(VERSION 3.10)
+cmake_minimum_required(VERSION 3.16)
Review Comment:
Do we use any instructions that required 3.16? I saw fetch content instruction required 3.11+.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r869156806
##########
CMakeLists.txt:
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-cmake_minimum_required(VERSION 3.10)
+cmake_minimum_required(VERSION 3.16)
Review Comment:
OK, thanks for your explaination.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123311867
I am not familiar with this topic, but it seems a better way.
BTW, i ever wanted to remove Makefile, only keep cmake, since we need to install some libs firstly if we use make.
@git-hulk told me we need a high version cmake which may be not convenient for users. I don't know 3.16 is high? If not, maybe in the future, we can only support cmake.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123143775
@PragmaTwice so, how can I build kvrocks with cmake now? You may add a section on README also.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123725008
Thanks @PragmaTwice great work again.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r869161481
##########
cmake/rocksdb.cmake:
##########
@@ -15,67 +15,34 @@
# specific language governing permissions and limitations
# under the License.
-if (NOT __ROCKSDB_INCLUDED)
- set(__ROCKSDB_INCLUDED TRUE)
+include_guard()
- find_package(Threads)
- set(CMAKE_INSTALL_LIBDIR lib)
- # build directory
- set(rocksdb_PREFIX ${CMAKE_BUILD_DIRECTORY}/external/rocksdb-prefix)
- # install directory
- set(rocksdb_INSTALL ${CMAKE_BUILD_DIRECTORY}/external/rocksdb-install)
- set(COMPILE_WITH_JEMALLOC ON)
+set(COMPILE_WITH_JEMALLOC ON)
- if (UNIX)
- set(ROCKSDB_EXTRA_COMPILER_FLAGS "-fPIC")
- endif()
+if (DISABLE_JEMALLOC)
+ set(COMPILE_WITH_JEMALLOC OFF)
+endif()
- set(ROCKSDB_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${ROCKSDB_EXTRA_COMPILER_FLAGS}")
- set(ROCKSDB_C_FLAGS "${CMAKE_C_FLAGS} ${ROCKSDB_EXTRA_COMPILER_FLAGS}")
- if (NOT DISABLE_JEMALLOC)
- list(APPEND rocksdb_DEPENDS jemalloc)
- list(APPEND rocksdb_DEPENDS snappy)
- set(JEMALLOC_ROOT_DIR ${jemalloc_INSTALL})
- else()
- set(rocksdb_DEPENDS "snappy")
- set(COMPILE_WITH_JEMALLOC OFF)
- endif()
- ExternalProject_Add(rocksdb
- DEPENDS ${rocksdb_DEPENDS}
- PREFIX ${rocksdb_PREFIX}
- #GIT_REPOSITORY "https://github.com/facebook/rocksdb"
- #GIT_TAG "v5.15.10"
- SOURCE_DIR ${PROJECT_SOURCE_DIR}/external/rocksdb
- INSTALL_DIR ${rocksdb_INSTALL}
- CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
- -DCMAKE_INSTALL_LIBDIR=${CMAKE_INSTALL_LIBDIR}
- -DCMAKE_INSTALL_PREFIX=${rocksdb_INSTALL}
- -DBUILD_SHARED_LIBS=OFF
- -DBUILD_STATIC_LIBS=ON
- -DBUILD_PACKAGING=OFF
- -DBUILD_TESTING=OFF
- -DBUILD_NC_TESTS=OFF
- -DBUILD_CONFIG_TESTS=OFF
- -DINSTALL_HEADERS=ON
- -DCMAKE_C_FLAGS=${ROCKSDB_C_FLAGS}
- -DCMAKE_CXX_FLAGS=${ROCKSDB_CXX_FLAGS}
- -DCMAKE_PREFIX_PATH=${snappy_INSTALL}
- -DJEMALLOC_ROOT_DIR=${JEMALLOC_ROOT_DIR}
- -DFAIL_ON_WARNINGS=OFF
- -DWITH_TESTS=OFF
- -DWITH_SNAPPY=ON
- -DWITH_TOOLS=OFF
- -DWITH_GFLAGS=OFF
- -DUSE_RTTI=ON
- -DWITH_JEMALLOC=${COMPILE_WITH_JEMALLOC}
- LOG_DOWNLOAD 1
- LOG_CONFIGURE 1
- LOG_INSTALL 1
- )
+include(FetchContent)
- include(GNUInstallDirs)
- set(rocksdb_FOUND TRUE)
- set(rocksdb_INCLUDE_DIRS ${rocksdb_INSTALL}/include)
- set(rocksdb_LIBRARIES ${rocksdb_INSTALL}/${CMAKE_INSTALL_LIBDIR}/librocksdb.a)
-endif()
+FetchContent_Declare(rocksdb
+ GIT_REPOSITORY https://github.com/facebook/rocksdb
+ GIT_TAG v6.29.5
+)
+
+include(cmake/utils.cmake)
+
+FetchContent_MakeAvailableWithArgs(rocksdb
Review Comment:
We specified the include and library path in jemalloc cmake
and referenced those paths in rocksdb cmake when enabling the jemalloc.
That's what I commented here.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r870164995
##########
build.sh:
##########
@@ -24,7 +24,7 @@ fi
BUILD_DIR=$1
WORKING_DIR=$(pwd)
CMAKE_INSTALL_DIR=$WORKING_DIR/$BUILD_DIR/cmake
-CMAKE_REQUIRE_VERSION="3.13.0"
+CMAKE_REQUIRE_VERSION="3.16.0"
Review Comment:
```suggestion
CMAKE_REQUIRE_VERSION="3.13.0"
```
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r869130333
##########
cmake/rocksdb.cmake:
##########
@@ -15,67 +15,34 @@
# specific language governing permissions and limitations
# under the License.
-if (NOT __ROCKSDB_INCLUDED)
- set(__ROCKSDB_INCLUDED TRUE)
+include_guard()
- find_package(Threads)
- set(CMAKE_INSTALL_LIBDIR lib)
- # build directory
- set(rocksdb_PREFIX ${CMAKE_BUILD_DIRECTORY}/external/rocksdb-prefix)
- # install directory
- set(rocksdb_INSTALL ${CMAKE_BUILD_DIRECTORY}/external/rocksdb-install)
- set(COMPILE_WITH_JEMALLOC ON)
+set(COMPILE_WITH_JEMALLOC ON)
- if (UNIX)
- set(ROCKSDB_EXTRA_COMPILER_FLAGS "-fPIC")
- endif()
+if (DISABLE_JEMALLOC)
+ set(COMPILE_WITH_JEMALLOC OFF)
+endif()
- set(ROCKSDB_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${ROCKSDB_EXTRA_COMPILER_FLAGS}")
- set(ROCKSDB_C_FLAGS "${CMAKE_C_FLAGS} ${ROCKSDB_EXTRA_COMPILER_FLAGS}")
- if (NOT DISABLE_JEMALLOC)
- list(APPEND rocksdb_DEPENDS jemalloc)
- list(APPEND rocksdb_DEPENDS snappy)
- set(JEMALLOC_ROOT_DIR ${jemalloc_INSTALL})
- else()
- set(rocksdb_DEPENDS "snappy")
- set(COMPILE_WITH_JEMALLOC OFF)
- endif()
- ExternalProject_Add(rocksdb
- DEPENDS ${rocksdb_DEPENDS}
- PREFIX ${rocksdb_PREFIX}
- #GIT_REPOSITORY "https://github.com/facebook/rocksdb"
- #GIT_TAG "v5.15.10"
- SOURCE_DIR ${PROJECT_SOURCE_DIR}/external/rocksdb
- INSTALL_DIR ${rocksdb_INSTALL}
- CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
- -DCMAKE_INSTALL_LIBDIR=${CMAKE_INSTALL_LIBDIR}
- -DCMAKE_INSTALL_PREFIX=${rocksdb_INSTALL}
- -DBUILD_SHARED_LIBS=OFF
- -DBUILD_STATIC_LIBS=ON
- -DBUILD_PACKAGING=OFF
- -DBUILD_TESTING=OFF
- -DBUILD_NC_TESTS=OFF
- -DBUILD_CONFIG_TESTS=OFF
- -DINSTALL_HEADERS=ON
- -DCMAKE_C_FLAGS=${ROCKSDB_C_FLAGS}
- -DCMAKE_CXX_FLAGS=${ROCKSDB_CXX_FLAGS}
- -DCMAKE_PREFIX_PATH=${snappy_INSTALL}
- -DJEMALLOC_ROOT_DIR=${JEMALLOC_ROOT_DIR}
- -DFAIL_ON_WARNINGS=OFF
- -DWITH_TESTS=OFF
- -DWITH_SNAPPY=ON
- -DWITH_TOOLS=OFF
- -DWITH_GFLAGS=OFF
- -DUSE_RTTI=ON
- -DWITH_JEMALLOC=${COMPILE_WITH_JEMALLOC}
- LOG_DOWNLOAD 1
- LOG_CONFIGURE 1
- LOG_INSTALL 1
- )
+include(FetchContent)
- include(GNUInstallDirs)
- set(rocksdb_FOUND TRUE)
- set(rocksdb_INCLUDE_DIRS ${rocksdb_INSTALL}/include)
- set(rocksdb_LIBRARIES ${rocksdb_INSTALL}/${CMAKE_INSTALL_LIBDIR}/librocksdb.a)
-endif()
+FetchContent_Declare(rocksdb
+ GIT_REPOSITORY https://github.com/facebook/rocksdb
+ GIT_TAG v6.29.5
+)
+
+include(cmake/utils.cmake)
+
+FetchContent_MakeAvailableWithArgs(rocksdb
Review Comment:
`FetchContent` commands will be executed on configuration stage, not the build stage, so there is no `DEPENDS` parameter (and the FetchContent declared names are not cmake targets actually, so no `add_dependencies`). The execution order plays the role of `DEPENDS`, you can check [here](https://cmake.org/cmake/help/latest/module/FetchContent.html#examples) to find details.
I will figure out why the CI failed :)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123152051
> It seems that current script make conflict with system-wise library?
Snappy use `find_package(gtest)` mechanism to locate gtest libs (in https://github.com/google/snappy/blob/b02bfa754ebf27921d8da3bd2517eab445b84ff9/CMakeLists.txt#L40).
The newer version of Snappy do not use `find_package` any more, but for the current version, we probably should add `FindGTest.cmake` to tell Snappy we have already build our own gtest.
BTW, we acctually defined `SNAPPY_BUILD_TESTS`, so gtest is useless for Snappy.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1122329581
@PragmaTwice please fix CI failure and ping me when this PR ready. I'll do a manual test on macos env.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r870124635
##########
CMakeLists.txt:
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-cmake_minimum_required(VERSION 3.10)
+cmake_minimum_required(VERSION 3.16)
Review Comment:
i think 20.04 also is newer, we still use ubuntu 18.04 in CI. For some enterprise user, they even still use CentOS 6.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123527391
CMake 3.16.0 is released at Nov 26, 2019
CMake 3.11.0 is released at Dec 01, 2018
no much difference 😂
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1121972637
After this PR, the cmake build process will not depend on any files in the `externel` directory. However, in order to make Makefiles still work, I will restore the `externel` directory (which is currently deleted in this pr).
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1121976445
Thanks @PragmaTwice. I think we can do that first if it's too complex.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123125830
@PragmaTwice Last failure caused by didn't link `-pthread`. Can have a test on local with running `sh build.sh _build`, so you are NOT need to wait for the CI feedback.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r869156806
##########
CMakeLists.txt:
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-cmake_minimum_required(VERSION 3.10)
+cmake_minimum_required(VERSION 3.16)
Review Comment:
OK, thanks for your explain.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123146653
I try to run with:
```bash
mkdir cmake-build-debug ; cd cmake-build-debug
cmake -DCMAKE_BUILD_TYPE=Release ..
```
which failed and complained:
```
CMake Error at /usr/local/lib/cmake/GTest/GTestTargets.cmake:37 (message):
Some (but not all) targets in this export set were already defined.
Targets Defined: GTest::gtest;GTest::gtest_main
Targets not yet defined: GTest::gmock;GTest::gmock_main
Call Stack (most recent call first):
/usr/local/lib/cmake/GTest/GTestConfig.cmake:32 (include)
/usr/local/Cellar/cmake/3.23.1/share/cmake/Modules/FindGTest.cmake:194 (find_package)
cmake-build-debug/_deps/snappy-src/CMakeLists.txt:40 (find_package)
```
It seems that current script make conflict with system-wise library?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123136602
> @PragmaTwice please fix CI failure and ping me when this PR ready. I'll do a manual test on macos env.
@tisonkun CI has been fixed now.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123379345
@git-hulk @ShooterIT as stated in https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1121972637, we may proceed this patch to introduce a `FetchContent` based cmake build system. And remove Makefile build system in a separated PR.
cc @PragmaTwice do I get it right?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123384217
Overall looks good, will take a new pass again and have a try on my side tonight.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123483038
@ShooterIT for choosing a newer version, @PragmaTwice explain in this https://github.com/apache/incubator-kvrocks/pull/564#discussion_r869138940.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r869130333
##########
cmake/rocksdb.cmake:
##########
@@ -15,67 +15,34 @@
# specific language governing permissions and limitations
# under the License.
-if (NOT __ROCKSDB_INCLUDED)
- set(__ROCKSDB_INCLUDED TRUE)
+include_guard()
- find_package(Threads)
- set(CMAKE_INSTALL_LIBDIR lib)
- # build directory
- set(rocksdb_PREFIX ${CMAKE_BUILD_DIRECTORY}/external/rocksdb-prefix)
- # install directory
- set(rocksdb_INSTALL ${CMAKE_BUILD_DIRECTORY}/external/rocksdb-install)
- set(COMPILE_WITH_JEMALLOC ON)
+set(COMPILE_WITH_JEMALLOC ON)
- if (UNIX)
- set(ROCKSDB_EXTRA_COMPILER_FLAGS "-fPIC")
- endif()
+if (DISABLE_JEMALLOC)
+ set(COMPILE_WITH_JEMALLOC OFF)
+endif()
- set(ROCKSDB_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${ROCKSDB_EXTRA_COMPILER_FLAGS}")
- set(ROCKSDB_C_FLAGS "${CMAKE_C_FLAGS} ${ROCKSDB_EXTRA_COMPILER_FLAGS}")
- if (NOT DISABLE_JEMALLOC)
- list(APPEND rocksdb_DEPENDS jemalloc)
- list(APPEND rocksdb_DEPENDS snappy)
- set(JEMALLOC_ROOT_DIR ${jemalloc_INSTALL})
- else()
- set(rocksdb_DEPENDS "snappy")
- set(COMPILE_WITH_JEMALLOC OFF)
- endif()
- ExternalProject_Add(rocksdb
- DEPENDS ${rocksdb_DEPENDS}
- PREFIX ${rocksdb_PREFIX}
- #GIT_REPOSITORY "https://github.com/facebook/rocksdb"
- #GIT_TAG "v5.15.10"
- SOURCE_DIR ${PROJECT_SOURCE_DIR}/external/rocksdb
- INSTALL_DIR ${rocksdb_INSTALL}
- CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
- -DCMAKE_INSTALL_LIBDIR=${CMAKE_INSTALL_LIBDIR}
- -DCMAKE_INSTALL_PREFIX=${rocksdb_INSTALL}
- -DBUILD_SHARED_LIBS=OFF
- -DBUILD_STATIC_LIBS=ON
- -DBUILD_PACKAGING=OFF
- -DBUILD_TESTING=OFF
- -DBUILD_NC_TESTS=OFF
- -DBUILD_CONFIG_TESTS=OFF
- -DINSTALL_HEADERS=ON
- -DCMAKE_C_FLAGS=${ROCKSDB_C_FLAGS}
- -DCMAKE_CXX_FLAGS=${ROCKSDB_CXX_FLAGS}
- -DCMAKE_PREFIX_PATH=${snappy_INSTALL}
- -DJEMALLOC_ROOT_DIR=${JEMALLOC_ROOT_DIR}
- -DFAIL_ON_WARNINGS=OFF
- -DWITH_TESTS=OFF
- -DWITH_SNAPPY=ON
- -DWITH_TOOLS=OFF
- -DWITH_GFLAGS=OFF
- -DUSE_RTTI=ON
- -DWITH_JEMALLOC=${COMPILE_WITH_JEMALLOC}
- LOG_DOWNLOAD 1
- LOG_CONFIGURE 1
- LOG_INSTALL 1
- )
+include(FetchContent)
- include(GNUInstallDirs)
- set(rocksdb_FOUND TRUE)
- set(rocksdb_INCLUDE_DIRS ${rocksdb_INSTALL}/include)
- set(rocksdb_LIBRARIES ${rocksdb_INSTALL}/${CMAKE_INSTALL_LIBDIR}/librocksdb.a)
-endif()
+FetchContent_Declare(rocksdb
+ GIT_REPOSITORY https://github.com/facebook/rocksdb
+ GIT_TAG v6.29.5
+)
+
+include(cmake/utils.cmake)
+
+FetchContent_MakeAvailableWithArgs(rocksdb
Review Comment:
`FetchContent` commands will be executed on configuration stage, not the build stage, so there is no `DEPENDS` parameter (and the FetchContent declared names are not cmake targets actually, so no `add_dependencies`). The execution order plays the role of `DEPENDS`, you can check [here](https://cmake.org/cmake/help/latest/module/FetchContent.html#id5) to find details.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r869058412
##########
cmake/rocksdb.cmake:
##########
@@ -15,67 +15,34 @@
# specific language governing permissions and limitations
# under the License.
-if (NOT __ROCKSDB_INCLUDED)
- set(__ROCKSDB_INCLUDED TRUE)
+include_guard()
- find_package(Threads)
- set(CMAKE_INSTALL_LIBDIR lib)
- # build directory
- set(rocksdb_PREFIX ${CMAKE_BUILD_DIRECTORY}/external/rocksdb-prefix)
- # install directory
- set(rocksdb_INSTALL ${CMAKE_BUILD_DIRECTORY}/external/rocksdb-install)
- set(COMPILE_WITH_JEMALLOC ON)
+set(COMPILE_WITH_JEMALLOC ON)
- if (UNIX)
- set(ROCKSDB_EXTRA_COMPILER_FLAGS "-fPIC")
- endif()
+if (DISABLE_JEMALLOC)
+ set(COMPILE_WITH_JEMALLOC OFF)
+endif()
- set(ROCKSDB_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${ROCKSDB_EXTRA_COMPILER_FLAGS}")
- set(ROCKSDB_C_FLAGS "${CMAKE_C_FLAGS} ${ROCKSDB_EXTRA_COMPILER_FLAGS}")
- if (NOT DISABLE_JEMALLOC)
- list(APPEND rocksdb_DEPENDS jemalloc)
- list(APPEND rocksdb_DEPENDS snappy)
- set(JEMALLOC_ROOT_DIR ${jemalloc_INSTALL})
- else()
- set(rocksdb_DEPENDS "snappy")
- set(COMPILE_WITH_JEMALLOC OFF)
- endif()
- ExternalProject_Add(rocksdb
- DEPENDS ${rocksdb_DEPENDS}
- PREFIX ${rocksdb_PREFIX}
- #GIT_REPOSITORY "https://github.com/facebook/rocksdb"
- #GIT_TAG "v5.15.10"
- SOURCE_DIR ${PROJECT_SOURCE_DIR}/external/rocksdb
- INSTALL_DIR ${rocksdb_INSTALL}
- CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
- -DCMAKE_INSTALL_LIBDIR=${CMAKE_INSTALL_LIBDIR}
- -DCMAKE_INSTALL_PREFIX=${rocksdb_INSTALL}
- -DBUILD_SHARED_LIBS=OFF
- -DBUILD_STATIC_LIBS=ON
- -DBUILD_PACKAGING=OFF
- -DBUILD_TESTING=OFF
- -DBUILD_NC_TESTS=OFF
- -DBUILD_CONFIG_TESTS=OFF
- -DINSTALL_HEADERS=ON
- -DCMAKE_C_FLAGS=${ROCKSDB_C_FLAGS}
- -DCMAKE_CXX_FLAGS=${ROCKSDB_CXX_FLAGS}
- -DCMAKE_PREFIX_PATH=${snappy_INSTALL}
- -DJEMALLOC_ROOT_DIR=${JEMALLOC_ROOT_DIR}
- -DFAIL_ON_WARNINGS=OFF
- -DWITH_TESTS=OFF
- -DWITH_SNAPPY=ON
- -DWITH_TOOLS=OFF
- -DWITH_GFLAGS=OFF
- -DUSE_RTTI=ON
- -DWITH_JEMALLOC=${COMPILE_WITH_JEMALLOC}
- LOG_DOWNLOAD 1
- LOG_CONFIGURE 1
- LOG_INSTALL 1
- )
+include(FetchContent)
- include(GNUInstallDirs)
- set(rocksdb_FOUND TRUE)
- set(rocksdb_INCLUDE_DIRS ${rocksdb_INSTALL}/include)
- set(rocksdb_LIBRARIES ${rocksdb_INSTALL}/${CMAKE_INSTALL_LIBDIR}/librocksdb.a)
-endif()
+FetchContent_Declare(rocksdb
+ GIT_REPOSITORY https://github.com/facebook/rocksdb
+ GIT_TAG v6.29.5
+)
+
+include(cmake/utils.cmake)
+
+FetchContent_MakeAvailableWithArgs(rocksdb
Review Comment:
rocksdb depends on jemalloc and snappy, so we should also do that like the old CMake file.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123444089
> @git-hulk @ShooterIT as stated in [#564 (comment)](https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1121972637), we may proceed this patch to introduce a `FetchContent` based cmake build system. And remove Makefile build system in a separated PR.
>
> cc @PragmaTwice do I get it right?
Yeah, I I also think it would be good to remove makefiles after this PR.
> BTW, i ever wanted to remove Makefile, only keep cmake, since we need to install some libs firstly if we use make. @git-hulk told me we need a high version cmake which may be not convenient for users. I don't know 3.16 is high? If not, maybe in the future, we can only support cmake.
You can easily get cmake binaries from https://github.com/Kitware/CMake/releases (I do not try it in an old distro environment like centos or debian stable, but I do know some glibc linking issue will appear while the glibc version in build environment is higher than it in user's environment). I think the most compatible way to use cmake is to build it by your own.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123475227
> Yeah, I also think it would be good to remove makefiles after this PR.
OK, thanks
> I can lower the required cmake version to 3.11 (if it is confirmed that this version contains all the features used), if that helps with compatibility.
If cmake version 3.11 can work well, i vote 3.11
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r868999029
##########
cmake/utils.cmake:
##########
@@ -0,0 +1,26 @@
+include_guard()
Review Comment:
I'll do you a favor :)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123127684
> @PragmaTwice Last failure caused by didn't link `-pthread`. Can have a test on local with running `sh build.sh _build`, so you are NOT need to wait for the CI feedback.
Sorry, fixed. I didn't encounter this problem when I tested the build locally, probably due to some differences between CI and my local environment.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r868941530
##########
cmake/utils.cmake:
##########
@@ -0,0 +1,26 @@
+include_guard()
Review Comment:
You should add a license head as other files to pass the checker.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r869138940
##########
CMakeLists.txt:
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-cmake_minimum_required(VERSION 3.10)
+cmake_minimum_required(VERSION 3.16)
Review Comment:
Yeah, use 3.16 just because it is the default version in apt repo of ubuntu 20.04. But I highly recommend a newer version of cmake. [A good post](https://www.scivision.dev/cmake-changelog) described its advantages.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1122342228
> @git-hulk it seems that lint-build-test-on-ubuntu fails on test phase, not on build phase, please take a look whether it's about unstable test or stateful test.
OK, I had a glance at the failure test case. The root cause should be that case depends on
time sleep, would improve it later.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r870118939
##########
CMakeLists.txt:
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-cmake_minimum_required(VERSION 3.10)
+cmake_minimum_required(VERSION 3.16)
Review Comment:
@PragmaTwice what features we use for 3.16?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r870151835
##########
cmake/gtest.cmake:
##########
@@ -15,38 +15,18 @@
# specific language governing permissions and limitations
# under the License.
-if (NOT __GTEST_INCLUDED) # guard against multiple includes
- set(__GTEST_INCLUDED TRUE)
+include_guard()
- # gtest will use pthreads if it's available in the system, so we must link with it
- find_package(Threads)
+include(FetchContent)
- # build directory
- set(gtest_PREFIX ${CMAKE_BUILD_DIRECTORY}/external/gtest-prefix)
- # install directory
- set(gtest_INSTALL ${CMAKE_BUILD_DIRECTORY}/external/gtest-install)
+FetchContent_Declare(gtest
+ GIT_REPOSITORY https://github.com/google/googletest/
Review Comment:
Can remove extra `/`
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r870162990
##########
cmake/gtest.cmake:
##########
@@ -15,38 +15,18 @@
# specific language governing permissions and limitations
# under the License.
-if (NOT __GTEST_INCLUDED) # guard against multiple includes
- set(__GTEST_INCLUDED TRUE)
+include_guard()
- # gtest will use pthreads if it's available in the system, so we must link with it
- find_package(Threads)
+include(FetchContent)
- # build directory
- set(gtest_PREFIX ${CMAKE_BUILD_DIRECTORY}/external/gtest-prefix)
- # install directory
- set(gtest_INSTALL ${CMAKE_BUILD_DIRECTORY}/external/gtest-install)
+FetchContent_Declare(gtest
+ GIT_REPOSITORY https://github.com/google/googletest/
Review Comment:
done
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123351330
> I am not familiar with this topic, but it seems a better way.
>
> BTW, i ever wanted to remove Makefile, only keep cmake, since we need to install some libs firstly if we use make. @git-hulk told me we need a high version cmake which may be not convenient for users. I don't know 3.16 is high? If not, maybe in the future, we can only support cmake.
Yes, especially on centos, but I'm agree to remove the Makefile since it will cause much extra
work to maintain, as well as can't compatible with FetchContent.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123460125
I can lower the required cmake version to 3.11 (if it is confirmed that this version contains all the features used), if that helps with compatibility.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123579596
@ShooterIT The required cmake version has been updated to 3.13. (refer to https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123562228)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1122331441
@git-hulk it seems that `lint-build-test-on-ubuntu` fails on test phase, not on build phase, please take a look whether it's about unstable test or stateful test.
@PragmaTwice it seems `build-on-ubuntu-with-cmake` complains with jemalloc not found.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#discussion_r869807686
##########
cmake/utils.cmake:
##########
@@ -0,0 +1,26 @@
+include_guard()
Review Comment:
thx!
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #564: Use FetchContent instead of ExternalProject and git submodules in CMake
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #564:
URL: https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1123565769
@PragmaTwice Cool, Looks good to me. Also tested on my side on MacOSX and Debian.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org