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: Fix ABI version for shared library and improve MSVC output artifacts naming #1237

Closed
wants to merge 3 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Mar 11, 2023

  1. Fixed ABI version for a shared library.

To test, one needs to simulate a release with backward-compatible API changes, which means:

  • increment of LIB_VERSION_CURRENT
  • setting LIB_VERSION_REVISION to zero
  • increment of LIB_VERSION_AGE

  1. When using Visual Studio generator, the "lib" prefix is not added automatically.

Also added ABI-version suffix for a shared library (not bothering about a static library, as it already has the "_static" suffix to avoid name collision with DLL import library; should be revisited after #1230).

On master (5658209):

  • shared library: secp256k1.dll and secp256k1.lib
  • static library: secp256k1_static.lib

With this PR:

  • shared library: libsecp256k1-2.dll and libsecp256k1-2.lib
  • static library: libsecp256k1_static.lib

@real-or-random
Copy link
Contributor

Concept ACK, also based on the fact that autotools+mingw adds the lib prefix:

With ./configure --enable-shared --enable-static --enable-dev-mode --host=x86_64-w64-mingw32, I get this in .libs:

-rwxr-xr-x 1 1740850 Mar 11 14:58 bench_ecmult.exe*
-rw-r--r-- 1    5990 Mar 11 14:58 bench_ecmult_ltshwrapper
-rwxr-xr-x 1  183064 Mar 11 14:58 bench.exe*
-rwxr-xr-x 1 1728983 Mar 11 14:58 bench_internal.exe*
-rw-r--r-- 1    6010 Mar 11 14:58 bench_internal_ltshwrapper
-rw-r--r-- 1    6360 Mar 11 14:58 bench_ltshwrapper
-rwxr-xr-x 1 1622008 Mar 11 14:58 libsecp256k1-2.dll*
-rw-r--r-- 1 1620676 Mar 11 14:58 libsecp256k1.a
-rw-r--r-- 1   50552 Mar 11 14:58 libsecp256k1.dll.a
lrwxrwxrwx 1      18 Mar 11 14:58 libsecp256k1.la -> ../libsecp256k1.la
-rw-r--r-- 1     936 Mar 11 14:58 libsecp256k1.lai
-rw-r--r-- 1 1120192 Mar 11 14:58 libsecp256k1_precomputed.a
lrwxrwxrwx 1      30 Mar 11 14:58 libsecp256k1_precomputed.la -> ../libsecp256k1_precomputed.la
-rw-r--r-- 1   32675 Mar 11 14:58 lt-bench.c
-rw-r--r-- 1   32021 Mar 11 14:58 lt-bench_ecmult.c
-rw-r--r-- 1   32049 Mar 11 14:58 lt-bench_internal.c

Do we also want the -2 ABI version suffix?


By the way, autotools+MSVC doesn't add the prefix... But that's a somewhat strange combination that probably noone uses, except our CI.
With ./configure --enable-shared --enable-static --enable-dev-mode --host=x86_64-w64-mingw32 CC=/opt/msvc/bin/x64/cl CFLAGS="-nologo -diagnostics:caret" LDFLAGS="-XCClinker -nologo -XCClinker -diagnostics:caret" NM="/opt/msvc/bin/x64/dumpbin -symbols -headers" AR="/opt/msvc/bin/x64/lib", I get this in .libs:

-rwxr-xr-x 1 1330176 Mar 11 15:05 bench_ecmult.exe*
-rw-r--r-- 1    5990 Mar 11 15:05 bench_ecmult_ltshwrapper
-rwxr-xr-x 1  157184 Mar 11 15:05 bench.exe*
-rwxr-xr-x 1 1320960 Mar 11 15:05 bench_internal.exe*
-rw-r--r-- 1    6010 Mar 11 15:05 bench_internal_ltshwrapper
-rw-r--r-- 1    6360 Mar 11 15:05 bench_ltshwrapper
lrwxrwxrwx 1      18 Mar 11 15:05 libsecp256k1.la -> ../libsecp256k1.la
-rw-r--r-- 1     928 Mar 11 15:05 libsecp256k1.lai
lrwxrwxrwx 1      30 Mar 11 15:05 libsecp256k1_precomputed.la -> ../libsecp256k1_precomputed.la
-rw-r--r-- 1   32675 Mar 11 15:05 lt-bench.c
-rw-r--r-- 1   32021 Mar 11 15:05 lt-bench_ecmult.c
-rw-r--r-- 1   32049 Mar 11 15:05 lt-bench_internal.c
-rw-r--r-- 1 1333760 Mar 11 15:05 secp256k1-2.dll
-rw-r--r-- 1    3180 Mar 11 15:05 secp256k1-2.dll.exp
-rw-r--r-- 1   12123 Mar 11 15:05 secp256k1.dll.exp
-rw-r--r-- 1   20200 Mar 11 15:05 secp256k1.dll.lib
-rw-r--r-- 1    2214 Mar 11 15:05 secp256k1.exp
-rw-r--r-- 1 1363666 Mar 11 15:05 secp256k1.lib
-rw-r--r-- 1 1123338 Mar 11 15:05 secp256k1_precomputed.lib

@hebasto
Copy link
Member Author

hebasto commented Mar 11, 2023

Do we also want the -2 ABI version suffix?

Incidentally, I'm thinking about the same at this moment :)

@hebasto
Copy link
Member Author

hebasto commented Mar 11, 2023

Updated ff0a509 -> e2e0f47 (pr1237.01 -> pr1237.02, diff):

Do we also want the -2 ABI version suffix?

@sipa
Copy link
Contributor

sipa commented Mar 11, 2023

Concept ACK

@hebasto hebasto changed the title cmake: Fix MSVC output artifacts naming cmake: Fix API version for shared library and improve MSVC output artifacts naming Mar 13, 2023
@hebasto hebasto changed the title cmake: Fix API version for shared library and improve MSVC output artifacts naming cmake: Fix ABI version for shared library and improve MSVC output artifacts naming Mar 13, 2023
@hebasto
Copy link
Member Author

hebasto commented Mar 13, 2023

Also I've noticed and fixed a wrong ABI version for a shared library.

The PR description has been updated as well.

@hebasto hebasto marked this pull request as draft April 10, 2023 20:38
@hebasto
Copy link
Member Author

hebasto commented Apr 10, 2023

Also I've noticed and fixed a wrong ABI version for a shared library.

The ABI version fix has been moved into a separated #1270.


Drafted for now.

@hebasto
Copy link
Member Author

hebasto commented Apr 21, 2023

Closing in favour of #1270.

@hebasto hebasto closed this Apr 21, 2023
@hebasto hebasto deleted the 230311-msvc branch April 29, 2023 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants