cmake: Fix library ABI versioning #1270

hebasto · 2023-04-10T20:36:54Z

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

real-or-random · 2023-04-10T20:56:49Z

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

hebasto · 2023-04-10T21:04:37Z

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

On the master branch (3bab71c), simulate a release with backward-compatible API changes with the diff as follows:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index a70165e..8216bad 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -17,9 +17,9 @@ project(libsecp256k1 VERSION 0.3.2 LANGUAGES C)
 # https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html
 # All changes in experimental modules are treated as if they don't affect the
 # interface and therefore only increase the revision.
-set(${PROJECT_NAME}_LIB_VERSION_CURRENT 2)
-set(${PROJECT_NAME}_LIB_VERSION_REVISION 2)
-set(${PROJECT_NAME}_LIB_VERSION_AGE 0)
+set(${PROJECT_NAME}_LIB_VERSION_CURRENT 3)
+set(${PROJECT_NAME}_LIB_VERSION_REVISION 0)
+set(${PROJECT_NAME}_LIB_VERSION_AGE 1)
 
 set(CMAKE_C_STANDARD 90)
 set(CMAKE_C_EXTENSIONS OFF)
diff --git a/configure.ac b/configure.ac
index 0b555ea..acb923e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,9 +13,9 @@ define(_PKG_VERSION_IS_RELEASE, false)
 # https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html
 # All changes in experimental modules are treated as if they don't affect the
 # interface and therefore only increase the revision.
-define(_LIB_VERSION_CURRENT, 2)
-define(_LIB_VERSION_REVISION, 2)
-define(_LIB_VERSION_AGE, 0)
+define(_LIB_VERSION_CURRENT, 3)
+define(_LIB_VERSION_REVISION, 0)
+define(_LIB_VERSION_AGE, 1)
 
 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])

Build using Autotools:

$ ./autogen.sh && ./configure && make
$ ls -1 .libs/libsecp256k1.so.*
.libs/libsecp256k1.so.2
.libs/libsecp256k1.so.2.1.0

Build using CMake:

$ cmake -S . -B build && cmake --build build
$ ls -1 build/src/libsecp256k1.so.*
build/src/libsecp256k1.so.3
build/src/libsecp256k1.so.3.1.0

real-or-random · 2023-04-11T06:11:44Z

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

 This emulates libtool to make sure libtool and cmake agree on the ABI version,
 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

hebasto · 2023-04-11T16:08:01Z

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

hebasto · 2023-04-12T14:42:16Z

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:

Problem: cmake-built libzmq is not binary-compatible with autotools builds zeromq/libzmq#2887
macOS dylib 'current version' and 'compatibility version' Exiv2/exiv2#2020
Fix CMake shared library ABI version on Apple platforms libsndfile/libsndfile#743
CMake: Enforce library versioning compatible with GNU Libtool for macOS (fixes #55) libexpat/libexpat#582

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

hebasto · 2023-04-18T11:11:04Z

Rebased on top of the #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.

hebasto · 2023-04-18T11:31:10Z

Can you add a comment that says that this emulates libtool, something like,
 This emulates libtool to make sure libtool and cmake agree on the ABI version,
 see below "Calculate the version variables" in build-aux/ltmain.sh.

Thanks! Added.

real-or-random · 2023-04-19T14:33:36Z

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?

hebasto · 2023-04-21T16:17:05Z

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.

real-or-random · 2023-04-21T18:22:09Z

src/CMakeLists.txt

+  set_target_properties(secp256k1 PROPERTIES
+    VERSION ${${PROJECT_NAME}_soversion}.${${PROJECT_NAME}_LIB_VERSION_AGE}.${${PROJECT_NAME}_LIB_VERSION_REVISION}
+  )
+elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin")


Suggested change

elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin")

elseif(APPLE)

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

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?

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

Thanks! Updated.

hebasto · 2023-04-25T21:15:54Z

Updated c6fd426 -> 5a73030 (pr1270.04 -> pr1270.05, diff):

addressed @real-or-random's comment

theuni · 2023-04-27T11:57:52Z

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.

real-or-random

ACK 5a73030 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.

hebasto · 2023-04-28T18:09:39Z

@hebasto How confident are you about macOS and Windows?

Tested once more with the following diff:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 91d2bed..22706d3 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -17,9 +17,9 @@ project(libsecp256k1 VERSION 0.3.2 LANGUAGES C)
 # https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html
 # All changes in experimental modules are treated as if they don't affect the
 # interface and therefore only increase the revision.
-set(${PROJECT_NAME}_LIB_VERSION_CURRENT 2)
-set(${PROJECT_NAME}_LIB_VERSION_REVISION 2)
-set(${PROJECT_NAME}_LIB_VERSION_AGE 0)
+set(${PROJECT_NAME}_LIB_VERSION_CURRENT 1000)
+set(${PROJECT_NAME}_LIB_VERSION_REVISION 100)
+set(${PROJECT_NAME}_LIB_VERSION_AGE 10)
 
 set(CMAKE_C_STANDARD 90)
 set(CMAKE_C_EXTENSIONS OFF)
diff --git a/configure.ac b/configure.ac
index 0b555ea..08d9b64 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,9 +13,9 @@ define(_PKG_VERSION_IS_RELEASE, false)
 # https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html
 # All changes in experimental modules are treated as if they don't affect the
 # interface and therefore only increase the revision.
-define(_LIB_VERSION_CURRENT, 2)
-define(_LIB_VERSION_REVISION, 2)
-define(_LIB_VERSION_AGE, 0)
+define(_LIB_VERSION_CURRENT, 1000)
+define(_LIB_VERSION_REVISION, 100)
+define(_LIB_VERSION_AGE, 10)
 
 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])

