Add CMake build system for valkey #1196

eifrah-aws · 2024-10-20T21:20:34Z

Replacing this PR #1082

NOTE:

This PR was tested on:

Amazon Linux 2
Ubuntu 22.04
AlmaLinux 9
macOS Sonoma 14.7 (ARM)
FreeBSD 9 - note on FreeBSD, we require gmake for building valkey

With this PR, users are able to build valkey using CMake.

Example usage:

Build valkey-server in Release mode with TLS enabled and using jemalloc as the allocator:

mkdir build-release
cd $_
cmake .. -DCMAKE_BUILD_TYPE=Release \
         -DCMAKE_INSTALL_PREFIX=/tmp/valkey-install \
         -DBUILD_MALLOC=jemalloc -DBUILD_TLS=1
make -j$(nproc) install

# start valkey
/tmp/valkey-install/bin/valkey-server

Build valkey-unit-tests:

mkdir build-release-ut
cd $_
cmake .. -DCMAKE_BUILD_TYPE=Release \
         -DBUILD_MALLOC=jemalloc -DBUILD_UNIT_TESTS=1
make -j$(nproc)

# Run the tests
./bin/valkey-unit-tests

Current features supported by this PR:

Building against different allocators: (jemalloc, tcmalloc, tcmalloc_minimal and libc), e.g. to enable jemalloc pass -DBUILD_MALLOC=jemalloc to cmake
OpenSSL builds (to enable TLS, pass -DBUILD_TLS=1 to cmake)
Sanitizier: pass -DBUILD_SANITIZER=<address|thread|undefined> to cmake
Install target + redis symbolic links
Build valkey-unit-tests executable
Standard CMake variables are supported. e.g. to install valkey under /home/you/root pass -DCMAKE_INSTALL_PREFIX=/home/you/root

Why using CMake? To list some of the advantages of using CMake:

Superior IDE integrations: cmake generates the file compile_commands.json which is required by clangd to get a compiler accuracy code completion (in other words: your VScode will thank you)
Out of the source build tree: with the current build system, object files are created all over the place polluting the build source tree, the best practice is to build the project on a separate folder
Multiple build types co-existing: with the current build system, it is often hard to have multiple build configurations. With cmake you can do it easily:
It is the de-facto standard for C/C++ project these days

More build examples:

ASAN build:

mkdir build-asan
cd $_
cmake .. -DBUILD_SANITIZER=address -DBUILD_MALLOC=libc
make -j$(nproc)

ASAN with jemalloc:

mkdir build-asan-jemalloc
cd $_
cmake .. -DBUILD_SANITIZER=address -DBUILD_MALLOC=jemalloc 
make -j$(nproc)

As seen by the previous examples, any combination is allowed and co-exist on the same source tree.

Valkey installation

With this new CMake, it is possible to install the binary by running make install or creating a package make package (currently supported on Debian like distros)

Example 1: build & install using `make install`:

mkdir build-release
cd $_
cmake .. -DCMAKE_INSTALL_PREFIX=$HOME/valkey-install -DCMAKE_BUILD_TYPE=Release
make -j$(nproc) install
# valkey is now installed under $HOME/valkey-install

Example 2: create a `.deb` installer:

mkdir build-release
cd $_
cmake .. -DCMAKE_BUILD_TYPE=Release
make -j$(nproc) package
# ... CPack deb generation output
sudo gdebi -n ./valkey_8.1.0_amd64.deb
# valkey is now installed under /opt/valkey

Example 3: create installer for non Debian systems (e.g. FreeBSD or macOS):

mkdir build-release
cd $_
cmake .. -DCMAKE_BUILD_TYPE=Release
make -j$(nproc) package
mkdir -p /opt/valkey && ./valkey-8.1.0-Darwin.sh --prefix=/opt/valkey  --exclude-subdir
# valkey-server is now installed under /opt/valkey

Current CMake PR status:

Signed-off-by: Eran Ifrah <[email protected]>

eifrah-aws · 2024-10-20T21:23:01Z

@PingXie see this PR instead.

codecov · 2024-10-20T21:34:41Z

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.49%. Comparing base (2743b7e) to head (86ae036).
Report is 21 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/debug.c	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1196      +/-   ##
============================================
- Coverage     70.69%   70.49%   -0.21%     
============================================
  Files           114      114              
  Lines         63076    63150      +74     
