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

[cmake] Fix cmake options position to support cmake toolchain #1250

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

uilianries
Copy link
Contributor

Greetings!

By declaring options in the CMake toolchain after the project() command in the CMakeLists.txt, you ensure that they are applied within the context of the project's environment, making them more coherent and ensuring that they override any default settings appropriately.

However, the current CMakeLists.txt declares options() first, before project(), which means, the context is not initialized and the options values from toolchain is ignored.

The current scenario WITHOUT this PR:

# hiredis_toolchain.cmake
set(BUILD_SHARED_LIBS OFF)
set(ENABLE_SSL ON)
set(DISABLE_TESTS ON)
set(ENABLE_SSL_TESTS OFF)
set(ENABLE_EXAMPLES OFF)
set(ENABLE_ASYNC_TESTS OFF)

cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE=hiredis_toolchain.cmake

% cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE=hiredis_toolchain.cmake -LAH
Detected version: 1.2.0
-- The C compiler identification is AppleClang 15.0.0.15000100
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Found OpenSSL: /opt/homebrew/Cellar/openssl@3/3.2.0_1/lib/libcrypto.dylib (found version "3.2.0")  
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/uilian/Development/hiredis/build
-- Cache values
// Build shared libraries
BUILD_SHARED_LIBS:BOOL=ON

// The CMake toolchain file
CMAKE_TOOLCHAIN_FILE:FILEPATH=/Users/uilian/Development/hiredis/hiredis_toolchain.cmake

// Enable building hiredis examples
ENABLE_EXAMPLES:BOOL=OFF

// Install NuGET packaging details
ENABLE_NUGET:BOOL=ON

// Build hiredis_ssl for SSL support
ENABLE_SSL:BOOL=OFF

// Should we test SSL connections
ENABLE_SSL_TESTS:BOOL=OFF

As you can see, the toolchain file is consumed, but CMake ignores its values.

In order to fix the current situation, options() should be moved after project().

@michael-grunder michael-grunder merged commit 398e16e into redis:master Feb 14, 2024
12 checks passed
@michael-grunder
Copy link
Collaborator

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants