build: Use SECP256K1_STATICLIB macro instead of warning suppressions #1346

1. 
   hebasto commented at 12:45 pm on June 13, 2023: member

   The changes from #1209 has been described as:

   Fixed declarations of API variables for MSVC (__declspec(dllimport)). This fixes MSVC builds of programs which link against a libsecp256k1 DLL dynamically and use API variables (and not only API functions). Unfortunately, the MSVC linker now will emit warning LNK4217 when trying to link against libsecp256k1 statically. Pass /ignore:4217 to the linker to suppress this warning.

   Apparently, this description is not complete. When building Bitcoin Core, the other warning is raised as well:

   0LINK : warning LNK4217: symbol 'secp256k1_context_static' defined in 'libsecp256k1.lib(secp256k1.obj)' is imported by 'libbitcoin_consensus.lib(pubkey.obj)' in function '"public: static bool __cdecl CPubKey::CheckLowS(class std::vector<unsigned char,class std::allocator<unsigned char> > const &)" (?CheckLowS@CPubKey@@SA_NAEBV?$vector@EV?$allocator@E@std@@@std@@@Z)' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
   1LINK : warning LNK4286: symbol 'secp256k1_context_static' defined in 'libsecp256k1.lib(secp256k1.obj)' is imported by 'libbitcoin_common.lib(key.obj)' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
   

   This PR provides to the user of a static libsecp256k1 library an option to define the SECP256K1_STATICLIB macro instead of ignoring MSVC linker warnings LNK4217 and LNK4286.

2. hebasto marked this as a draft on Jun 13, 2023
3. build: Use `SECP256K1_STATICLIB` macro instead of warning suppressions

   This change allows the user to define the `SECP256K1_STATICLIB` macro
   instead of ignoring MSVC linker warnings LNK4217 and LNK4286.

   6177b8a75c
4. hebasto force-pushed on Jun 13, 2023
5. hebasto marked this as ready for review on Jun 13, 2023
6. 
   real-or-random commented at 10:16 am on June 14, 2023: contributor

   Concept ACK

   The macro is cheap, and if it helps Core get a cleaner build, then we should just add it. Many hours of researching and experimenting went into that piece of code, so I’ll probably follow up with a PR that adds extensive comments.

   ---

   Apparently, this description is not complete. When building Bitcoin Core, the other warning is raised as well:

   0LINK : warning LNK4217: symbol 'secp256k1_nonce_function_rfc6979' defined in 'libsecp256k1.lib(secp256k1.obj)' is imported by 'libbitcoin_common.lib(key.obj)' in function '"public: bool __cdecl CKey::Sign(class uint256 const &,class std::vector<unsigned char,class std::allocator<unsigned char> > &,bool,unsigned int)const " (?Sign@CKey@@QEBA_NAEBVuint256@@AEAV?$vector@EV?$allocator@E@std@@@std@@_NI@Z)' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
   

   Do you mean the LNK4286 is also raised? I see that one in the logs.

7. 
   hebasto commented at 10:19 am on June 14, 2023: member

   Do you mean the LNK4286 is also raised? I see that one in the logs.

   Yes. I put the wrong line into the PR description. Updated.

8. 
   hebasto commented at 11:42 am on June 23, 2023: member

   When building Bitcoin Core, the other warning is raised as well:

   0LINK : warning LNK4217: symbol 'secp256k1_context_static' defined in 'libsecp256k1.lib(secp256k1.obj)' is imported by 'libbitcoin_consensus.lib(pubkey.obj)' in function '"public: static bool __cdecl CPubKey::CheckLowS(class std::vector<unsigned char,class std::allocator<unsigned char> > const &)" (?CheckLowS@CPubKey@@SA_NAEBV?$vector@EV?$allocator@E@std@@@std@@@Z)' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
   1LINK : warning LNK4286: symbol 'secp256k1_context_static' defined in 'libsecp256k1.lib(secp256k1.obj)' is imported by 'libbitcoin_common.lib(key.obj)' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
   

   This PR provides to the user of a static libsecp256k1 library an option to define the SECP256K1_STATICLIB macro instead of ignoring MSVC linker warnings LNK4217 and LNK4286. @sipsorcery Friendly ping :)

   As a Windows connoisseur, what do you think?

9. 
   sipsorcery commented at 8:43 pm on June 23, 2023: member

   As a Windows user I’m used to just ignoring crap from the OS so I would have stopped at adding an option to ignore the warnings.

   This PR is a step further and makes it even cleaner so looks good to me.

   ACK 6177b8a75c2b4e8263fc7af4930197abc5dd9f6b.

10. real-or-random added the label build on Jun 26, 2023
11. real-or-random cross-referenced this on Jun 27, 2023 from issue build: Improvements to symbol visibility logic on Windows by real-or-random
12. 
    hebasto commented at 1:21 pm on June 28, 2023: member

    Closing in favor of #1362.
13. hebasto closed this on Jun 28, 2023

    ---

14. hebasto cross-referenced this on Jun 29, 2023 from issue build: Improvements to symbol visibility logic on Windows (attempt 3) by hebasto
15. real-or-random referenced this in commit 9e6d1b0e9b on Jul 3, 2023
16. hebasto deleted the branch on Feb 13, 2024

Contributors
hebasto real-or-random sipsorcery

Labels
build