cmake: Fix library ABI versioning #1270

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:230410-soversion changing 1 files +36 −2
  1. hebasto commented at 8:36 pm on April 10, 2023: member

    This change emulates Libtool to make sure Libtool and CMake agree on the ABI version.

    To test, one needs to simulate a release with backward-compatible API changes, which means the following changes in configure.ac and CMakeLists.txt:

    • incrementing of *_LIB_VERSION_CURRENT
    • setting *_LIB_VERSION_REVISION to zero
    • incrementing of *_LIB_VERSION_AGE
  2. hebasto cross-referenced this on Apr 10, 2023 from issue cmake: Fix ABI version for shared library and improve MSVC output artifacts naming by hebasto
  3. real-or-random commented at 8:56 pm on April 10, 2023: contributor
    Sorry, I lost context here a bit. Can you explain again what the issue is and how it’s fixed?
  4. hebasto commented at 9:04 pm on April 10, 2023: member

    Sorry, I lost context here a bit. Can you explain again what the issue is and how it’s fixed?

    1. On the master branch (3bab71cf0534fd5948386de2b6ac31a7044ad76b), simulate a release with backward-compatible API changes with the diff as follows:
     0diff --git a/CMakeLists.txt b/CMakeLists.txt
     1index a70165e..8216bad 100644
     2--- a/CMakeLists.txt
     3+++ b/CMakeLists.txt
     4@@ -17,9 +17,9 @@ project(libsecp256k1 VERSION 0.3.2 LANGUAGES C)
     5 # https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html
     6 # All changes in experimental modules are treated as if they don't affect the
     7 # interface and therefore only increase the revision.
     8-set(${PROJECT_NAME}_LIB_VERSION_CURRENT 2)
     9-set(${PROJECT_NAME}_LIB_VERSION_REVISION 2)
    10-set(${PROJECT_NAME}_LIB_VERSION_AGE 0)
    11+set(${PROJECT_NAME}_LIB_VERSION_CURRENT 3)
    12+set(${PROJECT_NAME}_LIB_VERSION_REVISION 0)
    13+set(${PROJECT_NAME}_LIB_VERSION_AGE 1)
    14 
    15 set(CMAKE_C_STANDARD 90)
    16 set(CMAKE_C_EXTENSIONS OFF)
    17diff --git a/configure.ac b/configure.ac
    18index 0b555ea..acb923e 100644
    19--- a/configure.ac
    20+++ b/configure.ac
    21@@ -13,9 +13,9 @@ define(_PKG_VERSION_IS_RELEASE, false)
    22 # https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html
    23 # All changes in experimental modules are treated as if they don't affect the
    24 # interface and therefore only increase the revision.
    25-define(_LIB_VERSION_CURRENT, 2)
    26-define(_LIB_VERSION_REVISION, 2)
    27-define(_LIB_VERSION_AGE, 0)
    28+define(_LIB_VERSION_CURRENT, 3)
    29+define(_LIB_VERSION_REVISION, 0)
    30+define(_LIB_VERSION_AGE, 1)
    31 
    32 AC_INIT([libsecp256k1],m4_join([.], _PKG_VERSION_MAJOR, _PKG_VERSION_MINOR, _PKG_VERSION_PATCH)m4_if(_PKG_VERSION_IS_RELEASE, [true], [], [-dev]),[https://github.com/bitcoin-core/secp256k1/issues],[libsecp256k1],[https://github.com/bitcoin-core/secp256k1])
    33 
    
    1. Build using Autotools:
    0$ ./autogen.sh && ./configure && make
    1$ ls -1 .libs/libsecp256k1.so.*
    2.libs/libsecp256k1.so.2
    3.libs/libsecp256k1.so.2.1.0
    
    1. Build using CMake:
    0$ cmake -S . -B build && cmake --build build
    1$ ls -1 build/src/libsecp256k1.so.*
    2build/src/libsecp256k1.so.3
    3build/src/libsecp256k1.so.3.1.0
    
  5. real-or-random commented at 6:11 am on April 11, 2023: contributor

    Can you add a comment that says that this emulates libtool, something like,

    0 This emulates libtool to make sure libtool and cmake agree on the ABI version,
    1 see below "Calculate the version variables" in build-aux/ltmain.sh.
    

    (quicklink here for any readers here who don’t want to look it up: https://github.com/autotools-mirror/libtool/blob/1ec8fa28dcb29500d485c136db28315671ec4c3b/build-aux/ltmain.in#L7002 )

    The problem I see here is that libtool’s logic depends on the OS… :/

    So, I think the changes here correctly emulate libtools on Linux. Have you checked this against libtool on MacOS too (including the -current_version and -compatibility_version passed to the linker)? Do we care about Windows? I guess there’s anyway no fixed versioning scheme on Windows, so it probably won’t matter.

    Related: https://cmake.org/cmake/help/latest/prop_tgt/SOVERSION.html

  6. hebasto commented at 4:08 pm on April 11, 2023: member

    Have you checked this against libtool on MacOS too (including the -current_version and -compatibility_version passed to the linker)?

    Checked. It doesn’t look right. Investigating…

    Do we care about Windows?

    I’ve noticed this issue while working on Windows stuff :)

  7. hebasto commented at 2:42 pm on April 12, 2023: member

    Have you checked this against libtool on MacOS too (including the -current_version and -compatibility_version passed to the linker)?

    Checked. It doesn’t look right. Investigating…

    To make versioning 100%-compatible with Libtool’s scheme for macOS, MACHO_COMPATIBILITY_VERSION and MACHO_CURRENT_VERSION are required, but they are available in CMake 3.17+ only.

    UPD. Here are similar issues in other projects:

    Also: https://gitlab.kitware.com/cmake/cmake/-/issues/17652

  8. hebasto force-pushed on Apr 18, 2023
  9. hebasto renamed this:
    cmake: Fix ABI version for shared library
    cmake: Fix library ABI version on Linux
    on Apr 18, 2023
  10. hebasto commented at 11:11 am on April 18, 2023: member

    Rebased on top of the bitcoin-core/secp256k1#1230.

    The problem I see here is that libtool’s logic depends on the OS… :/

    I’ve limited this PR to Linux platform explicitly, leaving other platforms discussion for follow ups.

  11. hebasto force-pushed on Apr 18, 2023
  12. hebasto commented at 11:31 am on April 18, 2023: member

    Can you add a comment that says that this emulates libtool, something like,

    0 This emulates libtool to make sure libtool and cmake agree on the ABI version,
    1 see below "Calculate the version variables" in build-aux/ltmain.sh.
    

    Thanks! Added.

  13. real-or-random commented at 2:33 pm on April 19, 2023: contributor

    I’ve limited this PR to Linux platform explicitly, leaving other platforms discussion for follow ups.

    Concept ACK, but I fail to see how the current diff affects Linux only

    Given that you looked into this, what would you recommend for other problems? I think we could make it work on CMake 3.17+ and perhaps output a warning on lower versions (or document it somewhere else)? What about Windows?

  14. hebasto force-pushed on Apr 21, 2023
  15. hebasto renamed this:
    cmake: Fix library ABI version on Linux
    cmake: Fix library ABI versioning
    on Apr 21, 2023
  16. hebasto commented at 4:17 pm on April 21, 2023: member

    I’ve limited this PR to Linux platform explicitly, leaving other platforms discussion for follow ups.

    Concept ACK, but I fail to see how the current diff affects Linux only

    Given that you looked into this, what would you recommend for other problems? I think we could make it work on CMake 3.17+ and perhaps output a warning on lower versions (or document it somewhere else)? What about Windows?

    Reworked to be applied for all supported target platforms.

    Added a warning if the user is going to build macOS DYLIB using CMake < 3.17.

  17. in src/CMakeLists.txt:48 in c6fd4263af outdated
    45 )
    46+if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
    47+  set_target_properties(secp256k1 PROPERTIES
    48+    VERSION ${${PROJECT_NAME}_soversion}.${${PROJECT_NAME}_LIB_VERSION_AGE}.${${PROJECT_NAME}_LIB_VERSION_REVISION}
    49+  )
    50+elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
    


    real-or-random commented at 6:22 pm on April 21, 2023:
    0elseif(APPLE)
    

    Would this make a difference? (If not, I’m not sure if it’s better ^^.)


    hebasto commented at 6:35 pm on April 21, 2023:

    As @theuni pointed:

    From CMake docs:

    APPLE

    Set to True when the target system is an Apple platform (macOS, iOS, tvOS or watchOS).

    APPLE != OSX. Can we narrow this down some?


    real-or-random commented at 3:26 pm on April 25, 2023:
    Since MACHO_COMPATIBILITY_VERSION applies to all Apple platforms, I think that would actually be more correct in our case. (I guess in Core it doesn’t matter that much, but for libsecp256k1 it’s totally reasonable to build for embedded platforms.)

    hebasto commented at 9:16 pm on April 25, 2023:
    Thanks! Updated.
  18. hebasto force-pushed on Apr 25, 2023
  19. hebasto commented at 9:15 pm on April 25, 2023: member

    Updated c6fd4263af266fd9ec52d6dda90d66e55ce79871 -> 5a730304c55d77f377072993f9c76688a9f19246 (pr1270.04 -> pr1270.05, diff):

  20. theuni commented at 11:57 am on April 27, 2023: contributor

    Concept ACK.

    It’s frustrating that they differ. Also, note that this is a case where a user gets worse binaries with older CMake, but I’m not sure we can do anything about that.

  21. real-or-random approved
  22. real-or-random commented at 2:19 pm on April 28, 2023: contributor
    ACK https://github.com/bitcoin-core/secp256k1/pull/1270/commits/5a730304c55d77f377072993f9c76688a9f19246 diff looks good and I tested on Linux @hebasto How confident are you about macOS and Windows? If you’ve tested on these platforms, I think this is ready for merge.
  23. hebasto commented at 6:09 pm on April 28, 2023: member

    @hebasto How confident are you about macOS and Windows?

    Tested once more with the following diff:

     0diff --git a/CMakeLists.txt b/CMakeLists.txt
     1index 91d2bed..22706d3 100644
     2--- a/CMakeLists.txt
     3+++ b/CMakeLists.txt
     4@@ -17,9 +17,9 @@ project(libsecp256k1 VERSION 0.3.2 LANGUAGES C)
     5 # https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html
     6 # All changes in experimental modules are treated as if they don't affect the
     7 # interface and therefore only increase the revision.
     8-set(${PROJECT_NAME}_LIB_VERSION_CURRENT 2)
     9-set(${PROJECT_NAME}_LIB_VERSION_REVISION 2)
    10-set(${PROJECT_NAME}_LIB_VERSION_AGE 0)
    11+set(${PROJECT_NAME}_LIB_VERSION_CURRENT 1000)
    12+set(${PROJECT_NAME}_LIB_VERSION_REVISION 100)
    13+set(${PROJECT_NAME}_LIB_VERSION_AGE 10)
    14 
    15 set(CMAKE_C_STANDARD 90)
    16 set(CMAKE_C_EXTENSIONS OFF)
    17diff --git a/configure.ac b/configure.ac
    18index 0b555ea..08d9b64 100644
    19--- a/configure.ac
    20+++ b/configure.ac
    21@@ -13,9 +13,9 @@ define(_PKG_VERSION_IS_RELEASE, false)
    22 # https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html
    23 # All changes in experimental modules are treated as if they don't affect the
    24 # interface and therefore only increase the revision.
    25-define(_LIB_VERSION_CURRENT, 2)
    26-define(_LIB_VERSION_REVISION, 2)
    27-define(_LIB_VERSION_AGE, 0)
    28+define(_LIB_VERSION_CURRENT, 1000)
    29+define(_LIB_VERSION_REVISION, 100)
    30+define(_LIB_VERSION_AGE, 10)
    31 
    32 AC_INIT([libsecp256k1],m4_join([.], _PKG_VERSION_MAJOR, _PKG_VERSION_MINOR, _PKG_VERSION_PATCH)m4_if(_PKG_VERSION_IS_RELEASE, [true], [], [-dev]),[https://github.com/bitcoin-core/secp256k1/issues],[libsecp256k1],[https://github.com/bitcoin-core/secp256k1])
    33 
    

    On macOS:

    • Autotools:
    0% ./autogen.sh && ./configure --disable-static
    1% make
    2% ls -l .libs/*dylib*
    3-rwxr-xr-x  1 hebasto  staff  1361344 Apr 28 19:58 .libs/libsecp256k1.990.dylib
    4lrwxr-xr-x  1 hebasto  staff       22 Apr 28 19:58 .libs/libsecp256k1.dylib -> libsecp256k1.990.dylib
    5% otool -L .libs/libsecp256k1.dylib 
    6.libs/libsecp256k1.dylib:
    7 /usr/local/lib/libsecp256k1.990.dylib (compatibility version 1001.0.0, current version 1001.100.0)
    8 /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.0.0)
    
    • CMake:
    0% cmake -S . -B build
    1% cmake --build build
    2% ls -l build/src/*dylib*
    3-rwxr-xr-x  1 hebasto  staff  1361320 Apr 28 20:00 build/src/libsecp256k1.990.dylib
    4lrwxr-xr-x  1 hebasto  staff       22 Apr 28 20:00 build/src/libsecp256k1.dylib -> libsecp256k1.990.dylib
    5% otool -L build/src/libsecp256k1.dylib 
    6build/src/libsecp256k1.dylib: [@rpath](/bitcoin-core-secp256k1/contributor/rpath/)/libsecp256k1.990.dylib (compatibility version 1001.0.0, current version 1001.100.0)
    7 /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.0.0)
    

    A minor issue has been noted when cross-building for Windows. Going to address it shortly. @real-or-random

    I think this is ready for merge.

    Let’s fix and test the issue for Windows first :)

  24. hebasto force-pushed on Apr 28, 2023
  25. hebasto commented at 7:14 pm on April 28, 2023: member

    Updated 5a730304c55d77f377072993f9c76688a9f19246 -> d2e4e18a1ab7761282a24e90b1a77d10af3c2de8 (pr1270.05 -> pr1270.06, diff):

    • fixed the issue when cross compiling for Windows

    On Ubuntu 23.04 (with the diff from #1270 (comment)):

    • Autotools:
    0$ ./autogen.sh && ./configure --host=x86_64-w64-mingw32 --disable-static
    1$ ./configure --disable-static --host=x86_64-w64-mingw32
    2$ make
    3$ ls -1 .libs/*dll*
    4.libs/libsecp256k1-990.dll
    5.libs/libsecp256k1.dll.a
    
    • CMake:
    0$ cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE=cmake/x86_64-w64-mingw32.toolchain.cmake
    1$ cmake --build build
    2$ ls -1 build/src/*dll*
    3build/src/libsecp256k1-990.dll
    4build/src/libsecp256k1.dll.a
    

    ~Not able to test native builds on Windows right now…~

  26. cmake: Fix library ABI versioning
    This change emulates Libtool to make sure Libtool and CMake agree on the
    ABI version.
    bef448f9af
  27. hebasto force-pushed on Apr 28, 2023
  28. hebasto commented at 8:04 pm on April 28, 2023: member

    Updated d2e4e18a1ab7761282a24e90b1a77d10af3c2de8 -> bef448f9af248dba016883401de07b431f3e686e (pr1270.06 -> pr1270.07, diff):

    How confident are you about macOS and Windows?

    100%

    If you’ve tested on these platforms, I think this is ready for merge.

    I’ve tested cross-compiling for Windows and native builds on macOS and on Windows.

  29. in src/CMakeLists.txt:67 in bef448f9af
    64+  endif()
    65+elseif(CMAKE_SYSTEM_NAME STREQUAL "Windows")
    66+  set(${PROJECT_NAME}_windows "secp256k1")
    67+  if(MSVC)
    68+    set(${PROJECT_NAME}_windows "${PROJECT_NAME}")
    69+  endif()
    


    real-or-random commented at 3:22 pm on April 29, 2023:
    For my understanding: Why does this need to be different on MSVC?

    hebasto commented at 3:38 pm on April 29, 2023:
    MSVC generator skips adding a “lib” prefix by default. See the item 2 in #1237#issue-1619808558 for additional details.
  30. real-or-random approved
  31. real-or-random commented at 1:58 pm on May 3, 2023: contributor
    ACK bef448f9af248dba016883401de07b431f3e686e diff looks good and I tested on Linux
  32. real-or-random merged this on May 3, 2023
  33. real-or-random closed this on May 3, 2023

  34. hebasto deleted the branch on May 3, 2023
  35. sipa referenced this in commit b4eb644b6c on May 12, 2023
  36. hebasto referenced this in commit 49c52ea2b1 on May 13, 2023
  37. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  38. sipa referenced this in commit 901336eee7 on Jun 21, 2023
  39. in src/CMakeLists.txt:70 in bef448f9af
    67+  if(MSVC)
    68+    set(${PROJECT_NAME}_windows "${PROJECT_NAME}")
    69+  endif()
    70+  set_target_properties(secp256k1 PROPERTIES
    71+    ARCHIVE_OUTPUT_NAME "${${PROJECT_NAME}_windows}"
    72+    RUNTIME_OUTPUT_NAME "${${PROJECT_NAME}_windows}-${${PROJECT_NAME}_soversion}"
    


    hebasto commented at 8:57 am on June 27, 2023:
    CMake 3.27+ has DLL_NAME_WITH_SOVERSION on the same purpose.
  40. hebasto cross-referenced this on Jun 27, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by hebasto
  41. vmta referenced this in commit 8f03457eed on Jul 1, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 16:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me