============================================
- Hits          44594    44516      -78     
- Misses        18482    18634     +152

Files with missing lines	Coverage Δ
src/server.c	`87.73% <ø> (-1.02%)`	⬇️
src/debug.c	`53.05% <0.00%> (ø)`

... and 17 files with indirect coverage changes

pizhenwei

Is it possible to use WITH_TLS instead of USE_TLS? Then we have a single TLS related building variable.

cmake/Modules/ValkeySetup.cmake

PingXie

Thanks @eifrah-aws

.cmake-format.yaml

CMakeLists.txt

cmake/Modules/Packaging.cmake

CMakeLists.txt

cmake/Modules/SourceFiles.cmake

cmake/Modules/ValkeySetup.cmake

CMakeLists.txt

pizhenwei · 2024-10-21T03:02:14Z

Is it possible to generate config.h by cmake?
I guess this may be the next step.

- Force function names to lowercase in YAML config file - Parse version from version.h instead of hard-coding it - Fixed typo - Moved utility functions into their own module `Utils.cmake` - macOS: do not hard code "/usr/bin/clang", instead use `find_program` - Support for WITH_TLS=module|yes|off|1|0 - Support for WITH_RDMA=module|off|0 - Fixed (never worked for the original Makefile as well): build TLS as module on macOS Signed-off-by: Eran Ifrah <[email protected]>

eifrah-aws · 2024-10-21T17:52:09Z

Is it possible to generate config.h by cmake?
I guess this may be the next step.

Yes, but this will break Makefile users. So until CMake is agreed as the main build tool, I wouldn't go this path

pizhenwei

Hi, I also find the missing CMakeLists.txt for tests:

$ find tests -name Makefile
tests/rdma/Makefile
tests/modules/Makefile

And cmake related help information in README.md is needed?

cmake/Modules/Utils.cmake

cmake/Modules/ValkeySetup.cmake

PingXie · 2024-10-22T06:11:50Z

I wonder if we should drop building TLS as a module now in CMake. Looks like RDMA can only be built as a module now, which is fine, as long as we don't support two build systems.

@valkey-io/core-team thoughts?

- Fixed comment - Use a more accurate variable name `RDMACM` as the output prefix - Avoid using condition like `if (USE_TLS EQUAL 2)`, instead use `if (BUILD_TLS_MODULE)` - Build "hello*" moodules - Build "test" modules (using conditional variable `-DBUILD_TEST_MODULES=1`) - Build "rdma-test" Signed-off-by: Eran Ifrah <[email protected]>

Signed-off-by: Eran Ifrah <[email protected]>

eifrah-aws · 2024-10-22T14:20:49Z

Hi, I also find the missing CMakeLists.txt for tests:

Fixed with recent commit

Signed-off-by: Eran Ifrah <[email protected]>

eifrah-aws · 2024-10-22T16:56:37Z

And cmake related help information in README.md is needed?

README updated

madolson

I am fine removing building TLS as a module. No longer really seems necessary.

Before merging, we should all add it to the CI to make sure it runs as expected.

README.md

src/modules/CMakeLists.txt

src/debug.c

src/server.c

README.md

- Build of the example modules is now conditional (defaults to NO) - README.md fixup - Fixed UT build (exposed VALKEY_USE_TEST_MAIN & VALKEY_USE_TEST_SERVER_ASSERT macros only for macOS) - Added missing valkeylib for UT build (which was removed previous commit) Signed-off-by: Eran Ifrah <[email protected]>

cmake/Modules/SourceFiles.cmake

- Moved rdma-tests source file to their own file - Unit tests are no longer handled separately - Use CMake function `ProcessorCount` to determine the number of jobs to run when invoking `make` - FreeBSD: valkey-unit-tests: link against execinfo for backtrace symbols Signed-off-by: Eran Ifrah <[email protected]>

pizhenwei

LGTM. Also test this PR on my local platform Ubuntu 22.04.4 LTS, it works fine.

It would be appreciated if adding these message into commit message.

Tested-by: zhenwei pi [email protected]
Reviewed-by: zhenwei pi [email protected]

