Refactor build system - remove readies from module build [MOD-5722] by DvirDukhan · Pull Request #5737 · RediSearch/RediSearch

DvirDukhan · 2025-03-09T22:46:56Z

Describe the changes in the pull request

(PR started by @DvirDukhan, continue by @alonre24)
The goal is to purge https://github.com/RedisLabsModules/readies from our build system.
In this PR we introduce a new script .build.sh which is an alternative entry point for building the module using cmake without going through the makefile.
This includes refactoring of our CMakeLists.txt so it will account for the rules and flags that are passed via readies.

Now we can to the following operations without going through readies:

build the module and the unit tests by calling ./build.sh [TESTS]
run flow tests by calling ./build.sh RUN_PYTEST
build and run the sanitizer with flow tests by calling ./build.sh [RUN_PYTEST]

This is still an intermediate step on the way to purge readies, and building the module in the "old way" using makefile rules is still possible (and will probably be available in the future as well, but internally will call to ./build.sh - TBD).

As part of that, I also:

Removed redundant files related to build, including the docker template (we are not using it anymore).
Removed the option to build redisearch as a static library and redisearch lite since these are no longer in use (light as of redis 8)
Bump googletest version

** Benchmarks flow does not show any regression **

Left to do (in upcoming PRs):

Build and run coverage without readies - currently this still works via make coverage call only.
Refactor unit tests flow that is still using readies internally (will also require some refactoring regardless) - currently requires calling make unit-tests
Add a linter that brings an informative message upon setting up the module about missing dependencies.
Refactor pack and upload-artifacts flows

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

CMakeLists.txt

.github/workflows/task-test.yml

GuyAv46 · 2025-03-11T16:54:42Z

My concerns with removing readies from our build path (I'm sure you thought about it too 😃)

We have to make sure we still compile the project correctly, with all the right compilation flags, including performance flags that may or may not come from readies
Maybe we should even go over https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html and see if we can improve the performance in some way (probably as a tech dept)
We should ensure we don't see any performance regression from our benchmark suite -> we should be able to run the benchmarks again before merging the PR...
We should make it easy to understand what flags we use when compiling (like a single section in the main CMakeLists.txt that sets them and explains them if needed)

run unit tests with old "make unit-tests" way (until this is fixed via ./build.sh)

…or debug)

codecov · 2025-03-23T13:48:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.25%. Comparing base (0418b35) to head (c8a2c20).
Report is 16 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5737      +/-   ##
==========================================
+ Coverage   87.05%   87.25%   +0.20%     
==========================================
  Files         211      197      -14     
  Lines       38569    36019    -2550     
  Branches     1893        0    -1893     
==========================================
- Hits        33576    31429    -2147     
+ Misses       4986     4590     -396     
+ Partials        7        0       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2025-04-15T05:49:42Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

.install/install_llvm.sh

CMakeLists.txt

GuyAv46 · 2025-04-22T14:04:16Z

build.sh

+      TESTS|tests)
+        BUILD_TESTS=1


Bad name IMO. This script also runs the tests (we may want to rename the script as well)

Will address it in next PR that is dedicated for building tests

GuyAv46 · 2025-04-22T14:05:13Z

build.sh

+      SA|sa)
+        SA=1
+        ;;
+      REDIS_STANDALONE|redis_standalone)
+        REDIS_STANDALONE=1
+        ;;


Some say this is not intuitive, and we better use CLUSTER=1 instead of SA=0

I agree
WIll address it in next PR :)

GuyAv46 · 2025-04-22T14:07:00Z

build.sh

+COORD="oss"      # Coordinator type: oss or rlec
+DEBUG=0          # Debug build flag
+PROFILE=0        # Profile build flag
+FORCE=0          # Force clean build flag
+VERBOSE=0        # Verbose output flag
+QUICK=0          # Quick test mode (subset of tests)
+
+# Test configuration (0=disabled, 1=enabled)
+BUILD_TESTS=0    # Build test binaries
+RUN_UNIT_TESTS=0 # Run C/C++ unit tests
+RUN_RUST_TESTS=0 # Run Rust tests
+RUN_PYTEST=0     # Run Python tests
+RUN_ALL_TESTS=0  # Run all test types


Consider taking the environment variable values as default, instead of ignoring them

build.sh

github-actions · 2025-04-23T07:28:06Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

GuyAv46 · 2025-04-23T10:27:26Z

CMakeLists.txt

 # Define debug symbols extraction function
 function(extract_debug_symbols target)
-  if(CMAKE_BUILD_TYPE STREQUAL "Release" AND NOT APPLE AND NOT WIN32)
+  if(CMAKE_BUILD_TYPE STREQUAL "RelWithDebInfo" AND NOT APPLE)


GuyAv46 · 2025-04-23T10:28:59Z

build.sh

 RUN_RUST_TESTS=0 # Run Rust tests
 RUN_PYTEST=0     # Run Python tests
-RUN_ALL_TESTS=0  # Run all test types
+RUN_ALL_TESTS=${RUN_ALL_TESTS:-0}  # Run all test types


