Build: allow static or shared but not both #1230

theuni commented at 9:44 PM on March 8, 2023: contributor

Unfortunately it wasn't really possible to keep a clean diff here because of the nature of the change. I suggest reviewing the lib creation stuff in its entirety, sorry about that :\

Rather than allowing for shared and static libs to be built at the same time like autotools, this PR switches to the CMake convention of allowing only 1.

A new BUILD_SHARED_LIBS option is added to match CMake convention, as well as a SECP256K1_DISABLE_SHARED option which overrides it. That way even projects which have BUILD_SHARED_LIBS=1 can opt-into a static libsecp in particular.

Details:

Two object libraries are created: secp256k1_asm and secp256k1_precomputed_objs. Some tests/benchmarks use the object libraries directly, some link against the real lib: secp256k1.

Because the objs don't know what they're going to be linked into, they need to be told how to deal with PIC.

The DEFINE_SYMBOL property sets the DLL_EXPORT define as necessary (when building a shared lib)

theuni force-pushed on Mar 8, 2023

theuni commented at 10:19 PM on March 8, 2023: contributor

Hmm, this blows up when tested with CMake 3.1. Going to convert to draft for now until that works.

theuni marked this as a draft on Mar 8, 2023

theuni commented at 11:57 PM on March 8, 2023: contributor

I have this working locally but it's pretty ugly due to sacrifices made for such an ancient CMake version.

It seems like such a shame to introduce a new buildsystem that's already saddled with technical debt. What was the reasoning behind choosing compatibility for CMake 3.1. Is that up for discussion still?

hebasto commented at 1:06 AM on March 9, 2023: member

I have this working locally but it's pretty ugly due to sacrifices made for such an ancient CMake version.

It seems like such a shame to introduce a new buildsystem that's already saddled with technical debt. What was the reasoning behind choosing compatibility for CMake 3.1. Is that up for discussion still?

This project docs do not stipulate any minimum supported versions for compilers, which implicitly assumes that they can be old enough. In the same spirit, the minimum required CMake version was chosen as oldest as possible. Particularly, CMake 3.1 supports C_STANDARD property.

On the recent IRC discussion, it was established that we need at least CMake 3.11 to support SameMinorVersion.

Speaking of bumping minimum required CMake version, I'd prefer 3.13 which has improved syntax for object libraries and available on Debian 10.

theuni commented at 1:09 AM on March 9, 2023: contributor

Ok, great. I think 3.13 is what I needed for objects for my desired approach as well.

I'll go back to my non-ancient approach and push something up tomorrow.

real-or-random commented at 8:54 AM on March 9, 2023: contributor

It seems like such a shame to introduce a new buildsystem that's already saddled with technical debt.

I agree, and I'd totally be up to a newer version. We're not going to drop autotools soon, so there's always an alternative available on older systems. If someone really wants an ancient compiler, they probably anyway want autotools.

Speaking of bumping minimum required CMake version, I'd prefer 3.13 which has improved syntax for object libraries and available on Debian 10.

https://repology.org/project/cmake/versions is a good overview. Debian 10 is the oldest supported release, and they have 3.13. Ubuntu LTS 18.04 still has 3.10 but 18.04 is only in "Extended Security Maintenance" (see https://ubuntu.com/about/release-cycle)...

But I'd also be okay with a version newer than 3.13 if it unlocks more features that will really help. Again, we still have autotools for people on older distros.