On macOS:

Autotools:

% ./autogen.sh && ./configure --disable-static
% make
% ls -l .libs/*dylib*
-rwxr-xr-x  1 hebasto  staff  1361344 Apr 28 19:58 .libs/libsecp256k1.990.dylib
lrwxr-xr-x  1 hebasto  staff       22 Apr 28 19:58 .libs/libsecp256k1.dylib -> libsecp256k1.990.dylib
% otool -L .libs/libsecp256k1.dylib 
.libs/libsecp256k1.dylib:
 /usr/local/lib/libsecp256k1.990.dylib (compatibility version 1001.0.0, current version 1001.100.0)
 /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.0.0)

CMake:

% cmake -S . -B build
% cmake --build build
% ls -l build/src/*dylib*
-rwxr-xr-x  1 hebasto  staff  1361320 Apr 28 20:00 build/src/libsecp256k1.990.dylib
lrwxr-xr-x  1 hebasto  staff       22 Apr 28 20:00 build/src/libsecp256k1.dylib -> libsecp256k1.990.dylib
% otool -L build/src/libsecp256k1.dylib 
build/src/libsecp256k1.dylib:
 @rpath/libsecp256k1.990.dylib (compatibility version 1001.0.0, current version 1001.100.0)
 /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 :)

hebasto · 2023-04-28T19:14:42Z

Updated 5a73030 -> d2e4e18 (pr1270.05 -> pr1270.06, diff):

fixed the issue when cross compiling for Windows

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

Autotools:

$ ./autogen.sh && ./configure --host=x86_64-w64-mingw32 --disable-static
$ ./configure --disable-static --host=x86_64-w64-mingw32
$ make
$ ls -1 .libs/*dll*
.libs/libsecp256k1-990.dll
.libs/libsecp256k1.dll.a

CMake:

$ cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE=cmake/x86_64-w64-mingw32.toolchain.cmake
$ cmake --build build
$ ls -1 build/src/*dll*
build/src/libsecp256k1-990.dll
build/src/libsecp256k1.dll.a

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

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

hebasto · 2023-04-28T20:04:07Z

Updated d2e4e18 -> bef448f (pr1270.06 -> pr1270.07, diff):

fixed all Windows-related issues

@real-or-random

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.

real-or-random · 2023-04-29T15:22:37Z

src/CMakeLists.txt

+  set(${PROJECT_NAME}_windows "secp256k1")
+  if(MSVC)
+    set(${PROJECT_NAME}_windows "${PROJECT_NAME}")
+  endif()


For my understanding: Why does this need to be different on MSVC?

MSVC generator skips adding a "lib" prefix by default. See the item 2 in #1237 (comment) for additional details.

real-or-random

ACK bef448f diff looks good and I tested on Linux

hebasto · 2023-06-27T08:57:02Z

src/CMakeLists.txt

+  endif()
+  set_target_properties(secp256k1 PROPERTIES
+    ARCHIVE_OUTPUT_NAME "${${PROJECT_NAME}_windows}"
+    RUNTIME_OUTPUT_NAME "${${PROJECT_NAME}_windows}-${${PROJECT_NAME}_soversion}"


CMake 3.27+ has DLL_NAME_WITH_SOVERSION on the same purpose.

hebasto mentioned this pull request Apr 10, 2023

cmake: Fix ABI version for shared library and improve MSVC output artifacts naming #1237

Closed

hebasto force-pushed the 230410-soversion branch from 0efcf2a to 069f9e0 Compare April 18, 2023 11:08

hebasto changed the title ~~cmake: Fix ABI version for shared library~~ cmake: Fix library ABI version on Linux Apr 18, 2023

hebasto force-pushed the 230410-soversion branch from 069f9e0 to 026b0ed Compare April 18, 2023 11:24

hebasto force-pushed the 230410-soversion branch from 026b0ed to c6fd426 Compare April 21, 2023 16:13

hebasto changed the title ~~cmake: Fix library ABI version on Linux~~ cmake: Fix library ABI versioning Apr 21, 2023

real-or-random reviewed Apr 21, 2023

View reviewed changes

hebasto force-pushed the 230410-soversion branch from c6fd426 to 5a73030 Compare April 25, 2023 21:11

real-or-random approved these changes Apr 28, 2023

View reviewed changes

hebasto force-pushed the 230410-soversion branch from 5a73030 to d2e4e18 Compare April 28, 2023 19:04

cmake: Fix library ABI versioning

bef448f

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

hebasto force-pushed the 230410-soversion branch from d2e4e18 to bef448f Compare April 28, 2023 20:00

real-or-random reviewed Apr 29, 2023

View reviewed changes

real-or-random approved these changes May 3, 2023

View reviewed changes

real-or-random merged commit f30c748 into bitcoin-core:master May 3, 2023

hebasto deleted the 230410-soversion branch May 3, 2023 14:40

hebasto commented Jun 27, 2023

View reviewed changes

hebasto mentioned this pull request Jun 27, 2023

build: Meta-issue for follow-ups to initial CMake merge (#1113) #1235

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Fix library ABI versioning #1270

cmake: Fix library ABI versioning #1270

hebasto commented Apr 10, 2023 •

edited

Loading

real-or-random commented Apr 10, 2023

hebasto commented Apr 10, 2023 •

edited

Loading

real-or-random commented Apr 11, 2023

hebasto commented Apr 11, 2023

hebasto commented Apr 12, 2023 •

edited

Loading

hebasto commented Apr 18, 2023

hebasto commented Apr 18, 2023

real-or-random commented Apr 19, 2023

hebasto commented Apr 21, 2023

real-or-random Apr 21, 2023

hebasto Apr 21, 2023 •

edited

Loading

real-or-random Apr 25, 2023

hebasto Apr 25, 2023

hebasto commented Apr 25, 2023

theuni commented Apr 27, 2023

real-or-random left a comment

hebasto commented Apr 28, 2023 •

edited

Loading

hebasto commented Apr 28, 2023 •

edited

Loading

hebasto commented Apr 28, 2023

real-or-random Apr 29, 2023

hebasto Apr 29, 2023 •

edited

Loading

real-or-random left a comment •

edited

Loading

hebasto Jun 27, 2023

cmake: Fix library ABI versioning #1270

cmake: Fix library ABI versioning #1270

Conversation

hebasto commented Apr 10, 2023 • edited Loading

real-or-random commented Apr 10, 2023

hebasto commented Apr 10, 2023 • edited Loading

real-or-random commented Apr 11, 2023

hebasto commented Apr 11, 2023

hebasto commented Apr 12, 2023 • edited Loading

hebasto commented Apr 18, 2023

hebasto commented Apr 18, 2023

real-or-random commented Apr 19, 2023

hebasto commented Apr 21, 2023

real-or-random Apr 21, 2023

Choose a reason for hiding this comment

hebasto Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

APPLE

real-or-random Apr 25, 2023

Choose a reason for hiding this comment

hebasto Apr 25, 2023

Choose a reason for hiding this comment

hebasto commented Apr 25, 2023

theuni commented Apr 27, 2023

real-or-random left a comment

Choose a reason for hiding this comment

hebasto commented Apr 28, 2023 • edited Loading

hebasto commented Apr 28, 2023 • edited Loading

hebasto commented Apr 28, 2023

real-or-random Apr 29, 2023

Choose a reason for hiding this comment

hebasto Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

real-or-random left a comment • edited Loading

Choose a reason for hiding this comment

hebasto Jun 27, 2023

Choose a reason for hiding this comment

hebasto commented Apr 10, 2023 •

edited

Loading

hebasto commented Apr 10, 2023 •

edited

Loading

hebasto commented Apr 12, 2023 •

edited

Loading

hebasto Apr 21, 2023 •

edited

Loading

hebasto commented Apr 28, 2023 •

edited

Loading

hebasto commented Apr 28, 2023 •

edited

Loading

hebasto Apr 29, 2023 •

edited

Loading

real-or-random left a comment •

edited

Loading