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: Switch to libsecp256k1 upstream build system #192

Merged
merged 8 commits into from
Jun 30, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented May 7, 2024

To test this PR, one can compare the ouputs of the following commands:

  • Autotools:
$ make -C src/secp256k1 libsecp256k1.la V=1
  • CMake:
$ cmake --build build -t secp256k1 --verbose

Also sets of the libsecp256k1-specific tests can be compared between make check and ctest.

src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
@hebasto
Copy link
Owner Author

hebasto commented May 7, 2024

For CMake-based projects, it is not common to print the summary at the end of the configuration stage.

Printing summaries by subprojects--libsecp256k1 in this PR and minisketch in the future--makes them less useful for the user due to their verbosity.

@hebasto hebasto force-pushed the 240507-cmake-secp branch from a02f802 to 586d32b Compare May 7, 2024 15:30
@hebasto
Copy link
Owner Author

hebasto commented May 7, 2024

@fanquake
Copy link

fanquake commented May 8, 2024

For CMake-based projects, it is not common to print the summary at the end of the configuration stage.

I'd say that atleast half of the projects we are (going to) build in depends with CMake print some sort of configuration? So I don't think it's uncommon.

Printing summaries by subprojects--libsecp256k1 in this PR and minisketch in the future--makes them less useful for the user due to their verbosity.

I'm not exactly sure what you're trying to say here. It's not less useful if you actually want to see how things have been configured / are going to be compiled, which, if the plan is to use the CMake build systems of our subtrees, we definitely do. This is useful for developers, in CI etc etc.

option(SECP256K1_BUILD_TESTS "" OFF)
option(SECP256K1_BUILD_EXHAUSTIVE_TESTS "" OFF)
option(SECP256K1_BUILD_CTIME_TESTS "" OFF)
add_subdirectory(secp256k1)
Copy link

Choose a reason for hiding this comment

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

Does this need EXCLUDE_FROM_ALL to keep it from installing?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Nope :)

Thanks to:

option(SECP256K1_INSTALL "Enable installation." ${PROJECT_IS_TOP_LEVEL})

Copy link

Choose a reason for hiding this comment

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

I think it still makes sense to go ahead and set that option though, since this is what it's meant for. We could remove the SECP256K1_BUILD_* options and not have to worry about any new binaries being added in the future.

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! Reworked.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Copy link

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

Concept ACK

The libsecp256k1 config looks good to me.

@hebasto
Copy link
Owner Author

hebasto commented May 25, 2024

Rebased on top of the #93.

@hebasto hebasto added the enhancement New feature or request label May 25, 2024
@hebasto hebasto force-pushed the 240507-cmake-secp branch 2 times, most recently from 37ab6d2 to 9f4bf00 Compare May 26, 2024 09:18
@hebasto hebasto force-pushed the 240507-cmake-secp branch from 9f4bf00 to e6d1796 Compare June 4, 2024 08:35
@hebasto hebasto force-pushed the 240507-cmake-secp branch 2 times, most recently from 035c96d to 44a172c Compare June 8, 2024 14:48
@hebasto hebasto marked this pull request as ready for review June 8, 2024 14:49
@hebasto
Copy link
Owner Author

hebasto commented Jun 17, 2024

Friendly ping @TheCharlatan @real-or-random @theuni @fanquake :)

@hebasto hebasto force-pushed the 240507-cmake-secp branch from 44a172c to dd47ca9 Compare June 17, 2024 08:20
@hebasto hebasto added this to the Ready for master milestone Jun 18, 2024
@hebasto hebasto force-pushed the 240507-cmake-secp branch from dd47ca9 to d7e2989 Compare June 18, 2024 13:38
@hebasto
Copy link
Owner Author

hebasto commented Jun 18, 2024

Rebased.

set(SECP256K1_ECMULT_GEN_KB 86 CACHE BOOL "" FORCE)
include(GetTargetInterface)
get_target_interface(core_c_flags "" core_base_interface COMPILE_OPTIONS)
set(SECP256K1_LATE_CFLAGS ${core_c_flags} CACHE STRING "" FORCE)

Choose a reason for hiding this comment

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

What are the flags added on various platforms?

Copy link
Owner Author

@hebasto hebasto Jun 20, 2024

Choose a reason for hiding this comment

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

