Skip to content
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

Modernize the build setup #107

Merged
merged 7 commits into from
Apr 16, 2024
Merged

Modernize the build setup #107

merged 7 commits into from
Apr 16, 2024

Conversation

lemire
Copy link
Contributor

@lemire lemire commented Apr 12, 2024

This should improve the build setup.

@aviggiano
Copy link
Owner

hey @lemire
Thank you very much for this update.
It seems like the build step is not saving the library at the previous location

12110:M 15 Apr 2024 00:03:31.009 # Module ./build/libredis-roaring.so failed to load: ./build/libredis-roaring.so: cannot open shared object file: No such file or directory
12110:M 15 Apr 2024 00:03:31.010 # Can't load module from ./build/libredis-roaring.so: server aborting

Do you know what might be the cause of this? I can take a look later before merging if you don't have time.

@lemire
Copy link
Contributor Author

lemire commented Apr 15, 2024

@aviggiano

The proposed build setup will compile to static libraries:

option(BUILD_SHARED_LIBS "use shared libraries" OFF)

If one must have dynamic libraries then this should not be OFF, but then you need to make sure that the linker finds the location of the dynamic library.

@lemire
Copy link
Contributor Author

lemire commented Apr 15, 2024

I have pushed an update which should generate a shared library as well.

@lemire
Copy link
Contributor Author

lemire commented Apr 15, 2024

@aviggiano I pushed another update.

@aviggiano
Copy link
Owner

Thanks, I'll try to merge this ASAP
For some reason the test is not finishing, not sure why

@lemire
Copy link
Contributor Author

lemire commented Apr 16, 2024

@aviggiano I am sure we can improve the PR.

Get in touch with me if I can help further. There is obviously a lot of interest for this project and I am available to help.

@xl-xueling
Copy link

@aviggiano @lemire thanks all of you,this project is very very useful !

@aviggiano
Copy link
Owner

aviggiano commented Apr 16, 2024

@lemire I've managed to fix the tests, the library name needed to change.
Can you add this to your PR so that we can merge it? I can't edit it.

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 0c7bb92..c27107e 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -11,6 +11,11 @@ jobs:
       uses: actions/checkout@v2
     - name: Configure
       run: ./configure.sh
+    - name: Check configuration
+      run: |
+        ./dist/redis-server --version
+        ./dist/redis-cli --version
+        ls ./build/libredis-roaring-shared.so
   test:
     name: Test project
     runs-on: ubuntu-latest
diff --git a/Dockerfile b/Dockerfile
index 5224480..ab59880 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -22,11 +22,11 @@ RUN set -ex; \
     cmake ..; \
     make -j; \
     cd ..; \
-    cp build/libredis-roaring.so /usr/local/lib/; \
+    cp build/libredis-roaring-shared.so /usr/local/lib/; \
     \
     apt-get purge -y --auto-remove $BUILD_DEPS
 ADD http://download.redis.io/redis-stable/redis.conf /usr/local/etc/redis/redis.conf
-RUN sed -i '1i loadmodule /usr/local/lib/libredis-roaring.so' /usr/local/etc/redis/redis.conf; \
+RUN sed -i '1i loadmodule /usr/local/lib/libredis-roaring-shared.so' /usr/local/etc/redis/redis.conf; \
     chmod 644 /usr/local/etc/redis/redis.conf; \
     sed -i 's/^bind 127.0.0.1/#bind 127.0.0.1/g' /usr/local/etc/redis/redis.conf; \
     sed -i 's/^protected-mode yes/protected-mode no/g' /usr/local/etc/redis/redis.conf
diff --git a/deps/CRoaring b/deps/CRoaring
index c501a95..fd6f6ff 160000
--- a/deps/CRoaring
+++ b/deps/CRoaring
@@ -1 +1 @@
-Subproject commit c501a95206dec8187aa63a053b1f1a8acb943f7c
+Subproject commit fd6f6ff423bf1f9d240e23e9d2a900885f809c2c
diff --git a/deps/hiredis b/deps/hiredis
index 60e5075..3d8709d 160000
--- a/deps/hiredis
+++ b/deps/hiredis
@@ -1 +1 @@
-Subproject commit 60e5075d4ac77424809f855ba3e398df7aacefe8
+Subproject commit 3d8709d19d7fa67d203a33c969e69f0f1a4eab02
diff --git a/deps/redis b/deps/redis
index d2c8a4b..58f79e2 160000
--- a/deps/redis
+++ b/deps/redis
@@ -1 +1 @@
-Subproject commit d2c8a4b91e8c0e6aefd1f5bc0bf582cddbe046b7
+Subproject commit 58f79e2ff48bb20e48d5eb60ff6a87c9afae5fe9
diff --git a/helper.sh b/helper.sh
index 0aafd5f..014d2c9 100755
--- a/helper.sh
+++ b/helper.sh
@@ -31,7 +31,7 @@ function start_redis()
     shift
   done
 
-  local REDIS_COMMAND="./deps/redis/src/redis-server --loadmodule ./build/libredis-roaring.so"
+  local REDIS_COMMAND="./deps/redis/src/redis-server --loadmodule ./build/libredis-roaring-shared.so"
   local VALGRIND_COMMAND="valgrind --leak-check=yes --show-leak-kinds=definite,indirect --suppressions=./deps/redis/src/valgrind.sup --error-exitcode=1 --log-file=$LOG_FILE"
   local AOF_OPTION="--appendonly $USE_AOF"
   if [ "$USE_VALGRIND" == "no" ]; then

@lemire
Copy link
Contributor Author

lemire commented Apr 16, 2024

@aviggiano I have updated the PR with something that I hope is even better than your patch.

@aviggiano
Copy link
Owner

Merging

@aviggiano aviggiano merged commit ff98989 into aviggiano:master Apr 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants