-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
dragonflydb: 0.1.0 -> 1.6.1; fix build #243300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,197 @@ | ||
| diff --git a/helio/cmake/third_party.cmake b/helio/cmake/third_party.cmake | ||
| index 78c3058..0077eb8 100644 | ||
| --- a/helio/cmake/third_party.cmake | ||
| +++ b/helio/cmake/third_party.cmake | ||
| @@ -150,7 +150,7 @@ endfunction() | ||
|
|
||
| FetchContent_Declare( | ||
| gtest | ||
| - URL https://github.com/google/googletest/archive/release-1.11.0.zip | ||
| + DOWNLOAD_COMMAND true | ||
| ) | ||
|
|
||
| FetchContent_GetProperties(gtest) | ||
| @@ -161,7 +161,7 @@ endif () | ||
|
|
||
| FetchContent_Declare( | ||
| benchmark | ||
| - URL https://github.com/google/benchmark/archive/v1.7.1.tar.gz | ||
| + DOWNLOAD_COMMAND true | ||
| ) | ||
|
|
||
| FetchContent_GetProperties(benchmark) | ||
| @@ -173,35 +173,13 @@ | ||
| add_subdirectory(${benchmark_SOURCE_DIR} ${benchmark_BINARY_DIR}) | ||
| endif () | ||
|
|
||
| - | ||
| -FetchContent_Declare( | ||
| - abseil_cpp | ||
| - URL https://github.com/abseil/abseil-cpp/archive/20230125.2.tar.gz | ||
| -) | ||
| - | ||
| -FetchContent_GetProperties(abseil_cpp) | ||
| -if(NOT abseil_cpp_POPULATED) | ||
| - FetchContent_Populate(abseil_cpp) | ||
| - set(BUILD_TESTING OFF CACHE INTERNAL "") | ||
| - set(ABSL_PROPAGATE_CXX_STD ON CACHE INTERNAL "") | ||
| - | ||
| - # If we want to override a variable in a subproject, we can temporary change the var | ||
| - # and then restore it if we use it ourselves. | ||
| - set(CMAKE_CXX_FLAGS_RELEASE_OLD ${CMAKE_CXX_FLAGS_RELEASE}) | ||
| - set(CMAKE_CXX_FLAGS_RELEASE "-O3 -DNDEBUG") | ||
| - add_subdirectory(${abseil_cpp_SOURCE_DIR} ${abseil_cpp_BINARY_DIR}) | ||
| - set(CMAKE_CXX_FLAGS_RELEASE ${CMAKE_CXX_FLAGS_RELEASE_OLD}) | ||
| -endif() | ||
| +find_package(absl REQUIRED) | ||
|
|
||
| set(FETCHCONTENT_UPDATES_DISCONNECTED_GLOG ON CACHE BOOL "") | ||
|
|
||
| FetchContent_Declare( | ||
| glog | ||
| - GIT_REPOSITORY https://github.com/romange/glog | ||
| - GIT_TAG Absl | ||
| - | ||
| - GIT_PROGRESS TRUE | ||
| - GIT_SHALLOW TRUE | ||
| + DOWNLOAD_COMMAND true | ||
| ) | ||
|
|
||
| FetchContent_GetProperties(glog) | ||
| @@ -253,8 +231,7 @@ | ||
|
|
||
| add_third_party( | ||
| gperf | ||
| - URL https://github.com/gperftools/gperftools/archive/gperftools-2.10.tar.gz | ||
| - | ||
| + DOWNLOAD_COMMAND true | ||
| GIT_SHALLOW TRUE | ||
| PATCH_COMMAND autoreconf -i # update runs every time for some reason | ||
| # CMAKE_PASS_FLAGS "-DGPERFTOOLS_BUILD_HEAP_PROFILER=OFF -DGPERFTOOLS_BUILD_HEAP_CHECKER=OFF \ | ||
| @@ -279,9 +256,7 @@ | ||
| set (MIMALLOC_PATCH_COMMAND patch -p1 -d ${THIRD_PARTY_DIR}/mimalloc/ -i ${CMAKE_CURRENT_LIST_DIR}/../patches/mimalloc-v2.0.9.patch) | ||
|
|
||
| add_third_party(mimalloc | ||
| - #GIT_REPOSITORY https://github.com/microsoft/mimalloc.git | ||
| - #GIT_TAG v2.0.9 | ||
| - URL https://github.com/microsoft/mimalloc/archive/refs/tags/v2.0.9.tar.gz | ||
| + DOWNLOAD_COMMAND true | ||
| PATCH_COMMAND "${MIMALLOC_PATCH_COMMAND}" | ||
| # -DCMAKE_BUILD_TYPE=Release | ||
| # Add -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_FLAGS=-O0 to debug | ||
| @@ -297,7 +272,7 @@ | ||
| ) | ||
|
|
||
| add_third_party(jemalloc | ||
| - URL https://github.com/jemalloc/jemalloc/releases/download/5.2.1/jemalloc-5.2.1.tar.bz2 | ||
| + URL @jemallocUrl@ | ||
| PATCH_COMMAND ./autogen.sh | ||
| CONFIGURE_COMMAND <SOURCE_DIR>/configure --prefix=${THIRD_PARTY_LIB_DIR}/jemalloc --with-jemalloc-prefix=je_ --disable-libdl | ||
| ) | ||
| @@ -305,7 +280,7 @@ | ||
|
|
||
| add_third_party( | ||
| xxhash | ||
| - URL https://github.com/Cyan4973/xxHash/archive/v0.8.1.tar.gz | ||
| + DOWNLOAD_COMMAND true | ||
|
|
||
| # A bug in xxhash 0.8.1 that searches for a file that doesn't exist | ||
| PATCH_COMMAND touch <SOURCE_DIR>/xxhsum.1 | ||
| @@ -316,17 +291,14 @@ | ||
|
|
||
| add_third_party( | ||
| uring | ||
| - URL https://github.com/axboe/liburing/archive/refs/tags/liburing-2.2.tar.gz | ||
| - #GIT_REPOSITORY https://github.com/axboe/liburing.git | ||
| - # GIT_TAG liburing-2.3 | ||
| + DOWNLOAD_COMMAND true | ||
| CONFIGURE_COMMAND <SOURCE_DIR>/configure --prefix=${THIRD_PARTY_LIB_DIR}/uring | ||
| BUILD_IN_SOURCE 1 | ||
| ) | ||
|
|
||
| add_third_party( | ||
| rapidjson | ||
| - GIT_REPOSITORY https://github.com/Tencent/rapidjson.git | ||
| - GIT_TAG 1a803826f1197b5e30703afe4b9c0e7dd48074f5 | ||
| + DOWNLOAD_COMMAND true | ||
| CMAKE_PASS_FLAGS "-DRAPIDJSON_BUILD_TESTS=OFF -DRAPIDJSON_BUILD_EXAMPLES=OFF \ | ||
| -DRAPIDJSON_BUILD_DOC=OFF" | ||
| LIB "none" | ||
| @@ -334,7 +306,7 @@ | ||
|
|
||
| add_third_party( | ||
| cares | ||
| - URL https://c-ares.org/download/c-ares-1.19.0.tar.gz | ||
| + DOWNLOAD_COMMAND true | ||
| CMAKE_PASS_FLAGS "-DCARES_SHARED:BOOL=OFF -DCARES_STATIC:BOOL=ON -DCARES_STATIC_PIC:BOOL=ON \ | ||
| -DCMAKE_INSTALL_LIBDIR=lib" | ||
| ) | ||
| diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt | ||
| index 50ce1e5..fbe5bdf 100644 | ||
| --- a/src/CMakeLists.txt | ||
| +++ b/src/CMakeLists.txt | ||
| @@ -7,7 +7,7 @@ endif() | ||
|
|
||
| add_third_party( | ||
| lua | ||
| - URL https://github.com/lua/lua/archive/refs/tags/v5.4.4.tar.gz | ||
| + URL @luaUrl@ | ||
| PATCH_COMMAND patch -p1 -i "${CMAKE_SOURCE_DIR}/patches/lua-v5.4.4.patch" | ||
| CONFIGURE_COMMAND echo | ||
| BUILD_IN_SOURCE 1 | ||
| @@ -47,28 +47,23 @@ | ||
|
|
||
| add_third_party( | ||
| dconv | ||
| - URL https://github.com/google/double-conversion/archive/refs/tags/v3.2.0.tar.gz | ||
| + DOWNLOAD_COMMAND true | ||
| + CMAKE_PASS_FLAGS "-DCMAKE_INSTALL_LIBDIR=${THIRD_PARTY_LIB_DIR}/dconv/lib" | ||
| LIB libdouble-conversion.a | ||
| ) | ||
|
|
||
| -add_third_party( | ||
| - reflex | ||
| - URL https://github.com/Genivia/RE-flex/archive/refs/tags/v3.3.2.tar.gz | ||
| - CONFIGURE_COMMAND <SOURCE_DIR>/configure --disable-avx --prefix=${THIRD_PARTY_LIB_DIR}/reflex | ||
| -) | ||
| - | ||
| -set(REFLEX "${THIRD_PARTY_LIB_DIR}/reflex/bin/reflex") | ||
| +find_program(REFLEX reflex REQUIRED) | ||
|
|
||
| add_third_party( | ||
| jsoncons | ||
| - URL https://github.com/danielaparker/jsoncons/archive/refs/tags/v0.168.7.tar.gz | ||
| + DOWNLOAD_COMMAND true | ||
| CMAKE_PASS_FLAGS "-DJSONCONS_BUILD_TESTS=OFF" | ||
| LIB "none" | ||
| ) | ||
|
|
||
| add_third_party( | ||
| lz4 | ||
| - URL https://github.com/lz4/lz4/archive/refs/tags/v1.9.4.tar.gz | ||
| + DOWNLOAD_COMMAND true | ||
|
|
||
| BUILD_IN_SOURCE 1 | ||
| CONFIGURE_COMMAND echo skip | ||
| @@ -119,7 +114,7 @@ | ||
|
|
||
| COMMAND ${REFLEX} -o ${gen_dir}/${name}.cc --unicode --header-file=${gen_dir}/${name}.h | ||
| --bison-complete --bison-locations ${_in} | ||
| - DEPENDS ${_in} reflex_project | ||
| + DEPENDS ${_in} | ||
| WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} | ||
| COMMENT "Generating lexer from ${name}.lex" VERBATIM) | ||
|
|
||
| diff -Naur a/src/core/search/CMakeLists.txt b/src/core/search/CMakeLists.txt | ||
| --- a/src/core/search/CMakeLists.txt | ||
| +++ b/src/core/search/CMakeLists.txt | ||
| @@ -5,7 +5,8 @@ | ||
|
|
||
| add_library(query_parser ast_expr.cc query_driver.cc search.cc indices.cc vector.cc compressed_sorted_set.cc | ||
| ${gen_dir}/parser.cc ${gen_dir}/lexer.cc) | ||
| -target_link_libraries(query_parser base absl::strings TRDP::reflex) | ||
| +target_link_libraries(query_parser base absl::strings ${REFLEX_LIBRARY}) | ||
| +target_include_directories(query_parser PRIVATE ${REFLEX_INCLUDE}) | ||
| cxx_test(compressed_sorted_set_test query_parser LABELS DFLY) | ||
| cxx_test(search_parser_test query_parser LABELS DFLY) | ||
| cxx_test(search_test query_parser LABELS DFLY) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this really need these specific versions for mimalloc and reflex? If not, those would already be in nixpkgs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try to use the (slightly newer) nixpkgs version of mimalloc. There is a small patch in dragonflydb but it applies cleanly. It then failed because some header files were moved around and so I decided to stick with this specific version. I can revisit that and patch the
#includedirectives if you prefer.I missed that re-flex was already in nixpkgs and it's the same version, so I'll fix that tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm honestly not sure what is usually done in this case. Personally, if it's relatively straightforward to get it to link against the already packaged libraries, that's what I'd do, but if it requires hacky patches or anything that looks like it's prone to breaking with updates it's probably not worth the effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a go, but wasn't able to get it working. I think it will also be likely to break with updates because it references an internal struct in mimalloc.
However, I fixed both reflex and the abseil (neither are patched) dependencies to at least use the sources in nixpkgs. Probably it would be even better to patch the build to autodiscover these libraries rather than "fake download" and build the sources, but I don't know cmake well enough to do that.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if it uses internal stuff then doing that is not a good idea.
This patch would get it to work with the re-flex and abseil builds from nixpkgs (the relevant cmake calls to import installed libraries/programs are find_library and find_program):
Details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that's great. Thanks! I feel like you have probably done more on this PR than I have, now :)