dragonflydb: 0.1.0 -> 1.6.1; fix build

[nrhtr]

Description of changes

Bump version and (mostly incidentally) fix whatever was stopping it from building. I tried to make the patching / vendored dependencies a little nicer, but please let me know if it can be improved further.

127.0.0.1:6379> set mykey "HELLO FRIEND"
OK
127.0.0.1:6379> get mykey
"HELLO FRIEND"
127.0.0.1:6379> 

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[dblsaiko]

Result of nixpkgs-review pr 243300 run on x86_64-linux 1

1 package built:

• dragonflydb

[dblsaiko]

Does this really need these specific versions for mimalloc and reflex? If not, those would already be in nixpkgs

[nrhtr]

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 #include directives if you prefer.

I missed that re-flex was already in nixpkgs and it's the same version, so I'll fix that tomorrow.

[dblsaiko]

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.

[nrhtr]

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.

[dblsaiko]

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

diff --git a/pkgs/servers/nosql/dragonflydb/cmake-fixes.patch b/pkgs/servers/nosql/dragonflydb/cmake-fixes.patch
index 87740f105fe4..4b5762f0024c 100644
--- a/pkgs/servers/nosql/dragonflydb/cmake-fixes.patch
+++ b/pkgs/servers/nosql/dragonflydb/cmake-fixes.patch
@@ -20,16 +20,32 @@ index 78c3058..0077eb8 100644
  )
  
  FetchContent_GetProperties(benchmark)
-@@ -176,7 +176,7 @@ endif ()
+@@ -173,35 +173,13 @@
+     add_subdirectory(${benchmark_SOURCE_DIR} ${benchmark_BINARY_DIR})
+ endif ()
  
- FetchContent_Declare(
-   abseil_cpp
+-
+-FetchContent_Declare(
+-  abseil_cpp
 -  URL https://github.com/abseil/abseil-cpp/archive/20230125.2.tar.gz
-+  DOWNLOAD_COMMAND true
- )
+-)
+-
+-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)
  
- FetchContent_GetProperties(abseil_cpp)
-@@ -197,11 +197,7 @@ set(FETCHCONTENT_UPDATES_DISCONNECTED_GLOG ON CACHE BOOL "")
+ set(FETCHCONTENT_UPDATES_DISCONNECTED_GLOG ON CACHE BOOL "")
  
  FetchContent_Declare(
    glog
@@ -42,7 +58,7 @@ index 78c3058..0077eb8 100644
  )
  
  FetchContent_GetProperties(glog)
-@@ -253,8 +249,7 @@ endif()
+@@ -253,8 +231,7 @@
  
  add_third_party(
    gperf
@@ -52,7 +68,7 @@ index 78c3058..0077eb8 100644
    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 +274,7 @@ set(MIMALLOC_INCLUDE_DIR ${THIRD_PARTY_LIB_DIR}/mimalloc/include)
+@@ -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
@@ -63,7 +79,7 @@ index 78c3058..0077eb8 100644
     PATCH_COMMAND "${MIMALLOC_PATCH_COMMAND}"
     # -DCMAKE_BUILD_TYPE=Release
     # Add -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_FLAGS=-O0 to debug
-@@ -297,7 +290,7 @@ set (MIMALLOC_PATCH_COMMAND patch -p1 -d ${THIRD_PARTY_DIR}/mimalloc/ -i ${CMAKE
+@@ -297,7 +272,7 @@
  )
  
  add_third_party(jemalloc
@@ -72,16 +88,16 @@ index 78c3058..0077eb8 100644
    PATCH_COMMAND ./autogen.sh
    CONFIGURE_COMMAND <SOURCE_DIR>/configure --prefix=${THIRD_PARTY_LIB_DIR}/jemalloc --with-jemalloc-prefix=je_ --disable-libdl
  )
-@@ -305,7 +298,7 @@ add_third_party(jemalloc
+@@ -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 +309,14 @@ add_third_party(
+@@ -316,17 +291,14 @@
  
  add_third_party(
    uring
@@ -101,7 +117,7 @@ index 78c3058..0077eb8 100644
    CMAKE_PASS_FLAGS "-DRAPIDJSON_BUILD_TESTS=OFF -DRAPIDJSON_BUILD_EXAMPLES=OFF \
                      -DRAPIDJSON_BUILD_DOC=OFF"
    LIB "none"
-@@ -334,7 +324,7 @@ add_third_party(
+@@ -334,7 +306,7 @@
  
  add_third_party(
    cares
@@ -123,7 +139,7 @@ index 50ce1e5..fbe5bdf 100644
    PATCH_COMMAND patch -p1 -i "${CMAKE_SOURCE_DIR}/patches/lua-v5.4.4.patch"
    CONFIGURE_COMMAND echo
    BUILD_IN_SOURCE 1
-@@ -47,13 +47,14 @@ endfunction()
+@@ -47,28 +47,23 @@
  
  add_third_party(
    dconv
@@ -133,14 +149,14 @@ index 50ce1e5..fbe5bdf 100644
    LIB libdouble-conversion.a
  )
  
- add_third_party(
-   reflex
+-add_third_party(
+-  reflex
 -  URL https://github.com/Genivia/RE-flex/archive/refs/tags/v3.3.2.tar.gz
-+  DOWNLOAD_COMMAND true
-   CONFIGURE_COMMAND <SOURCE_DIR>/configure --disable-avx --prefix=${THIRD_PARTY_LIB_DIR}/reflex
- )
- 
-@@ -61,14 +62,14 @@ set(REFLEX "${THIRD_PARTY_LIB_DIR}/reflex/bin/reflex")
+-  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
@@ -157,3 +173,25 @@ index 50ce1e5..fbe5bdf 100644
  
    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)
diff --git a/pkgs/servers/nosql/dragonflydb/default.nix b/pkgs/servers/nosql/dragonflydb/default.nix
index 72d269d3d4ee..67392062355a 100644
--- a/pkgs/servers/nosql/dragonflydb/default.nix
+++ b/pkgs/servers/nosql/dragonflydb/default.nix
@@ -70,12 +70,10 @@ stdenv.mkDerivation {
     mkdir -p ./build/{third_party,_deps} ./build/third_party/cares
 
     ln -s ${double-conversion.src} ./build/third_party/dconv
-    ln -s ${re-flex.src} ./build/third_party/reflex
     ln -s ${jsoncons} ./build/third_party/jsoncons
     ln -s ${rapidjson.src} ./build/third_party/rapidjson
     ln -s ${gtest.src} ./build/_deps/gtest-src
     ln -s ${gbenchmark.src} ./build/_deps/benchmark-src
-    ln -s ${abseil-cpp.src} ./build/_deps/abseil_cpp-src
 
     tar xvf ${c-ares.src} --strip-components=1 -C ./build/third_party/cares
 
@@ -114,9 +112,11 @@ stdenv.mkDerivation {
     cmake
     ninja
     bison
+    re-flex
   ];
 
   buildInputs = [
+    abseil-cpp
     boost
     libunwind
     libtool
@@ -128,6 +128,9 @@ stdenv.mkDerivation {
   cmakeFlags = [
     "-DCMAKE_AR=${gcc-unwrapped}/bin/gcc-ar"
     "-DCMAKE_RANLIB=${gcc-unwrapped}/bin/gcc-ranlib"
+    # re-flex doesn't include a cmake-config/pkgconfig file
+    "-DREFLEX_LIBRARY=${re-flex}/lib/libreflex.a"
+    "-DREFLEX_INCLUDE=${re-flex}/include"
   ];
 
   ninjaFlags = [ "dragonfly" ];

[nrhtr]

Wow, that's great. Thanks! I feel like you have probably done more on this PR than I have, now :)

[batonac]

Are we at a stalemate here? I was sad to find that the nixpkgs version of dragonflydb is woefully out of date and not building cleanly ATM.

[FliegendeWurst]

@batonac you can always try to build on top of this PR. If you manage to get it working, you can open another PR here.