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.

Sign up for GitHub

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

Jump to bottom

cmake: Switch to libsecp256k1 upstream build system #192

Merged
merged 8 commits into from
Jun 30, 2024
Merged

cmake: Switch to libsecp256k1 upstream build system #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f0aec78
cmake: Switch to libsecp256k1 upstream build system
hebasto Jun 28, 2024
c3134b0
cmake: Let libsecp256k1 manage config-specific C flags
hebasto May 25, 2024
469e1ad
cmake: Remove C stuff from summary
hebasto May 25, 2024
65f15d1
cmake: Pass to secp256k1 build system only sanitizer flags
hebasto Jun 25, 2024
cdd843a
cmake: Override build configurations for libsecp256k1
hebasto Jun 22, 2024
70ec2d9
cmake: Reorder subdirectories
hebasto Jun 25, 2024
bfe7c4d
[FIXUP] cmake: Enable tests before adding subdirectories
hebasto Jun 26, 2024
458d54c
cmake: Run secp256k1 tests
hebasto Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 11 additions & 27 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,11 +322,13 @@ target_link_libraries(core_base_interface INTERFACE
Threads::Threads
)

add_library(sanitize_interface INTERFACE)
target_link_libraries(core_base_interface INTERFACE sanitize_interface)
if(SANITIZERS)
# First check if the compiler accepts flags. If an incompatible pair like
# -fsanitize=address,thread is used here, this check will fail. This will also
# fail if a bad argument is passed, e.g. -fsanitize=undfeined
try_append_cxx_flags("-fsanitize=${SANITIZERS}" TARGET core_base_interface
try_append_cxx_flags("-fsanitize=${SANITIZERS}" TARGET sanitize_interface
RESULT_VAR cxx_supports_sanitizers
SKIP_LINK
)
Expand All @@ -353,7 +355,7 @@ if(SANITIZERS)
message(FATAL_ERROR "Linker did not accept requested flags, you are missing required libraries.")
endif()
endif()
target_link_options(core_base_interface INTERFACE ${SANITIZER_LDFLAGS})
target_link_options(sanitize_interface INTERFACE ${SANITIZER_LDFLAGS})

include(AddBoostIfNeeded)
add_boost_if_needed()
Expand All @@ -369,7 +371,6 @@ include(cmake/ccache.cmake)
include(cmake/crc32c.cmake)
include(cmake/leveldb.cmake)
include(cmake/minisketch.cmake)
include(cmake/secp256k1.cmake)

add_library(warn_interface INTERFACE)
target_link_libraries(core_interface INTERFACE warn_interface)
Expand Down Expand Up @@ -429,15 +430,12 @@ target_compile_definitions(core_interface_debug INTERFACE
)
# We leave assertions on.
if(MSVC)
remove_c_flag_from_all_configs(/DNDEBUG)
remove_cxx_flag_from_all_configs(/DNDEBUG)
else()
remove_c_flag_from_all_configs(-DNDEBUG)
remove_cxx_flag_from_all_configs(-DNDEBUG)

# Adjust flags used by the C/CXX compiler during RELEASE builds.
# Adjust flags used by the CXX compiler during RELEASE builds.
# Prefer -O2 optimization level. (-O3 is CMake's default for Release for many compilers.)
replace_c_flag_in_config(Release -O3 -O2)
replace_cxx_flag_in_config(Release -O3 -O2)

are_flags_overridden(CMAKE_CXX_FLAGS_DEBUG cxx_flags_debug_overridden)
Expand All @@ -447,6 +445,7 @@ else()
if(compiler_supports_g3)
replace_cxx_flag_in_config(Debug -g -g3)
endif()
unset(compiler_supports_g3)

try_append_cxx_flags("-ftrapv" RESULT_VAR compiler_supports_ftrapv)
if(compiler_supports_ftrapv)
Expand All @@ -463,24 +462,6 @@ else()
)
endif()
unset(cxx_flags_debug_overridden)

are_flags_overridden(CMAKE_C_FLAGS_DEBUG c_flags_debug_overridden)
if(NOT c_flags_debug_overridden)
# Redefine flags used by the C compiler during DEBUG builds.
if(compiler_supports_g3)
replace_c_flag_in_config(Debug -g -g3)
endif()

string(PREPEND CMAKE_C_FLAGS_DEBUG "-O0 ")

set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG}"
CACHE STRING
"Flags used by the C compiler during DEBUG builds."
FORCE
)
endif()
unset(compiler_supports_g3)
unset(c_flags_debug_overridden)
endif()

set(CMAKE_CXX_FLAGS_COVERAGE "-Og --coverage")
Expand Down Expand Up @@ -599,6 +580,9 @@ if(DEFINED ENV{LDFLAGS})
deduplicate_flags(CMAKE_EXE_LINKER_FLAGS)
endif()

if(BUILD_TESTS)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed for secp256k1 specifically?

Copy link
Owner Author

@hebasto hebasto Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not secp256k1-specific change. As a part of the "[FIXUP] cmake: Enable tests before adding subdirectories" commit this change adjusts the moment when enable_testing() being called. Previously, it was done implicitly via include(CTest) command after processing the src subdirectory. Now it done explicitly and before processing the src subdirectory. Which activates any add_test commands within the src subdirectory including the src/secp256k1 subdirectory as well.

enable_testing()
endif()
# TODO: The `CMAKE_SKIP_BUILD_RPATH` variable setting can be deleted
# in the future after reordering Guix script commands to
# perform binary checks after the installation step.
Expand All @@ -607,10 +591,11 @@ endif()
# - https://github.com/bitcoin/bitcoin/pull/30312#issuecomment-2191235833
set(CMAKE_SKIP_BUILD_RPATH TRUE)
set(CMAKE_SKIP_INSTALL_RPATH TRUE)
add_subdirectory(src)
add_subdirectory(test)
add_subdirectory(doc)

add_subdirectory(src)

include(cmake/tests.cmake)

include(Maintenance)
Expand Down Expand Up @@ -664,7 +649,6 @@ else()
set(cross_status "FALSE")
endif()
message("Cross compiling ....................... ${cross_status}")
message("C compiler ............................ ${CMAKE_C_COMPILER_ID} ${CMAKE_C_COMPILER_VERSION}, ${CMAKE_C_COMPILER}")
message("C++ compiler .......................... ${CMAKE_CXX_COMPILER_ID} ${CMAKE_CXX_COMPILER_VERSION}, ${CMAKE_CXX_COMPILER}")
include(FlagsSummary)
flags_summary()
Expand Down
11 changes: 0 additions & 11 deletions cmake/module/FlagsSummary.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,6 @@ function(print_flags_per_config config indent_num)
get_target_interface(definitions ${config} core_interface COMPILE_DEFINITIONS)
indent_message("Preprocessor defined macros ..........." "${definitions}" ${indent_num})

string(STRIP "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${config_uppercase}}" combined_c_flags)
string(STRIP "${combined_c_flags} ${CMAKE_C${CMAKE_C_STANDARD}_STANDARD_COMPILE_OPTION}" combined_c_flags)
if(CMAKE_POSITION_INDEPENDENT_CODE)
string(JOIN " " combined_c_flags ${combined_c_flags} ${CMAKE_C_COMPILE_OPTIONS_PIC})
endif()
get_target_interface(core_c_flags ${config} core_base_interface COMPILE_OPTIONS)
string(STRIP "${combined_c_flags} ${core_c_flags}" combined_c_flags)
string(STRIP "${combined_c_flags} ${APPEND_CPPFLAGS}" combined_c_flags)
string(STRIP "${combined_c_flags} ${APPEND_CFLAGS}" combined_c_flags)
indent_message("C compiler flags ......................" "${combined_c_flags}" ${indent_num})

string(STRIP "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${config_uppercase}}" combined_cxx_flags)
string(STRIP "${combined_cxx_flags} ${CMAKE_CXX${CMAKE_CXX_STANDARD}_STANDARD_COMPILE_OPTION}" combined_cxx_flags)
if(CMAKE_POSITION_INDEPENDENT_CODE)
Expand Down
28 changes: 0 additions & 28 deletions cmake/module/ProcessConfigurations.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,23 +84,6 @@ function(set_default_config config)
endif()
endfunction()

function(remove_c_flag_from_all_configs flag)
get_all_configs(all_configs)
foreach(config IN LISTS all_configs)
string(TOUPPER "${config}" config_uppercase)
set(flags "${CMAKE_C_FLAGS_${config_uppercase}}")
separate_arguments(flags)
list(FILTER flags EXCLUDE REGEX "${flag}")
list(JOIN flags " " new_flags)
set(CMAKE_C_FLAGS_${config_uppercase} "${new_flags}" PARENT_SCOPE)
set(CMAKE_C_FLAGS_${config_uppercase} "${new_flags}"
CACHE STRING
"Flags used by the C compiler during ${config_uppercase} builds."
FORCE
)
endforeach()
endfunction()

function(remove_cxx_flag_from_all_configs flag)
get_all_configs(all_configs)
foreach(config IN LISTS all_configs)
Expand All @@ -118,17 +101,6 @@ function(remove_cxx_flag_from_all_configs flag)
endforeach()
endfunction()

function(replace_c_flag_in_config config old_flag new_flag)
string(TOUPPER "${config}" config_uppercase)
string(REGEX REPLACE "(^| )${old_flag}( |$)" "\\1${new_flag}\\2" new_flags "${CMAKE_C_FLAGS_${config_uppercase}}")
set(CMAKE_C_FLAGS_${config_uppercase} "${new_flags}" PARENT_SCOPE)
set(CMAKE_C_FLAGS_${config_uppercase} "${new_flags}"
CACHE STRING
"Flags used by the C compiler during ${config_uppercase} builds."
FORCE
)
endfunction()

function(replace_cxx_flag_in_config config old_flag new_flag)
string(TOUPPER "${config}" config_uppercase)
string(REGEX REPLACE "(^| )${old_flag}( |$)" "\\1${new_flag}\\2" new_flags "${CMAKE_CXX_FLAGS_${config_uppercase}}")
Expand Down
62 changes: 0 additions & 62 deletions cmake/secp256k1.cmake
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions cmake/tests.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
# Distributed under the MIT software license, see the accompanying
# file COPYING or https://opensource.org/license/mit/.

include(CTest)

if(TARGET bitcoin-util AND TARGET bitcoin-tx AND PYTHON_COMMAND)
add_test(NAME util_test_runner
COMMAND ${CMAKE_COMMAND} -E env BITCOINUTIL=$<TARGET_FILE:bitcoin-util> BITCOINTX=$<TARGET_FILE:bitcoin-tx> ${PYTHON_COMMAND} ${CMAKE_BINARY_DIR}/test/util/test_runner.py
Expand Down
36 changes: 34 additions & 2 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Copyright (c) 2023 The Bitcoin Core developers
# Copyright (c) 2023-present The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
# file COPYING or https://opensource.org/license/mit/.

include(GNUInstallDirs)
include(AddWindowsResources)
Expand Down Expand Up @@ -38,6 +38,38 @@ if(WITH_MULTIPROCESS)
add_subdirectory(ipc)
endif()

#=============================
# secp256k1 subtree
#=============================
message("")
message("Configuring secp256k1 subtree...")
set(SECP256K1_DISABLE_SHARED ON CACHE BOOL "" FORCE)
set(SECP256K1_ENABLE_MODULE_ECDH OFF CACHE BOOL "" FORCE)
set(SECP256K1_ENABLE_MODULE_RECOVERY ON CACHE BOOL "" FORCE)
set(SECP256K1_ECMULT_GEN_KB 86 CACHE STRING "" FORCE)
set(SECP256K1_BUILD_BENCHMARK OFF CACHE BOOL "" FORCE)
set(SECP256K1_BUILD_TESTS ${BUILD_TESTS} CACHE BOOL "" FORCE)
set(SECP256K1_BUILD_EXHAUSTIVE_TESTS ${BUILD_TESTS} CACHE BOOL "" FORCE)
set(SECP256K1_BUILD_CTIME_TESTS OFF CACHE BOOL "" FORCE)
set(SECP256K1_BUILD_EXAMPLES OFF CACHE BOOL "" FORCE)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we landed on that running the ctime tests would actually be valuable? If not, or if it's waiting on something else, it'd be good to have a comment here explaining why these tests are being skipped.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry if I misunderstood the previous discussion, but I thought that the idea was to mirror the master branch behaviour during migration to CMake.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that the idea was to mirror the master branch behaviour during migration to CMake.

Ok, but the ctime tests are currently compiled, and runnable there.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Fixed in #253.

include(GetTargetInterface)
# -fsanitize and related flags apply to both C++ and C,
# so we can pass them down to libsecp256k1 as CFLAGS.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be passing down LDFLAGS? Or is that being done somwhere else?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be passing down LDFLAGS?

Why? libsecp256k1 is a static library, and no linker is used to produce it, right?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See bitcoin#30791 (comment).

get_target_interface(core_sanitizer_cxx_flags "" sanitize_interface COMPILE_OPTIONS)
set(SECP256K1_LATE_CFLAGS ${core_sanitizer_cxx_flags} CACHE STRING "" FORCE)
unset(core_sanitizer_cxx_flags)
# We want to build libsecp256k1 with the most tested RelWithDebInfo configuration.
enable_language(C)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to build libsecp256k1 with the most tested RelWithDebInfo configuration.

Not sure this comment adds much value, and the CMake config is not really the most relevant thing? I'd say what matters more is the secp256k1 configuration, which currently, diverges from upstream, and isn't extensively tested.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you suggest an improved comment or other approach?

foreach(config IN LISTS CMAKE_BUILD_TYPE CMAKE_CONFIGURATION_TYPES)
if(config STREQUAL "")
continue()
endif()
string(TOUPPER "${config}" config)
set(CMAKE_C_FLAGS_${config} "${CMAKE_C_FLAGS_RELWITHDEBINFO}")
endforeach()
add_subdirectory(secp256k1)
set_target_properties(secp256k1 PROPERTIES EXPORT_COMPILE_COMMANDS OFF)
string(APPEND CMAKE_C_COMPILE_OBJECT " ${APPEND_CPPFLAGS} ${APPEND_CFLAGS}")

# Stable, backwards-compatible consensus functionality.
add_library(bitcoin_consensus STATIC EXCLUDE_FROM_ALL
Expand Down
Loading