CMake 3.11 would also unlock COMPILE_OPTIONS / add_compile_options (listed in #1224). Though, when I suggested it, I thought this could help get rid of the string modifications in the default CFLAGS. But no, it's just for adding more CFLAGS; I corrected this in #1224. (It's still nice...)

hebasto commented at 10:17 AM on March 9, 2023: member

I apologies if my question looks out of this PR scope, but what are actual drawbacks of using global set(CMAKE_POSITION_INDEPENDENT_CODE ON) nowadays? It restores PIC-consistency across all build artifacts while keeping ability to build dual shared/static libraries.

hebasto commented at 10:28 AM on March 9, 2023: member

Ok, great. I think 3.13 is what I needed for objects for my desired approach as well.

But I'd also be okay with a version newer than 3.13 if it unlocks more features that will really help. Again, we still have autotools for people on older distros.

The current WIP for implementing a CMake-based build system in Bitcoin Core contains:

cmake_minimum_required(VERSION 3.13...3.24)

which is based mostly on availability of CMake across current Linux distros, as macOS (via Homebrew) and Windows (via Visual Studio) provide quite recent versions.

theuni force-pushed on Mar 9, 2023

theuni marked this as ready for review on Mar 9, 2023

theuni commented at 7:49 PM on March 9, 2023: contributor

I apologies if my question looks out of this PR scope, but what are actual drawbacks of using global set(CMAKE_POSITION_INDEPENDENT_CODE ON) nowadays? It restores PIC-consistency across all build artifacts while keeping ability to build dual shared/static libraries.

It wouldn't be the end of the world, it's just a pretty clear sign to me that we're not defining our targets explicitly enough if we need to set a global and let it trickle down. Also, it is slower for architectures that care. This means that on some architectures and to some very small degree, libtool-built libsecp would be faster than cmake-built.

But besides, there are other reasons (like dllimport/export) to want to compile shared objects differently that static. So I don't see why we'd use a global hammer for all downstreams rather than just setting the correct properties.

Note that the CMake docs actually recommend this approach (very bottom). They use ${BUILD_SHARED_LIBS} as opposed to querying the property itself which is about the same thing I guess, but a bit less explicit imo.

For Core, however, sure. Let's set the global :)

hebasto cross-referenced this on Mar 10, 2023 from issue cmake: Set `POSITION_INDEPENDENT_CODE` property properly by hebasto

hebasto commented at 5:37 PM on March 10, 2023: member

@theuni

It seems like such a shame to introduce a new buildsystem that's already saddled with technical debt.

Ok, great. I think 3.13 is what I needed for objects for my desired approach as well. @real-or-random But I'd also be okay with a version newer than 3.13 if it unlocks more features that will really help. Again, we still have autotools for people on older distros.

I agree to bump the minimum required CMake version up to 3.13.

@fanquake Your opinion regarding the minimum required CMake version will be much appreciated.

hebasto commented at 6:03 PM on March 10, 2023: member

f96bd5184b6534de7c20efba1a35248d940bebb5

This PR changes the way how examples are linked. Aren't they supposed to be linked as if for actual users (with no using of internal objects)?

in CMakeLists.txt:32 in f96bd5184b outdated

  32 | -if(NOT SECP256K1_BUILD_SHARED AND NOT SECP256K1_BUILD_STATIC)
  33 | -  message(FATAL_ERROR "At least one of SECP256K1_BUILD_SHARED and SECP256K1_BUILD_STATIC must be enabled.")
  34 | +option(BUILD_SHARED_LIBS "Build shared libraries" ON)
  35 | +option(SECP256K1_DISABLE_SHARED "Disable shared library. Overrides BUILD_SHARED_LIBS." OFF)
  36 | +if (SECP256K1_DISABLE_SHARED)
  37 | +  set(BUILD_SHARED_LIBS FALSE)

hebasto commented at 6:08 PM on March 10, 2023:

nit: There is a convention (?) to use values "ON" and "OFF" for options.

  set(BUILD_SHARED_LIBS OFF)

in CMakeLists.txt:232 in f96bd5184b outdated

 228 | @@ -230,8 +229,7 @@ message("\n")
 229 |  message("secp256k1 configure summary")
 230 |  message("===========================")
 231 |  message("Build artifacts:")
 232 | -message("  shared library ...................... ${SECP256K1_BUILD_SHARED}")
 233 | -message("  static library ...................... ${SECP256K1_BUILD_STATIC}")
 234 | +message("  shared library ...................... ${BUILD_SHARED_LIBS}")

hebasto commented at 6:10 PM on March 10, 2023:

nit: When BUILD_SHARED_LIBS is FALSE/OFF, at first glance, it is not obvious that the build system is configured to build a static library.

hebasto commented at 6:16 PM on March 10, 2023: member

Concept ACK.

theuni commented at 6:21 PM on March 10, 2023: contributor

f96bd51

This PR changes the way how examples are linked. Aren't they supposed to be linked as if for actual users (with no using of internal objects)?

Ah thanks, that's very helpful context. This PR indeed changes to link everything against objects which I thought would be easier than dealing with tests linked against local shared libs, but that's a really good point that the api/abi really isn't getting tested this way.

I will re-work and see what breaks when linked against actual shared libs.

theuni commented at 6:26 PM on March 10, 2023: contributor

Just to reiterate: I threw this PR up for discussion. I wanted to see what the simplification would look like, but it's not my intention to be a bully about it.

I'm not wedded to it (though I do prefer it ;) and I agree there are still some outstanding questions (@hebasto has mentioned downstream packaging).

hebasto cross-referenced this on Mar 10, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by hebasto

theuni commented at 7:05 PM on March 10, 2023: contributor

I will re-work and see what breaks when linked against actual shared libs.

Huh, it just worked. Neat.

in examples/CMakeLists.txt:16 in c394a28b87 outdated

  19 | +  target_link_options(example INTERFACE /IGNORE:4217)
  20 |  endif()
  21 |  
  22 |  add_executable(ecdsa_example ecdsa.c)
  23 | -target_link_libraries(ecdsa_example example)
  24 | +target_link_libraries(ecdsa_example example secp256k1)

hebasto commented at 7:11 PM on March 10, 2023:

secp256k1 can be mentioned in the example INTERFACE library.

in src/CMakeLists.txt:26 in c394a28b87 outdated

  31 | +# Add objects explicitly rather than linking to the object libs to keep them
  32 | +# from being exported.
  33 | +add_library(secp256k1 $<TARGET_OBJECTS:secp_precomputed_objs> $<TARGET_OBJECTS:secp_common_objs>)
  34 | +
  35 | +target_compile_definitions(secp_interface INTERFACE
  36 | +  $<$<C_COMPILER_ID:MSVC>:_CRT_SECURE_NO_WARNINGS>

theuni commented at 7:16 PM on March 10, 2023:

@hebasto What is this btw? ~I don't see it in the autotools build~ git grep _CRT_SECURE_NO_WARNINGS comes up empty and I suspect I didn't handle it correctly here.

hebasto commented at 7:31 PM on March 10, 2023:

During working on #1113, at some point in the past, I had a "unsafe CRT Library function" warning when building on Windows with MSVC. However, I'm not able to reproduce it now.

hebasto commented at 7:34 PM on March 10, 2023:

FWIW, a similar suppression is used in Bitcoin Core:

      <PreprocessorDefinitions>_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING;ZMQ_STATIC;NOMINMAX;WIN32;HAVE_CONFIG_H;_CRT_SECURE_NO_WARNINGS;_SCL_SECURE_NO_WARNINGS;_CONSOLE;_WIN32_WINNT=0x0601;_WIN32_IE=0x0501;WIN32_LEAN_AND_MEAN;%(PreprocessorDefinitions)</PreprocessorDefinitions>

theuni commented at 7:55 PM on March 10, 2023:

I realize this is kinda unrelated here, but it snuck in in #1113 so we may as well address it.

Due to the nature of that suppression, I think we'd rather know about it and address it. I'm tempted to remove this as part of this PR and ask you to submit a new one to rea-add it if you're ever able to reproduce again. That way we can debate the merit/alternatives.

Thoughts on that?

hebasto commented at 7:57 PM on March 10, 2023:

I'm tempted to remove this as part of this PR...

Agree.

theuni commented at 8:53 PM on March 10, 2023: contributor

Looking at this again... now that we're (correctly) linking against the real shared lib rather than object files, I think this could be simplified even further to a simple helper static lib. I'm going to continue to work on this a bit.

in examples/CMakeLists.txt:14 in 1a7f4657c6 outdated

  16 | -    target_link_options(example INTERFACE /IGNORE:4217)
  17 | -  endif()
  18 | +if(NOT BUILD_SHARED_LIBS AND MSVC)
  19 | +  target_link_options(example INTERFACE /IGNORE:4217)
  20 |  endif()
  21 | +target_link_libraries(example INTERFACE secp256k1)

hebasto commented at 10:01 PM on March 10, 2023:

Maybe reuse target_link_libraries command:

--- a/examples/CMakeLists.txt
+++ b/examples/CMakeLists.txt
@@ -6,12 +6,12 @@ target_compile_options(example INTERFACE
   $<$<C_COMPILER_ID:MSVC>:/wd4005>
 )
 target_link_libraries(example INTERFACE
+  secp256k1
   $<$<PLATFORM_ID:Windows>:bcrypt>
 )
 if(NOT BUILD_SHARED_LIBS AND MSVC)
   target_link_options(example INTERFACE /IGNORE:4217)
 endif()
-target_link_libraries(example INTERFACE secp256k1)
 
 add_executable(ecdsa_example ecdsa.c)
 target_link_libraries(ecdsa_example example)

?

in src/CMakeLists.txt:15 in 1a7f4657c6 outdated

  22 | -add_library(secp256k1 SHARED EXCLUDE_FROM_ALL
  23 | -  secp256k1.c
  24 | -  ${internal_obj}
  25 | -)
  26 | -target_include_directories(secp256k1 INTERFACE
  27 | +add_library(secp_common_objs OBJECT EXCLUDE_FROM_ALL secp256k1.c)

hebasto commented at 10:11 PM on March 10, 2023:

Why is secp256k1.c the source of the secp_common_objs library, not the secp256k1 one?

theuni commented at 12:25 AM on March 11, 2023:

Yeah, I'm going to do away with common entirely as it's not needed now that I understand that we're linking benches/tests directly to shared libs. Will push up a cleaner version next week.

hebasto commented at 12:51 AM on March 11, 2023: member

@theuni

What do you think about splitting minimum required CMake version bumping into a separate PR, as it is a blocker for other issues?

hebasto cross-referenced this on Mar 11, 2023 from issue cmake: Fix ABI version for shared library and improve MSVC output artifacts naming by hebasto

theuni cross-referenced this on Mar 12, 2023 from issue build: bump CMake minimum requirement to 3.13 by theuni

theuni force-pushed on Mar 14, 2023

theuni commented at 9:29 PM on March 14, 2023: contributor

Updated, fixed nits, squashed, and rebased. Now tested with external arm asm which was broken but now works.

Should be ready for review.

in src/CMakeLists.txt:45 in f5d117d9df outdated

  91 | -  target_link_libraries(bench link_library)
  92 | -  add_executable(bench_internal bench_internal.c ${internal_obj})
  93 | -  add_executable(bench_ecmult bench_ecmult.c ${internal_obj})
  94 | +  target_link_libraries(bench secp256k1)
  95 | +  add_executable(bench_internal bench_internal.c)
  96 | +  target_link_libraries(bench_internal secp_precomputed_objs secp_asm)

sipa commented at 9:12 PM on March 15, 2023:

Would it make sense (if it's possible) to define a library/target/whatever that combines secp256k1_precomputed_objs and secp_asm?

Also naming... can we be more consistent than having secp256k1 in one and secp in the other?

hebasto commented at 2:01 PM on March 16, 2023:

Would it make sense (if it's possible) to define a library/target/whatever that combines secp256k1_precomputed_objs and secp_asm?

I was thinking about the same :) The diff as follows can help:

--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -10,14 +10,12 @@ add_library(secp_precomputed_objs OBJECT EXCLUDE_FROM_ALL
 # from being exported.
 add_library(secp256k1 secp256k1.c $<TARGET_OBJECTS:secp_precomputed_objs>)
 
-add_library(secp_asm INTERFACE)
 if(SECP256K1_ASM STREQUAL "arm")
   add_library(secp_asm_arm OBJECT EXCLUDE_FROM_ALL)
   target_sources(secp_asm_arm PUBLIC
     asm/field_10x26_arm.s
   )
   target_sources(secp256k1 PRIVATE $<TARGET_OBJECTS:secp_asm_arm>)
-  target_link_libraries(secp_asm INTERFACE secp_asm_arm)
 endif()
 
 #Define our export symbol only for Win32 and only for shared libs.
@@ -38,31 +36,36 @@ set_target_properties(secp256k1 PROPERTIES
   SOVERSION "${${PROJECT_NAME}_LIB_VERSION_CURRENT}"
 )
 
+add_library(secp256k1_internal INTERFACE)
+set_property(TARGET secp256k1_internal PROPERTY INTERFACE_SOURCES
+  $<TARGET_OBJECTS:secp_precomputed_objs>
+  $<$<TARGET_EXISTS:secp_asm_arm>:$<TARGET_OBJECTS:secp_asm_arm>>
+)
+
 if(SECP256K1_BUILD_BENCHMARK)
   add_executable(bench bench.c)
   target_link_libraries(bench secp256k1)
   add_executable(bench_internal bench_internal.c)
-  target_link_libraries(bench_internal secp_precomputed_objs secp_asm)
+  target_link_libraries(bench_internal secp256k1_internal)
   add_executable(bench_ecmult bench_ecmult.c)
-  target_link_libraries(bench_ecmult secp_precomputed_objs secp_asm)
+  target_link_libraries(bench_ecmult secp256k1_internal)
 endif()
 
 if(SECP256K1_BUILD_TESTS)
   add_executable(noverify_tests tests.c)
-  target_link_libraries(noverify_tests secp_precomputed_objs secp_asm)
+  target_link_libraries(noverify_tests secp256k1_internal)
   add_test(noverify_tests noverify_tests)
   if(NOT CMAKE_BUILD_TYPE STREQUAL "Coverage")
     add_executable(tests tests.c)
     target_compile_definitions(tests PRIVATE VERIFY)
-    target_link_libraries(tests secp_precomputed_objs secp_asm)
+    target_link_libraries(tests secp256k1_internal)
     add_test(tests tests)
   endif()
 endif()
 
 if(SECP256K1_BUILD_EXHAUSTIVE_TESTS)
   # Note: do not include secp_precomputed_objs in exhaustive_tests (it uses runtime-generated tables).
-  add_executable(exhaustive_tests tests_exhaustive.c)
-  target_link_libraries(exhaustive_tests secp_asm)
+  add_executable(exhaustive_tests tests_exhaustive.c $<$<TARGET_EXISTS:secp_asm_arm>:$<TARGET_OBJECTS:secp_asm_arm>>)
   target_compile_definitions(exhaustive_tests PRIVATE $<$<NOT:$<CONFIG:Coverage>>:VERIFY>)
   add_test(exhaustive_tests exhaustive_tests)
 endif()

theuni commented at 10:12 PM on March 16, 2023:

Would it make sense (if it's possible) to define a library/target/whatever that combines secp256k1_precomputed_objs and secp_asm?

I didn't combine them here because I wasn't sure if the combo of secp256k1_precomputed_objs and secp_asm made any logical sense worth defining, as opposed to just listing them explicitly where needed.

Happy to take a patch like @hebasto's above.

Also naming... can we be more consistent than having secp256k1 in one and secp in the other?

Roger.

hebasto commented at 8:52 AM on March 17, 2023:

I didn't combine them here because I wasn't sure if the combo of secp256k1_precomputed_objs and secp_asm made any logical sense worth defining, as opposed to just listing them explicitly where needed.

FWIW, the current Makefile.am does not use a combined entity.

theuni commented at 7:26 PM on March 21, 2023:

After looking at this again, Imo the complexity here isn't worth it and I'd rather not take this change unless anyone feels strongly about it.

in CMakeLists.txt:31 in f5d117d9df outdated

  31 | -option(SECP256K1_BUILD_STATIC "Build static library." ON)
  32 | -if(NOT SECP256K1_BUILD_SHARED AND NOT SECP256K1_BUILD_STATIC)
  33 | -  message(FATAL_ERROR "At least one of SECP256K1_BUILD_SHARED and SECP256K1_BUILD_STATIC must be enabled.")
  34 | +option(BUILD_SHARED_LIBS "Build shared libraries" ON)
  35 | +option(SECP256K1_DISABLE_SHARED "Disable shared library. Overrides BUILD_SHARED_LIBS." OFF)
  36 | +if (SECP256K1_DISABLE_SHARED)

hebasto commented at 2:02 PM on March 16, 2023:

style nit:

if(SECP256K1_DISABLE_SHARED)

in src/CMakeLists.txt:24 in f5d117d9df outdated

  29 | -  precomputed_ecmult.c
  30 | -  precomputed_ecmult_gen.c
  31 | -)
  32 | -set(internal_obj "$<TARGET_OBJECTS:precomputed>" "${common_obj}")
  33 | +#Define our export symbol only for Win32 and only for shared libs.
  34 | +if (WIN32)

hebasto commented at 2:02 PM on March 16, 2023:

style nit:

if(WIN32)

in src/CMakeLists.txt:16 in f5d117d9df outdated

  13 | +
  14 | +add_library(secp_asm INTERFACE)
  15 |  if(SECP256K1_ASM STREQUAL "arm")
  16 | -  add_library(common OBJECT
  17 | +  add_library(secp_asm_arm OBJECT EXCLUDE_FROM_ALL)
  18 | +  target_sources(secp_asm_arm PUBLIC

hebasto commented at 2:06 PM on March 16, 2023:

Why PUBLIC? If it was unintentional then adding of the target_sources seems unneeded, no?

theuni commented at 10:13 PM on March 16, 2023:

Otherwise it ends up missing as part of the interface and link fails. Is there some trick I'm missing?

hebasto commented at 8:49 AM on March 17, 2023:

Otherwise it ends up missing as part of the interface and link fails.

You're right. Sorry. Fwiw, it works with the patch suggested in #1230 (review) if it will be accepted.

hebasto commented at 2:06 PM on March 16, 2023: member

Approach ACK f5d117d9df400eb7cfa5fd0caf2af84f176d4a4e.

in src/CMakeLists.txt:17 in f5d117d9df outdated

  14 | +add_library(secp_asm INTERFACE)
  15 |  if(SECP256K1_ASM STREQUAL "arm")
  16 | -  add_library(common OBJECT
  17 | +  add_library(secp_asm_arm OBJECT EXCLUDE_FROM_ALL)
  18 | +  target_sources(secp_asm_arm PUBLIC
  19 |      asm/field_10x26_arm.s

hebasto commented at 11:13 AM on March 17, 2023:

Unrelated to this PR, just a note (for myself).

It will be more CMake-ish to move the -DUSE_ASM_X86_64=1 definition from globals to a usage requirement of a library which compiles this source file.

in src/CMakeLists.txt:10 in f5d117d9df outdated

   6 | +  precomputed_ecmult.c
   7 | +  precomputed_ecmult_gen.c
   8 | +)
   9 | +
  10 | +# Add objects explicitly rather than linking to the object libs to keep them
  11 | +# from being exported.

hebasto commented at 11:27 AM on March 17, 2023:

With what setup (platform / build configuration / ...) can I observe this difference?

theuni commented at 3:57 PM on March 17, 2023:

If you link against it, CMake will complain that it's not added to the export list. If you then add it to that list, it gets exported as an interface library in the installed libsecp256k1-*.cmake files. I'm unsure of the consequences of that, but because it's so easily avoidable I decided to just avoid finding out. If nothing else it feels a little icky to expose our internal structure when it's not necessary.

I can change it if there's any reason to though.

hebasto commented at 5:43 PM on March 17, 2023:

If you link against it, CMake will complain that it's not added to the export list.

Right. I can reproduced it with -DBUILD_SHARED_LIBS=OFF.

But the issue is called "Exporting shared library that depends on object library fails" :)

theuni commented at 5:46 PM on March 17, 2023:

Ah yes sorry, forgot to mention that requirement.

real-or-random referenced this in commit 9c8c4f443c on Mar 21, 2023

build: remove warning until it's reproducible

Also remove the interface it was attached to since it's no longer needed.
This removal simplifies the next commit.

36b0adf1b9

theuni force-pushed on Mar 21, 2023

hebasto approved

hebasto commented at 6:32 PM on March 21, 2023: member

ACK 71648cf19fdd986e005327736bc7b0b5439fe67a, tested on Ubuntu 22.04 (both shared and static libs).

Although some style nits still remain unresolved:

naming consistency
spaces (here and here)

theuni commented at 7:34 PM on March 21, 2023: contributor

Thanks, nits addressed. Will squash after deciding on #1230 (review) .

hebasto commented at 8:20 PM on March 21, 2023: member

... after deciding on #1230 (comment) .

I'm OK with the current state for a reason mentioned here.

theuni force-pushed on Mar 21, 2023

theuni commented at 8:41 PM on March 21, 2023: contributor

I'm OK with the current state for a reason mentioned here.

Thanks! Squashed.

hebasto approved

hebasto commented at 9:50 AM on March 22, 2023: member

ACK 79fed0ca8c0a2f0f073c3735f6c5e50b23c7ef5a, tested on Ubuntu 22.04 and Windows 11 22H2 (using VS Build Tools).

MatthewLM cross-referenced this on Apr 11, 2023 from issue Is CMake 3.13 absolutely required right now? by MatthewLM

theuni commented at 4:01 PM on April 12, 2023: contributor

Ping @real-or-random . Bumping for visibility in parallel to #1272.

If we're going to have a bumped CMake requirement, we may as well take advantage :)

real-or-random commented at 7:20 AM on April 13, 2023: contributor

I'll have a look next week!

in src/CMakeLists.txt:4 in 79fed0ca8c outdated

   0 | @@ -1,107 +1,78 @@
   1 |  # Must be included before CMAKE_INSTALL_INCLUDEDIR is used.
   2 |  include(GNUInstallDirs)
   3 | -set(${PROJECT_NAME}_installables "")
   4 |  
   5 | +add_library(secp256k1_precomputed_objs OBJECT EXCLUDE_FROM_ALL

real-or-random commented at 2:14 PM on April 17, 2023:

This would be a bit more consistent with autotools:

add_library(secp256k1_precomputed OBJECT EXCLUDE_FROM_ALL

Can you elaborate on EXCLUDE_FROM_ALL? Why is this desirable here?

theuni commented at 3:46 PM on April 17, 2023:

I just don't see it necessary to add the helper libs to all. As far as I can tell, it does the reasonable thing of rebuilding as needed.

in src/CMakeLists.txt:23 in 79fed0ca8c outdated

  28 | -add_library(precomputed OBJECT
  29 | -  precomputed_ecmult.c
  30 | -  precomputed_ecmult_gen.c
  31 | -)
  32 | -set(internal_obj "$<TARGET_OBJECTS:precomputed>" "${common_obj}")
  33 | +#Define our export symbol only for Win32 and only for shared libs.

real-or-random commented at 2:18 PM on April 17, 2023:

# Define our export symbol only for Win32 and only for shared libs.

Can you note that DLL_EXPORT is a libtool thing?

real-or-random commented at 2:36 PM on April 17, 2023: contributor

ACK mod nits

build: allow static or shared but not both ef49a11d29

theuni force-pushed on Apr 17, 2023

theuni commented at 4:32 PM on April 17, 2023: contributor

Addressed @real-or-random's feedback:

Renamed secp256k1_precomputed_objs to secp256k1_precomputed
Added comment about libtool

See diff with git diff 79fed0ca8c..ef49a11d296

hebasto approved

hebasto commented at 4:33 PM on April 17, 2023: member

re-ACK ef49a11d29601e09e94134975c968e92c0214102, only suggested changes since my recent review.

real-or-random approved

real-or-random commented at 10:29 AM on April 18, 2023: contributor

ACK ef49a11d29601e09e94134975c968e92c0214102

real-or-random merged this on Apr 18, 2023

real-or-random closed this on Apr 18, 2023

real-or-random cross-referenced this on Apr 18, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by real-or-random

hebasto cross-referenced this on Apr 18, 2023 from issue cmake: Fix library ABI versioning by hebasto

sipa referenced this in commit b4eb644b6c on May 12, 2023

hebasto referenced this in commit 49c52ea2b1 on May 13, 2023

real-or-random cross-referenced this on May 23, 2023 from issue Refine release process by jonasnick

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

sipa referenced this in commit 901336eee7 on Jun 21, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023

jonasnick cross-referenced this on Jul 24, 2023 from issue Upstream PRs 1268, 1276, 1267, 1265, 1230, 1279, 1273, 1263, 1231, 1285, 1283, 1205, 1286, 1275, 1234, 1239, 1240, 1284, 1277, 1289, 1270, 1296, 1301, 1299, 1066, 1300, 1292, 1305, 1303, 1133, 1306, 1207, 1304, 1307, 1311, 1309, 1312 by jonasnick