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.69%. Comparing base (2743b7e) to head (da9b98d).
Report is 34 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.69%   -0.01%     
============================================
  Files           114      114              
  Lines         63076    63150      +74     
============================================
+ Hits          44594    44644      +50     
- Misses        18482    18506      +24

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

... and 20 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

- 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)

- Drop TLS module build - Added CI for building on Ubuntu (latest) using CMake and running tests (unit tests + tcl tests) - lualib: create a proper CMake file and don't rely on the old Makefile Signed-off-by: Eran Ifrah <[email protected]>

- YAML formatting - CMake: use the correct keyword "WARNING" instead of "WARN" Signed-off-by: Eran Ifrah <[email protected]>

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

eifrah-aws · 2024-11-05T23:38:22Z

@madolson / @PingXie

TLS module build was dropped
CI updated with CMake (running tcl tests + runtests suite)

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

madolson

Didn't see anyone else complain, so I guess we can merge it and work to maintain it now.

PingXie

Great work, eifrah-aws! Appreciate it

PingXie · 2024-11-08T00:11:32Z

README.md

+
+Other options supported by Valkey's `CMake` build system:
+
+## Special build flags


not blocking this PR but can we also document the default settings for these flags?

Will be fixed as part of this small PR: #1276

PingXie · 2024-11-08T00:22:02Z

cmake/Modules/Packaging.cmake

@@ -0,0 +1,44 @@
+set(CPACK_PACKAGE_NAME "valkey")
+
+valkey_parse_version(CPACK_PACKAGE_VERSION_MAJOR CPACK_PACKAGE_VERSION_MINOR CPACK_PACKAGE_VERSION_PATCH)


can we consider a custom versioning scheme for the unstable branch such as ${CPACK_PACKAGE_NAME}-unstable-${CMAKE_SYSTEM_NAME}? It will look like ${CPACK_PACKAGE_NAME}-255.255.255-${CMAKE_SYSTEM_NAME} otherwise. I am a bit more in favor of explicitness. Separate PR is totally fine.

+1 for ${CPACK_PACKAGE_NAME}-unstable-${CMAKE_SYSTEM_NAME} for unstable branch

With this commit, 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: ```bash 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`: ```bash 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: ```bash mkdir build-asan cd $_ cmake .. -DBUILD_SANITIZER=address -DBUILD_MALLOC=libc make -j$(nproc) ``` ASAN with jemalloc: ```bash 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`: ```bash 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: ```bash 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): ```bash 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 ``` Signed-off-by: Eran Ifrah <[email protected]>

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 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

eifrah-aws added 3 commits November 6, 2024 00:53

Addressed PR comments

e9fb7f1

- Drop TLS module build - Added CI for building on Ubuntu (latest) using CMake and running tests (unit tests + tcl tests) - lualib: create a proper CMake file and don't rely on the old Makefile Signed-off-by: Eran Ifrah <[email protected]>

More fixes

046fda4

- YAML formatting - CMake: use the correct keyword "WARNING" instead of "WARN" Signed-off-by: Eran Ifrah <[email protected]>

CI fixes: added symbolic links to missing binaries

0b6db86

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

eifrah-aws requested a review from madolson November 5, 2024 23:38

Updated .gitignore file

da9b98d

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

madolson approved these changes Nov 7, 2024

View reviewed changes

madolson added the release-notes This issue should get a line item in the release notes label Nov 7, 2024

PingXie approved these changes Nov 8, 2024

View reviewed changes

madolson merged commit 07b3e7a into valkey-io:unstable Nov 8, 2024
57 checks passed

eifrah-aws deleted the cmake branch November 8, 2024 05:04

pizhenwei mentioned this pull request Nov 8, 2024

RDMA builtin support #1209

Open

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

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

eifrah-aws commented Nov 5, 2024 •

edited

Loading

madolson left a comment

PingXie left a comment

PingXie Nov 8, 2024

eifrah-aws Nov 8, 2024

PingXie Nov 8, 2024

eifrah-aws Nov 8, 2024


		Other options supported by Valkey's `CMake` build system:

		## Special build flags

		@@ -0,0 +1,44 @@
		set(CPACK_PACKAGE_NAME "valkey")

		valkey_parse_version(CPACK_PACKAGE_VERSION_MAJOR CPACK_PACKAGE_VERSION_MINOR CPACK_PACKAGE_VERSION_PATCH)

Add CMake build system for valkey #1196

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

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 Nov 5, 2024 • edited Loading

madolson left a comment

Choose a reason for hiding this comment

PingXie left a comment

Choose a reason for hiding this comment

PingXie Nov 8, 2024

Choose a reason for hiding this comment

eifrah-aws Nov 8, 2024

Choose a reason for hiding this comment

PingXie Nov 8, 2024

Choose a reason for hiding this comment

eifrah-aws Nov 8, 2024

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

eifrah-aws commented Nov 5, 2024 •

edited

Loading