meant to do it on all inputs... so we can accept both attributes (./build.sh ARG=value) and environment variables (VAR=value ./build.sh)

redisearch-backport-pull-request · 2025-04-23T11:51:53Z

Backport failed for 2.8, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.8
git worktree add -d .worktree/backport-5737-to-2.8 origin/2.8
cd .worktree/backport-5737-to-2.8
git switch --create backport-5737-to-2.8
git cherry-pick -x 39fcd2c0a38a915d73ad5aa6a56a19f7a88c18e0

redisearch-backport-pull-request · 2025-04-23T11:51:54Z

Backport failed for 2.6, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.6
git worktree add -d .worktree/backport-5737-to-2.6 origin/2.6
cd .worktree/backport-5737-to-2.6
git switch --create backport-5737-to-2.6
git cherry-pick -x 39fcd2c0a38a915d73ad5aa6a56a19f7a88c18e0

redisearch-backport-pull-request · 2025-04-23T11:51:56Z

Backport failed for 2.10, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.10
git worktree add -d .worktree/backport-5737-to-2.10 origin/2.10
cd .worktree/backport-5737-to-2.10
git switch --create backport-5737-to-2.10
git cherry-pick -x 39fcd2c0a38a915d73ad5aa6a56a19f7a88c18e0

redisearch-backport-pull-request · 2025-04-23T11:51:57Z

Backport failed for 8.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 8.0
git worktree add -d .worktree/backport-5737-to-8.0 origin/8.0
cd .worktree/backport-5737-to-8.0
git switch --create backport-5737-to-8.0
git cherry-pick -x 39fcd2c0a38a915d73ad5aa6a56a19f7a88c18e0

…-5722] (#5737)" This reverts commit 39fcd2c.

) Revert "Refactor build system - remove readies from module build [MOD-5722] (#5737)" This reverts commit 39fcd2c.

…5737) * purge readeis from cmake. Use new build script instead of makefile * call script in CI * libuv * restore build folder * libuv * unit tests * pretty build.sh * pytest * fixed extension build error * Add profile flags + try pass linker flag properly (wip) * fix build and linkage + pytest + adjust json * small cleanup * Add pytz dep * bump googletest version, use same variant name as before * remove ld libs * Fix executable linker flags - build unit test properly. run unit tests with old "make unit-tests" way (until this is fixed via ./build.sh) * fix rust build via build.sh * fix san build * Set coverage flags and use the same bin dir for sanitizer (build it for debug) * update benchmark image for regression test * remove leftover * Build hiredis static * fix json env flag in CI * CR changes * Restore deleted files until unit tests are done as well, use boolean params * restore vecsim version * fix profile and fix build error * fix for profile * try fix mac build * fix unit tests for arm * Define boost dir * remove the policies * try fix the binroot for unit-tests * change dir name for arm * remove policy from hash as well * use clang in mac * try to set hardcoded clang * try to mimic readies in macos includes * try set CMAKE_OSX_DEPLOYMENT_TARGET * whitespace formatting * Set CC to clang in apple * try to export llvm * try set compiler in build script * use bin in path * try EXPORT properly with homebrew * remove setting bad path to clang * set clang path * update to llvm@18 * update c++ path * set compiler with LLVM env var * fix? * set proper link flags for mac * remove bsymbolic from hiredis * revert hiredis in non macos to how it was * clean stuff * cleanups --------- Co-authored-by: alon <alonreshef24@gmail.com> Co-authored-by: GuyAv46 <guy.avimor@gmail.com>

) Revert "Refactor build system - remove readies from module build [MOD-5722] (#5737)" This reverts commit 39fcd2c.

…5737) * purge readeis from cmake. Use new build script instead of makefile * call script in CI * libuv * restore build folder * libuv * unit tests * pretty build.sh * pytest * fixed extension build error * Add profile flags + try pass linker flag properly (wip) * fix build and linkage + pytest + adjust json * small cleanup * Add pytz dep * bump googletest version, use same variant name as before * remove ld libs * Fix executable linker flags - build unit test properly. run unit tests with old "make unit-tests" way (until this is fixed via ./build.sh) * fix rust build via build.sh * fix san build * Set coverage flags and use the same bin dir for sanitizer (build it for debug) * update benchmark image for regression test * remove leftover * Build hiredis static * fix json env flag in CI * CR changes * Restore deleted files until unit tests are done as well, use boolean params * restore vecsim version * fix profile and fix build error * fix for profile * try fix mac build * fix unit tests for arm * Define boost dir * remove the policies * try fix the binroot for unit-tests * change dir name for arm * remove policy from hash as well * use clang in mac * try to set hardcoded clang * try to mimic readies in macos includes * try set CMAKE_OSX_DEPLOYMENT_TARGET * whitespace formatting * Set CC to clang in apple * try to export llvm * try set compiler in build script * use bin in path * try EXPORT properly with homebrew * remove setting bad path to clang * set clang path * update to llvm@18 * update c++ path * set compiler with LLVM env var * fix? * set proper link flags for mac * remove bsymbolic from hiredis * revert hiredis in non macos to how it was * clean stuff * cleanups --------- Co-authored-by: alon <alonreshef24@gmail.com> Co-authored-by: GuyAv46 <guy.avimor@gmail.com>

) Revert "Refactor build system - remove readies from module build [MOD-5722] (#5737)" This reverts commit 39fcd2c.

DvirDukhan added 2 commits March 10, 2025 00:46

purge readeis from cmake. Use new build script instead of makefile

d477db5

call script in CI

48cceb5

DvirDukhan changed the title ~~Purge readies from the build~~ [WIP] Purge readies from the build Mar 9, 2025

DvirDukhan added 5 commits March 10, 2025 01:22

libuv

205326a

restore build folder

f2babb9

libuv

88fce1f

unit tests

66d9a80

pretty build.sh

caa31a8

benoitdion reviewed Mar 10, 2025

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

.github/workflows/task-test.yml Show resolved Hide resolved

DvirDukhan added 2 commits March 10, 2025 18:02

pytest

29610d1

fixed extension build error

7c52ac9

alonre24 added 9 commits March 13, 2025 16:52

Add profile flags + try pass linker flag properly (wip)

e579e29

fix build and linkage + pytest + adjust json

c3646e9

rebase master

7bd5d66

small cleanup

444f76c

Merge branch 'master' into dvirdu_purge_readies

612e538

Add pytz dep

7093163

bump googletest version, use same variant name as before

652864d

remove ld libs

93716d4

Fix executable linker flags - build unit test properly.

855444e

run unit tests with old "make unit-tests" way (until this is fixed via ./build.sh)

alonre24 marked this pull request as ready for review March 19, 2025 18:59

alonre24 added 4 commits March 22, 2025 21:38

fix rust build via build.sh

1170f34

Merge branch 'master' into dvirdu_purge_readies

051c9d2

fix san build

173256f

Set coverage flags and use the same bin dir for sanitizer (build it f…

84af188

…or debug)

alonre24 added the action:run-benchmark label Mar 24, 2025

update benchmark image for regression test

f051a66

alonre24 changed the title ~~[WIP] Purge readies from the build~~ Refactor build system - remove readies from module build Mar 25, 2025

clean stuff

8a27ad4

alonre24 changed the title ~~Refactor build system - remove readies from module build~~ Refactor build system - remove readies from module build [MOD-5722] Apr 22, 2025

alonre24 reviewed Apr 22, 2025

View reviewed changes

.install/install_llvm.sh Outdated Show resolved Hide resolved

alonre24 reviewed Apr 22, 2025

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

alonre24 reviewed Apr 22, 2025

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

GuyAv46 reviewed Apr 22, 2025

View reviewed changes

cleanups

c8a2c20

alonre24 requested a review from GuyAv46 April 23, 2025 09:55

GuyAv46 approved these changes Apr 23, 2025

View reviewed changes

alonre24 added this pull request to the merge queue Apr 23, 2025

alonre24 added backport 2.8 backport 2.6 backport 2.10 backport 8.0 labels Apr 23, 2025

Merged via the queue into master with commit 39fcd2c Apr 23, 2025
30 checks passed

alonre24 deleted the dvirdu_purge_readies branch April 23, 2025 11:51

alonre24 added a commit that referenced this pull request Apr 23, 2025

Revert "Refactor build system - remove readies from module build [MOD…

a06c4a2

…-5722] (#5737)" This reverts commit 39fcd2c.

alonre24 mentioned this pull request Apr 23, 2025

Revert "Refactor build system - remove readies from module build" #6003

Merged

2 tasks

github-merge-queue bot pushed a commit that referenced this pull request Apr 23, 2025

Revert "Refactor build system - remove readies from module build" (#6003

5bc999e

) Revert "Refactor build system - remove readies from module build [MOD-5722] (#5737)" This reverts commit 39fcd2c.

JoanFM pushed a commit that referenced this pull request May 27, 2025

Revert "Refactor build system - remove readies from module build" (#6003

e408276

) Revert "Refactor build system - remove readies from module build [MOD-5722] (#5737)" This reverts commit 39fcd2c.

JoanFM pushed a commit that referenced this pull request May 27, 2025

Revert "Refactor build system - remove readies from module build" (#6003

b812490

) Revert "Refactor build system - remove readies from module build [MOD-5722] (#5737)" This reverts commit 39fcd2c.

Conversation

DvirDukhan commented Mar 9, 2025 • edited by alonre24 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuyAv46 commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Apr 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuyAv46 Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

alonre24 Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

GuyAv46 Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

alonre24 Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

GuyAv46 Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Apr 23, 2025

Uh oh!

GuyAv46 Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

GuyAv46 Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented Apr 23, 2025

Uh oh!

redisearch-backport-pull-request bot commented Apr 23, 2025

Uh oh!

redisearch-backport-pull-request bot commented Apr 23, 2025

Uh oh!

redisearch-backport-pull-request bot commented Apr 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DvirDukhan commented Mar 9, 2025 •

edited by alonre24

Loading

GuyAv46 commented Mar 11, 2025 •

edited

Loading

codecov bot commented Mar 23, 2025 •

edited

Loading