The core_c_flags variable holds the COMPILE_OPTIONS property of the core_base_interface library, which contains:

  • for MinGW:

    bitcoin/CMakeLists.txt

    Lines 285 to 287 in d7e2989

    # Avoid the use of aligned vector instructions when building for Windows.
    # See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54412.
    try_append_cxx_flags("-Wa,-muse-unaligned-vector-move" TARGET core_base_interface SKIP_LINK)

There are no compile options that are specific to other platforms.

Also it includes sanitizer flags:

bitcoin/CMakeLists.txt

Lines 324 to 327 in d7e2989

try_append_cxx_flags("-fsanitize=${SANITIZERS}" TARGET core_base_interface
RESULT_VAR cxx_supports_sanitizers
SKIP_LINK
)

Choose a reason for hiding this comment

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

The core_c_flags variable holds the COMPILE_OPTIONS property of the core_base_interface library, which contains:
There are no compile options that are specific to other platforms.

Why are we adding that flag specifically, into cflags, for mingw, and no other similar flags on different platforms?

Choose a reason for hiding this comment

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

re offline discussion: why do we have a core_c_flags variable, if we aren't otherwise loading or using a c compiler (and how are things ending up in it).

Choose a reason for hiding this comment

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

  • And these are cxx_flags, so they shouldn't apply to libsecp256k1? Or is it just that try_append_cxx_flags is a bit of a misleading name (in the end it's just COMPILE_OPTIONS?) I'm not sure after looking at https://github.com/hebasto/bitcoin/blob/cmake-staging/cmake/module/TryAppendCXXFlags.cmake .

  • Anyway, I wonder if this flag should be part of the core_base_interface. Just because it's useful for core, doesn't mean we should pass it also when building the dependencies.

Copy link

@real-or-random real-or-random Jun 20, 2024

Choose a reason for hiding this comment

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

re offline discussion: why do we have a core_c_flags variable, if we aren't otherwise loading or using a c compiler (and how are things ending up in it).

