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/17 12:06:26 UTC
[GitHub] [incubator-kvrocks] PragmaTwice opened a new pull request, #586: Refactor CMakeLists: introduce target kvrocks_objs to avoid redundance
PragmaTwice opened a new pull request, #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586
Solve #563 and #569.
`-O0` and `-g` is removed:
- `-O0` is automatically applied while `-DCMAKE_BUILD_TYPE=Debug` is passed to cmake
- `-g` is automatically applied while `-DCMAKE_BUILD_TYPE=Debug` (or `RelWithDebInfo`) is passed to 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] ShooterIT commented on pull request #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#issuecomment-1129535896
Thanks @PragmaTwice are you familiar with `address sanitizer` and `thread sanitize`. Kvrocks lacks these check on CI currently. We try to do that, but find it doesn't work well. @git-hulk do you remember？
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875505563
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
Review Comment:
@git-hulk should we us `CMAKE_BUILD_TYPE= RelWithDebInfo` instead of `Release` in `build.sh`?
AFAIK, `-g` also is important in production 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] ShooterIT merged pull request #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
ShooterIT merged PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875522022
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
-target_include_directories(kvrocks PRIVATE ${PROJECT_BINARY_DIR})
-find_package(Threads REQUIRED)
-if(THREADS_HAVE_PTHREAD_ARG)
- target_compile_options(kvrocks PUBLIC "-pthread")
-endif()
-if(CMAKE_THREAD_LIBS_INIT)
- target_link_libraries(kvrocks PUBLIC "${CMAKE_THREAD_LIBS_INIT}")
-endif()
-target_link_libraries(kvrocks PRIVATE -fno-omit-frame-pointer)
-target_link_libraries(kvrocks ${EXTERNAL_LIBS})
-if(UNWIND_LIB)
- target_link_libraries(kvrocks PRIVATE -lunwind)
-endif()
-target_sources(kvrocks PRIVATE
- src/cluster.cc
- src/cluster.h
- src/slot_migrate.h
- src/slot_migrate.cc
- src/slot_import.h
- src/slot_import.cc
- src/batch_extractor.h
- src/batch_extractor.cc
- src/redis_db.cc
- src/redis_db.h
- src/redis_connection.cc
- src/redis_connection.h
- src/compact_filter.cc
- src/compact_filter.h
- src/worker.cc
- src/worker.h
- src/main.cc
- src/redis_request.cc
- src/redis_request.h
- src/redis_cmd.cc
- src/redis_cmd.h
- src/util.cc
- src/util.h
- src/geohash.cc
- src/geohash.h
- src/storage.cc
- src/storage.h
- src/status.h
- src/redis_reply.h
- src/redis_reply.cc
- src/replication.cc
- src/replication.h
- src/task_runner.cc
- src/task_runner.h
- src/encoding.h
- src/encoding.cc
- src/redis_slot.h
- src/redis_slot.cc
- src/redis_metadata.h
- src/redis_metadata.cc
- src/redis_string.h
- src/redis_string.cc
- src/redis_hash.h
- src/redis_hash.cc
- src/redis_list.h
- src/redis_list.cc
- src/redis_set.h
- src/redis_set.cc
- src/redis_zset.cc
- src/redis_zset.h
- src/redis_geo.cc
- src/redis_geo.h
- src/redis_bitmap.cc
- src/redis_bitmap.h
- src/redis_bitmap_string.cc
- src/redis_bitmap_string.h
- src/redis_pubsub.cc
- src/redis_pubsub.h
- src/redis_sortedint.cc
- src/redis_sortedint.h
- src/lock_manager.cc
- src/rocksdb_crc32c.h
- src/config.cc
- src/config.h
- src/config_type.h
- src/stats.cc
- src/stats.h
- src/server.cc
- src/server.h
- src/cron.cc
- src/cron.h
- src/event_listener.h
- src/event_listener.cc
- src/log_collector.h
- src/log_collector.cc
- src/table_properties_collector.cc
- src/table_properties_collector.h
- src/compaction_checker.cc
- src/compaction_checker.h
- src/scripting.cc
- src/scripting.h
- src/sha1.cc
- src/sha1.h
- src/rand.cc
- src/rand.h)
+# kvrocks objects target
+file(GLOB KVROCKS_SRCS src/*.cc)
+list(FILTER KVROCKS_SRCS EXCLUDE REGEX src/main.cc)
-# kvrocks2redis sync tool
-add_executable(kvrocks2redis)
-target_compile_features(kvrocks2redis PRIVATE cxx_std_11)
-target_compile_options(kvrocks2redis PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
+add_library(kvrocks_objs OBJECT ${KVROCKS_SRCS})
+
+target_include_directories(kvrocks_objs PUBLIC src ${PROJECT_BINARY_DIR})
+target_compile_features(kvrocks_objs PUBLIC cxx_std_11)
+target_compile_options(kvrocks_objs PUBLIC ${WARNING_FLAGS} -fno-omit-frame-pointer)
+target_link_libraries(kvrocks_objs PUBLIC -fno-omit-frame-pointer)
if(ENBALE_ASAN)
Review Comment:
My bad for this stupid typo.
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875519995
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
Review Comment:
yes, exactly
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#issuecomment-1128830341
I cannot reproduce the test error on my local, do you think it is about the PR or not? Thx for checking. @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 a diff in pull request #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875493570
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
Review Comment:
CMake will add `-O3 -DNDEBUG ...` for `CMAKE_BUILD_TYPE=Release`, `-O2 -DNDEBUG -g` for `RelWithDebInfo` and `-O0 -g ...` for `Debug` in the makefile generator.
You can use `RelWithDebInfo` directly without changing the semantics of `Release` mode if you want `-O2 -g -DNDEBUG`.
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875483494
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
-target_include_directories(kvrocks PRIVATE ${PROJECT_BINARY_DIR})
-find_package(Threads REQUIRED)
-if(THREADS_HAVE_PTHREAD_ARG)
- target_compile_options(kvrocks PUBLIC "-pthread")
-endif()
-if(CMAKE_THREAD_LIBS_INIT)
- target_link_libraries(kvrocks PUBLIC "${CMAKE_THREAD_LIBS_INIT}")
-endif()
-target_link_libraries(kvrocks PRIVATE -fno-omit-frame-pointer)
-target_link_libraries(kvrocks ${EXTERNAL_LIBS})
-if(UNWIND_LIB)
- target_link_libraries(kvrocks PRIVATE -lunwind)
-endif()
-target_sources(kvrocks PRIVATE
- src/cluster.cc
- src/cluster.h
- src/slot_migrate.h
- src/slot_migrate.cc
- src/slot_import.h
- src/slot_import.cc
- src/batch_extractor.h
- src/batch_extractor.cc
- src/redis_db.cc
- src/redis_db.h
- src/redis_connection.cc
- src/redis_connection.h
- src/compact_filter.cc
- src/compact_filter.h
- src/worker.cc
- src/worker.h
- src/main.cc
- src/redis_request.cc
- src/redis_request.h
- src/redis_cmd.cc
- src/redis_cmd.h
- src/util.cc
- src/util.h
- src/geohash.cc
- src/geohash.h
- src/storage.cc
- src/storage.h
- src/status.h
- src/redis_reply.h
- src/redis_reply.cc
- src/replication.cc
- src/replication.h
- src/task_runner.cc
- src/task_runner.h
- src/encoding.h
- src/encoding.cc
- src/redis_slot.h
- src/redis_slot.cc
- src/redis_metadata.h
- src/redis_metadata.cc
- src/redis_string.h
- src/redis_string.cc
- src/redis_hash.h
- src/redis_hash.cc
- src/redis_list.h
- src/redis_list.cc
- src/redis_set.h
- src/redis_set.cc
- src/redis_zset.cc
- src/redis_zset.h
- src/redis_geo.cc
- src/redis_geo.h
- src/redis_bitmap.cc
- src/redis_bitmap.h
- src/redis_bitmap_string.cc
- src/redis_bitmap_string.h
- src/redis_pubsub.cc
- src/redis_pubsub.h
- src/redis_sortedint.cc
- src/redis_sortedint.h
- src/lock_manager.cc
- src/rocksdb_crc32c.h
- src/config.cc
- src/config.h
- src/config_type.h
- src/stats.cc
- src/stats.h
- src/server.cc
- src/server.h
- src/cron.cc
- src/cron.h
- src/event_listener.h
- src/event_listener.cc
- src/log_collector.h
- src/log_collector.cc
- src/table_properties_collector.cc
- src/table_properties_collector.h
- src/compaction_checker.cc
- src/compaction_checker.h
- src/scripting.cc
- src/scripting.h
- src/sha1.cc
- src/sha1.h
- src/rand.cc
- src/rand.h)
+# kvrocks objects target
+file(GLOB KVROCKS_SRCS src/*.cc)
+list(FILTER KVROCKS_SRCS EXCLUDE REGEX src/main.cc)
-# kvrocks2redis sync tool
-add_executable(kvrocks2redis)
-target_compile_features(kvrocks2redis PRIVATE cxx_std_11)
-target_compile_options(kvrocks2redis PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
+add_library(kvrocks_objs OBJECT ${KVROCKS_SRCS})
+
+target_include_directories(kvrocks_objs PUBLIC src ${PROJECT_BINARY_DIR})
+target_compile_features(kvrocks_objs PUBLIC cxx_std_11)
+target_compile_options(kvrocks_objs PUBLIC ${WARNING_FLAGS} -fno-omit-frame-pointer)
+target_link_libraries(kvrocks_objs PUBLIC -fno-omit-frame-pointer)
if(ENBALE_ASAN)
Review Comment:
maybe😂
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875493570
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
Review Comment:
CMake will add `-O3 -DNDEBUG ...` for `CMAKE_BUILD_TYPE=Release`, `-O2 -DNDEBUG -g` for `RelWithDebInfo` and `-O0 -g ...` for `Debug` in the makefile generator.
You can use `RelWithDebInfo` directly without changing the semantics of Release mode if you want `-O2 -g -DNDEBUG`.
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875514829
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
Review Comment:
I think we should keep the debug info that will help us a lot when crashing, even it will sacrifice a bit optimize feature. How do you think? @PragmaTwice @ShooterIT
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875527176
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
Review Comment:
Cool, @PragmaTwice can help to do this change, then can merge this improvement.
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#issuecomment-1129549256
Maybe a higher version of GCC and `-fsanitize=leak` is needed for detecting memory leaks.
BTW I highly recommand Clang while sanitizer or fuzzer instrumentation is required.
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#issuecomment-1129582227
It works on my side (archlinux, kernel 5.10, gcc 11.2) for a simple leak in `HSET`:
I think if there is some leak allocated in rocksdb or other deps, it may cannot be detected in current build configurations.
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#issuecomment-1129540653
> Thanks @PragmaTwice are you familiar with `address sanitizer` and `thread sanitize`. Kvrocks lacks these check on CI currently. We try to do that, but find it doesn't work well. @git-hulk do you remember？
Yes, I found the address sanitizer can't detect the explicitly memory leak that mallocs memory without releasing them.
@ShooterIT Can you help to summary and merge this PR? We can file another issue to discuss about the sanitizer.
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#issuecomment-1129562831
> > Thanks @PragmaTwice are you familiar with `address sanitizer` and `thread sanitize`. Kvrocks lacks these check on CI currently. We try to do that, but find it doesn't work well. @git-hulk do you remember？
>
> Maybe a higher version of GCC (>= 5) and `-fsanitize=leak` is needed for detecting memory leaks. BTW I highly recommand Clang while sanitizer or fuzzer instrumentation is required, LeakSanitizer is a part of AddressSanitizer in Clang.
Yes, we are running the sanitizer in Clang and can work when memory leak directly in starting the server, but after that. For example, leaks in `HSET` command, the sanitizer can't find out this issue even we ran this command.
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875493570
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
Review Comment:
CMake will add `-O3 -DNDEBUG ...` for `CMAKE_BUILD_TYPE=Release`, `-O2 -DNDEBUG -g` for `RelWithDebInfo` and `-O0 -g ...` for `Debug` in the makefile generator.
I wonder why use `-O2 -g` for `Release`, since it is rare for C++ projects. You can use `RelWithDebInfo` directly without changing the semantics of Release mode.
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875485399
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
Review Comment:
we want to use `-O2 -g -DNDEBUG` in release version.
what are the default values
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#issuecomment-1128836482
@PragmaTwice It's NOT related with this PR, I will rerun the CI. I didn't investigate why this test case frequently failed in CI environment. @ChrisZMF Can you have a look?
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875477034
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
-target_include_directories(kvrocks PRIVATE ${PROJECT_BINARY_DIR})
-find_package(Threads REQUIRED)
-if(THREADS_HAVE_PTHREAD_ARG)
- target_compile_options(kvrocks PUBLIC "-pthread")
-endif()
-if(CMAKE_THREAD_LIBS_INIT)
- target_link_libraries(kvrocks PUBLIC "${CMAKE_THREAD_LIBS_INIT}")
-endif()
-target_link_libraries(kvrocks PRIVATE -fno-omit-frame-pointer)
-target_link_libraries(kvrocks ${EXTERNAL_LIBS})
-if(UNWIND_LIB)
- target_link_libraries(kvrocks PRIVATE -lunwind)
-endif()
-target_sources(kvrocks PRIVATE
- src/cluster.cc
- src/cluster.h
- src/slot_migrate.h
- src/slot_migrate.cc
- src/slot_import.h
- src/slot_import.cc
- src/batch_extractor.h
- src/batch_extractor.cc
- src/redis_db.cc
- src/redis_db.h
- src/redis_connection.cc
- src/redis_connection.h
- src/compact_filter.cc
- src/compact_filter.h
- src/worker.cc
- src/worker.h
- src/main.cc
- src/redis_request.cc
- src/redis_request.h
- src/redis_cmd.cc
- src/redis_cmd.h
- src/util.cc
- src/util.h
- src/geohash.cc
- src/geohash.h
- src/storage.cc
- src/storage.h
- src/status.h
- src/redis_reply.h
- src/redis_reply.cc
- src/replication.cc
- src/replication.h
- src/task_runner.cc
- src/task_runner.h
- src/encoding.h
- src/encoding.cc
- src/redis_slot.h
- src/redis_slot.cc
- src/redis_metadata.h
- src/redis_metadata.cc
- src/redis_string.h
- src/redis_string.cc
- src/redis_hash.h
- src/redis_hash.cc
- src/redis_list.h
- src/redis_list.cc
- src/redis_set.h
- src/redis_set.cc
- src/redis_zset.cc
- src/redis_zset.h
- src/redis_geo.cc
- src/redis_geo.h
- src/redis_bitmap.cc
- src/redis_bitmap.h
- src/redis_bitmap_string.cc
- src/redis_bitmap_string.h
- src/redis_pubsub.cc
- src/redis_pubsub.h
- src/redis_sortedint.cc
- src/redis_sortedint.h
- src/lock_manager.cc
- src/rocksdb_crc32c.h
- src/config.cc
- src/config.h
- src/config_type.h
- src/stats.cc
- src/stats.h
- src/server.cc
- src/server.h
- src/cron.cc
- src/cron.h
- src/event_listener.h
- src/event_listener.cc
- src/log_collector.h
- src/log_collector.cc
- src/table_properties_collector.cc
- src/table_properties_collector.h
- src/compaction_checker.cc
- src/compaction_checker.h
- src/scripting.cc
- src/scripting.h
- src/sha1.cc
- src/sha1.h
- src/rand.cc
- src/rand.h)
+# kvrocks objects target
+file(GLOB KVROCKS_SRCS src/*.cc)
+list(FILTER KVROCKS_SRCS EXCLUDE REGEX src/main.cc)
-# kvrocks2redis sync tool
-add_executable(kvrocks2redis)
-target_compile_features(kvrocks2redis PRIVATE cxx_std_11)
-target_compile_options(kvrocks2redis PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
+add_library(kvrocks_objs OBJECT ${KVROCKS_SRCS})
+
+target_include_directories(kvrocks_objs PUBLIC src ${PROJECT_BINARY_DIR})
+target_compile_features(kvrocks_objs PUBLIC cxx_std_11)
+target_compile_options(kvrocks_objs PUBLIC ${WARNING_FLAGS} -fno-omit-frame-pointer)
+target_link_libraries(kvrocks_objs PUBLIC -fno-omit-frame-pointer)
if(ENBALE_ASAN)
Review Comment:
Oh, there is a typo in the original CMakeLists, which is found while I look into the build behaviors while ASAN is enabled.
Is it a reason for this issue? @git-hulk @ShooterIT
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875519864
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
Review Comment:
I think it is ok to use `RelWithDebInfo` mode in production environments.
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#issuecomment-1129656985
Changed build type to `RelWithDebInfo`, and also fixed the typo. @git-hulk @ShooterIT
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875477034
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
-target_include_directories(kvrocks PRIVATE ${PROJECT_BINARY_DIR})
-find_package(Threads REQUIRED)
-if(THREADS_HAVE_PTHREAD_ARG)
- target_compile_options(kvrocks PUBLIC "-pthread")
-endif()
-if(CMAKE_THREAD_LIBS_INIT)
- target_link_libraries(kvrocks PUBLIC "${CMAKE_THREAD_LIBS_INIT}")
-endif()
-target_link_libraries(kvrocks PRIVATE -fno-omit-frame-pointer)
-target_link_libraries(kvrocks ${EXTERNAL_LIBS})
-if(UNWIND_LIB)
- target_link_libraries(kvrocks PRIVATE -lunwind)
-endif()
-target_sources(kvrocks PRIVATE
- src/cluster.cc
- src/cluster.h
- src/slot_migrate.h
- src/slot_migrate.cc
- src/slot_import.h
- src/slot_import.cc
- src/batch_extractor.h
- src/batch_extractor.cc
- src/redis_db.cc
- src/redis_db.h
- src/redis_connection.cc
- src/redis_connection.h
- src/compact_filter.cc
- src/compact_filter.h
- src/worker.cc
- src/worker.h
- src/main.cc
- src/redis_request.cc
- src/redis_request.h
- src/redis_cmd.cc
- src/redis_cmd.h
- src/util.cc
- src/util.h
- src/geohash.cc
- src/geohash.h
- src/storage.cc
- src/storage.h
- src/status.h
- src/redis_reply.h
- src/redis_reply.cc
- src/replication.cc
- src/replication.h
- src/task_runner.cc
- src/task_runner.h
- src/encoding.h
- src/encoding.cc
- src/redis_slot.h
- src/redis_slot.cc
- src/redis_metadata.h
- src/redis_metadata.cc
- src/redis_string.h
- src/redis_string.cc
- src/redis_hash.h
- src/redis_hash.cc
- src/redis_list.h
- src/redis_list.cc
- src/redis_set.h
- src/redis_set.cc
- src/redis_zset.cc
- src/redis_zset.h
- src/redis_geo.cc
- src/redis_geo.h
- src/redis_bitmap.cc
- src/redis_bitmap.h
- src/redis_bitmap_string.cc
- src/redis_bitmap_string.h
- src/redis_pubsub.cc
- src/redis_pubsub.h
- src/redis_sortedint.cc
- src/redis_sortedint.h
- src/lock_manager.cc
- src/rocksdb_crc32c.h
- src/config.cc
- src/config.h
- src/config_type.h
- src/stats.cc
- src/stats.h
- src/server.cc
- src/server.h
- src/cron.cc
- src/cron.h
- src/event_listener.h
- src/event_listener.cc
- src/log_collector.h
- src/log_collector.cc
- src/table_properties_collector.cc
- src/table_properties_collector.h
- src/compaction_checker.cc
- src/compaction_checker.h
- src/scripting.cc
- src/scripting.h
- src/sha1.cc
- src/sha1.h
- src/rand.cc
- src/rand.h)
+# kvrocks objects target
+file(GLOB KVROCKS_SRCS src/*.cc)
+list(FILTER KVROCKS_SRCS EXCLUDE REGEX src/main.cc)
-# kvrocks2redis sync tool
-add_executable(kvrocks2redis)
-target_compile_features(kvrocks2redis PRIVATE cxx_std_11)
-target_compile_options(kvrocks2redis PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
+add_library(kvrocks_objs OBJECT ${KVROCKS_SRCS})
+
+target_include_directories(kvrocks_objs PUBLIC src ${PROJECT_BINARY_DIR})
+target_compile_features(kvrocks_objs PUBLIC cxx_std_11)
+target_compile_options(kvrocks_objs PUBLIC ${WARNING_FLAGS} -fno-omit-frame-pointer)
+target_link_libraries(kvrocks_objs PUBLIC -fno-omit-frame-pointer)
if(ENBALE_ASAN)
Review Comment:
Oh, there is a typo in the original CMakeLists, `ENBALE -> ENABLE`, which is found while I look into the build behaviors while ASAN is enabled.
Is it a reason for this issue? @git-hulk @ShooterIT
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#issuecomment-1129551954
Try it here: https://godbolt.org/z/8r48Mvz3W : )
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#issuecomment-1129569482
That's weird. I'll investigate 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] PragmaTwice commented on a diff in pull request #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875493570
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
Review Comment:
CMake will add `-O3 -DNDEBUG ...` for `CMAKE_BUILD_TYPE=Release`, `-O2 -DNDEBUG -g ...` for `RelWithDebInfo` and `-O0 -g ...` for `Debug` in the makefile generator.
You can use `RelWithDebInfo` directly without changing the semantics of `Release` mode if you want `-O2 -g -DNDEBUG`.
--
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 #586: Refactor CMakeLists: introduce target `kvrocks_objs` to avoid redundance
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #586:
URL: https://github.com/apache/incubator-kvrocks/pull/586#discussion_r875519864
##########
CMakeLists.txt:
##########
@@ -50,351 +46,60 @@ include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
include(cmake/lua.cmake)
+find_package(Threads REQUIRED)
+
list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
-
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
-
list(APPEND EXTERNAL_LIBS PRIVATE lua)
-
-# End dependencies
+list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)
# Add git sha to version.h
find_package(Git REQUIRED)
execute_process(COMMAND git rev-parse --short HEAD OUTPUT_VARIABLE GIT_SHA)
string(STRIP "${GIT_SHA}" GIT_SHA)
configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
-# Main target
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++")
endif()
-find_library(UNWIND_LIB unwind)
+find_library(HAS_UNWIND_LIB unwind)
-add_executable(kvrocks)
-target_compile_features(kvrocks PRIVATE cxx_std_11)
-target_compile_options(kvrocks PRIVATE -Wall -Wpedantic -g -Wsign-compare -Wreturn-type -fno-omit-frame-pointer -O0)
-option(ENABLE_ASAN "enable ASAN santinizer" OFF)
-if(ENBALE_ASAN)
- target_compile_options(kvrocks PRIVATE -fsanitize=address)
- target_link_libraries(kvrocks PRIVATE -fsanitize=address)
-endif()
+set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
Review Comment:
I think it is ok to use `RelWithDebInfo` (`-g -O2 -DNDEBUG`) mode in production environments.
--
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