- Make `main` + `serverAssert` "weak" symbols - Set LTO using CMake proper syntax instead of hacking it into the build flags - For consistency, change all args starting with `WITH_*` to `BUILD_*`. for example: `cmake .. -DBUILD_TLS=module -DBUILD_MALLOC=jemalloc -DBUILD_UNIT_TESTS=1` - Only search for OpenSSL if BUILD_TLS is "on" or "module" Signed-off-by: Eran Ifrah <[email protected]>

Signed-off-by: Eran Ifrah <[email protected]>

eifrah-aws · 2024-10-24T18:47:19Z

@madolson per your suggestion, I made the symbols main & serverAssert "weak" to allow them to be overridden by symbols from test_main.c file. Tested on Ubuntu. Amzn Linux 2, FreeBSD & macOS

eifrah-aws · 2024-10-27T12:22:54Z

@PingXie @madolson can one of you re-start the failing CI job?

Signed-off-by: Eran Ifrah <[email protected]>

eifrah-aws · 2024-10-30T17:12:15Z

@PingXie / @madolson can one of you please re-run the the flaky test? (I am pretty sure its flaky as this PR does not change any source file, nor it tests the artifacts produced by CMake 😉 )

Also, are there any outstanding issues that preventing us from merging this PR?

PingXie · 2024-10-31T07:16:26Z

Yeah that is a flaky test.

Also, are there any outstanding issues that preventing us from merging this PR?

Can we drop the option to build TLS as a module? I think now it is a good time to make this change.

@pizhenwei do you have any concerns?

pizhenwei · 2024-10-31T08:31:37Z

Yeah that is a flaky test.

Also, are there any outstanding issues that preventing us from merging this PR?

Can we drop the option to build TLS as a module? I think now it is a good time to make this change.

@pizhenwei do you have any concerns?

Dropping tls module option is OK to me!

eifrah-aws · 2024-10-31T09:09:58Z

Can we drop the option to build TLS as a module? I think now it is a good time to make this change.

Great, will drop it

madolson

Looks good to me, was able to test and run everything locally on my x86 mac. The only pending item I have now is I would like us to add it to CI so that we don't silently break it. Let's add a separate test here: https://github.com/valkey-io/valkey/blob/unstable/.github/workflows/ci.yml#L13 that does cmake + make. I think for now just doing it on linux is fine.

@valkey-io/core-team I added the approved tag since we did discuss this in our weekly meeting. But tagging again incase anyone else would like to review before we merge.

enjoy-binbin

Look ok to me. (i don't know much about it so did not do the review)

Added CMake build system

f06b624

Signed-off-by: Eran Ifrah <[email protected]>

eifrah-aws marked this pull request as ready for review October 20, 2024 21:20

eifrah-aws mentioned this pull request Oct 20, 2024

Add CMake build system for valkey #1082

Closed

20 tasks

pizhenwei reviewed Oct 21, 2024

View reviewed changes

PingXie added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Oct 21, 2024

pizhenwei reviewed Oct 21, 2024

View reviewed changes

cmake/Modules/ValkeySetup.cmake Outdated Show resolved Hide resolved

cmake/Modules/ValkeySetup.cmake Outdated Show resolved Hide resolved

PingXie reviewed Oct 21, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

eifrah-aws requested review from pizhenwei and PingXie October 21, 2024 12:21

pizhenwei reviewed Oct 22, 2024

View reviewed changes

cmake/Modules/Utils.cmake Outdated Show resolved Hide resolved

cmake/Modules/ValkeySetup.cmake Outdated Show resolved Hide resolved

eifrah-aws added 3 commits October 22, 2024 16:48

Fixed build error on Linux

8589132

Signed-off-by: Eran Ifrah <[email protected]>

limit the "-std=gnu11" to linux only

b5993a0

Signed-off-by: Eran Ifrah <[email protected]>

eifrah-aws added 6 commits October 22, 2024 17:21

fixed typo

c64b5d8

Signed-off-by: Eran Ifrah <[email protected]>

Updated README to include CMake section

f166512

Signed-off-by: Eran Ifrah <[email protected]>

Small fixes to the README.md

7734d9c

Signed-off-by: Eran Ifrah <[email protected]>

more README.md small fixes

9c81b77

Signed-off-by: Eran Ifrah <[email protected]>

grammar fixes

be704ac

Signed-off-by: Eran Ifrah <[email protected]>

fixed typos...

5281bb2

Signed-off-by: Eran Ifrah <[email protected]>

eifrah-aws requested a review from pizhenwei October 22, 2024 16:57

madolson reviewed Oct 22, 2024

View reviewed changes

eifrah-aws requested a review from madolson October 22, 2024 21:18

pizhenwei reviewed Oct 23, 2024

View reviewed changes

cmake/Modules/SourceFiles.cmake Outdated Show resolved Hide resolved

eifrah-aws requested a review from pizhenwei October 23, 2024 09:45

pizhenwei reviewed Oct 23, 2024

View reviewed changes

eifrah-aws added 3 commits October 24, 2024 21:37

Fixed typos

83a5840

Signed-off-by: Eran Ifrah <[email protected]>

Fixed yet another typo... :|

ec9ec96

Signed-off-by: Eran Ifrah <[email protected]>

eifrah-aws added 2 commits October 30, 2024 14:18

Reverted local changes done to debug.c / server.c

b5d5a91

Signed-off-by: Eran Ifrah <[email protected]>

Fixed: do not search for OpenSSL for tests if TLS is disabled

86ae036

Signed-off-by: Eran Ifrah <[email protected]>

madolson added the major-decision-pending Major decision pending by TSC team label Oct 31, 2024

madolson reviewed Oct 31, 2024

View reviewed changes

madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Oct 31, 2024

enjoy-binbin approved these changes Nov 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CMake build system for valkey #1196

Add CMake build system for valkey #1196

eifrah-aws commented Oct 20, 2024 •

edited

Loading

eifrah-aws commented Oct 20, 2024

codecov bot commented Oct 20, 2024 •

edited

Loading

pizhenwei left a comment

PingXie left a comment

pizhenwei commented Oct 21, 2024

eifrah-aws commented Oct 21, 2024

pizhenwei left a comment

PingXie commented Oct 22, 2024

eifrah-aws commented Oct 22, 2024

eifrah-aws commented Oct 22, 2024

madolson left a comment •

edited

Loading

pizhenwei left a comment

eifrah-aws commented Oct 24, 2024

eifrah-aws commented Oct 27, 2024 •

edited

Loading

eifrah-aws commented Oct 30, 2024

PingXie commented Oct 31, 2024

pizhenwei commented Oct 31, 2024

eifrah-aws commented Oct 31, 2024 •

edited

Loading

madolson left a comment •

edited

Loading

enjoy-binbin left a comment

Add CMake build system for valkey #1196

Are you sure you want to change the base?

Add CMake build system for valkey #1196

Conversation

eifrah-aws commented Oct 20, 2024 • edited Loading

Example usage:

Valkey installation

Example 1: build & install using make install:

Example 2: create a .deb installer:

Example 3: create installer for non Debian systems (e.g. FreeBSD or macOS):

eifrah-aws commented Oct 20, 2024

codecov bot commented Oct 20, 2024 • edited Loading

Codecov Report

pizhenwei left a comment

Choose a reason for hiding this comment

PingXie left a comment

Choose a reason for hiding this comment

pizhenwei commented Oct 21, 2024

eifrah-aws commented Oct 21, 2024

pizhenwei left a comment

Choose a reason for hiding this comment

PingXie commented Oct 22, 2024

eifrah-aws commented Oct 22, 2024

eifrah-aws commented Oct 22, 2024

madolson left a comment • edited Loading

Choose a reason for hiding this comment

pizhenwei left a comment

Choose a reason for hiding this comment

eifrah-aws commented Oct 24, 2024

eifrah-aws commented Oct 27, 2024 • edited Loading

eifrah-aws commented Oct 30, 2024

PingXie commented Oct 31, 2024

pizhenwei commented Oct 31, 2024

eifrah-aws commented Oct 31, 2024 • edited Loading

madolson left a comment • edited Loading

Choose a reason for hiding this comment

enjoy-binbin left a comment

Choose a reason for hiding this comment

eifrah-aws commented Oct 20, 2024 •

edited

Loading

Example 1: build & install using `make install`:

Example 2: create a `.deb` installer:

codecov bot commented Oct 20, 2024 •

edited

Loading

madolson left a comment •

edited

Loading

eifrah-aws commented Oct 27, 2024 •

edited

Loading

eifrah-aws commented Oct 31, 2024 •

edited

Loading

madolson left a comment •

edited

Loading