Not sure if this answers your question, but the variable is defined only in the get_target_interface(core_c_flags ... It's just a local variable in this file and unset further below.

Choose a reason for hiding this comment

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

Or is it just that try_append_cxx_flags is a bit of a misleading name (in the end it's just COMPILE_OPTIONS?) I'm not sure after looking at cmake-staging/cmake/module/TryAppendCXXFlags.cmake .

I still think that this is a bit confusing. Okay, the sanitizer flags are typically the same for the C++ compiler and the C compiler. If we make this assumption, would it be a good idea to state it explicitly? Something like this:

# -fsanitize and related flags apply to both C++ and C, so we can pass them down to libsecp256k1 as CFLAGS
get_target_interface(core_sanitizier_cxx_flags "" sanitize_interface COMPILE_OPTIONS)
set(SECP256K1_LATE_CFLAGS ${core_sanitizer_cxx_flags} CACHE STRING "" FORCE)

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! Reworked.

@hebasto hebasto force-pushed the 240507-cmake-secp branch from d7e2989 to b3766a3 Compare June 20, 2024 11:26
@hebasto
Copy link
Owner Author

hebasto commented Jun 26, 2024

Considering the recent discussion, which was nicely summarized in #192 (comment), it seems reasonable to proceed in this PR with enabling the secp256k1 tests as it is done in the master branch.

Two new commits did this job.

@hebasto hebasto force-pushed the 240507-cmake-secp branch from 6fb9b31 to 669b611 Compare June 28, 2024 11:23
@hebasto
Copy link
Owner Author

hebasto commented Jun 28, 2024

Addressed @real-or-random's #192 (comment).

@hebasto hebasto force-pushed the 240507-cmake-secp branch from 669b611 to 458d54c Compare June 29, 2024 09:21
@hebasto
Copy link
Owner Author

hebasto commented Jun 29, 2024

Rebased to resolve a conflict.

Copy link

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

utACK 458d54c as far as the secp256k1 config is concerned

@hebasto
Copy link
Owner Author

hebasto commented Jun 29, 2024

I've updated the PR descriptions with possible ways to test this PR.

@hebasto hebasto merged commit a7cc7d8 into cmake-staging Jun 30, 2024
38 checks passed
hebasto added a commit that referenced this pull request Jul 1, 2024
dc32026 cmake: Fix `clang-tidy` CI task (Hennadii Stepanov)

Pull request description:

  #192 introduced a regression in the "clang-tidy" CI task. For instance, see https://cirrus-ci.com/task/6601636905222144 from bitcoin#29790.

  The reason is that disabling `EXPORT_COMPILE_COMMANDS` for `secp256k1` target only is not enough because the subtree's build system has other intermediate build targets internally.

ACKs for top commit:
  TheCharlatan:
    ACK dc32026

Tree-SHA512: 134e7106c18abd7f07c15742edd73a16847f09452646ed4408d8cd35f7bca9f8501028f94b718720720c7ecc330a91dfd94680c091d010be88952ce4f4ef4c4e
@@ -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

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.

set(SECP256K1_BUILD_CTIME_TESTS OFF CACHE BOOL "" FORCE)
set(SECP256K1_BUILD_EXAMPLES OFF CACHE BOOL "" FORCE)
include(GetTargetInterface)
# -fsanitize and related flags apply to both C++ and C,
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.

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

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

hebasto added a commit that referenced this pull request Jul 2, 2024
d2ab006 cmake: Do not disable libsecp's constant time tests (Hennadii Stepanov)

Pull request description:

  This PR addresses #192 (comment):
  > Ok, but the ctime tests are currently compiled, and runnable there.

  and mirrors the behavior of the master branch.

  Assuming the Valgrind is installed, after building the user can tun:
  ```
  $ valgrind ./build/src/secp256k1/src/ctime_tests
  ```

ACKs for top commit:
  TheCharlatan:
    ACK d2ab006

Tree-SHA512: c4dd4967473fecb7297e1b60b02001a08bc4f302266512fcb76e8742e12305b2f3a4aa29a7fb72489fee68548199c9c87e5750ee4184b5eb64f8788083efeef3
hebasto added a commit that referenced this pull request Jul 3, 2024
… summaries

2995f0f cmake: Move `FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION` check (Hennadii Stepanov)
2296161 cmake: Use `TYPE` property to tell Qt libraries type (Hennadii Stepanov)
69afb91 cmake: Move Qt checks (Hennadii Stepanov)
9afe0c8 cmake: Fix `translate` target when `ENABLE_WALLET=OFF` (Hennadii Stepanov)

Pull request description:

  This PR addresses #192 (comment).

  Additionally, fixed a bug in the `translate` target for the `-DBUILD_GUI=ON -DENABLE_WALLET=OFF` configuration.

ACKs for top commit:
  m3dwards:
    ACK 2995f0f

Tree-SHA512: 03aad0a81077112ee26e04b6faa78abe0bde854faa706b22352f5dd8b0a1be65be5942fad985ec0277eca93227ef005e3a02712059858e6dd6e09ccc5c664e49
hebasto added a commit that referenced this pull request Jul 12, 2024
77876ca cmake: Move `CMAKE_C_FLAGS` deduplication after enabling C language (Hennadii Stepanov)
56d3878 cmake: Use C++ rather than C in `test-security-check` target (Hennadii Stepanov)
64dd87c contrib: c++ify test stubs after switching to c++ compilers (Cory Fields)
2ea2f53 contrib: rename cc to cxx in binary checking scripts (Cory Fields)
19307f5 contrib: use c++ rather than c for binary tests (Cory Fields)
439ceb0 guix: use glibc 2.31 (fanquake)

Pull request description:

  #192 introduced a regression in the Guix build. For instance, `env HOSTS=x86_64-w64-mingw32 ./contrib/guix/guix-build` for 1eb1250.

  The C language must be enabled _before_ any reference to `CMAKE_C_*` variables and _in the scope_ of those references (in case of non-cache variables). However, this is not the case on the staging branch at present for the following references: https://github.com/hebasto/bitcoin/blob/1eb125095fc5578d8faf7501f2cf023237b14d59/CMakeLists.txt#L573-L575 and https://github.com/hebasto/bitcoin/blob/1eb125095fc5578d8faf7501f2cf023237b14d59/cmake/module/Maintenance.cmake#L39-L46

  This PR fixes this issue by moving the `enable_language(C)` to the top of the root listfile.

Top commit has no ACKs.

Tree-SHA512: 246745f84973ee38a88fec5dc99887f4b8065bc837b7eb8fd0db44aaded7ea24ece3a2b54baf89df5f5a42a9206ca93690d54733946c9b01e345e4485103493e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants