Fix MSVC warning C4273 “inconsistent dll linkage” #30491

1. 
   hebasto commented at 9:07 pm on July 19, 2024: member

   Broken out of #30454.

   When using CMake, the user can select the MSVC runtime library to be:

   1. Statically-linked (with the corresponding x64-windows-static vcpkg triplet) or
   2. Dynamically-linked (with the corresponding x64-windows vcpkg triplet)

   In the latter case, the compiler emits the C4273 warning.

   As the “Necessary on some platforms” comment does not apply to MSVC, skip the declaration for MSVC.

   The MSVC build system in the master branch supports the statically-linked runtime only: https://github.com/bitcoin/bitcoin/blob/ed739d14b58b5e772a65b85bb421703963b06852/build_msvc/common.init.vcxproj.in#L65

2. Fix MSVC warning C4273 "inconsistent dll linkage"

   When using CMake, the user can select the MSVC runtime library to be:
   1) Statically-linked (with the corresponding `x64-windows-static` vcpkg
   triplet) or
   2) Dynamically-linked (with the corresponding `x64-windows` vcpkg
   triplet)
   
   In the latter case, the compiler emits the C4273 warning.
   
   As the "Necessary on some platforms" comment does not apply to MSVC,
   skip the declaration for MSVC.

   7703884ab1
3. 
   DrahtBot commented at 9:07 pm on July 19, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	sipa, sipsorcery, theuni

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. 
   hebasto commented at 9:14 pm on July 19, 2024: member

   Friendly ping:

   • @theuni per offline request for this PR
   • @sipsorcery as a Windows connoisseur
   • @sipa as an author of the original code
5. 
   sipa commented at 9:16 pm on July 19, 2024: member

   utACK 7703884ab19cd7ffddc5c52ba57dec82fbc8dc2b
6. 
   sipsorcery commented at 9:25 pm on July 19, 2024: member

   utACK 7703884ab19cd7ffddc5c52ba57dec82fbc8dc2b.
7. theuni approved
8. 
   theuni commented at 10:08 pm on July 19, 2024: member

   Microsoft says on Windows it’s coming from Stdlib.h, which something else must be including. (Well, Microsoft says it’s called _environ, but I’m pretty sure the linker fixes that up for us).

   From what I can tell, this will simply fail to link if it’s not found somewhere. Also, this is just another opportunistic entropy source for us, so setting aside a Debian+OpenSSL-type whoopsie, I don’t think it’s a huge deal if this is null anyway.

   So.. as long as this doesn’t break anything, fine by me.

   utACK 7703884ab19cd7ffddc5c52ba57dec82fbc8dc2b

   Though, I don’t like the slippery slope notion here. Should we go ahead and disable runtimes/link modes we don’t support? Just because CMake enables it doesn’t mean we want to start supporting it.

9. 
   fanquake commented at 1:49 pm on July 20, 2024: member

   Though, I don’t like the slippery slope notion here. Should we go ahead and disable runtimes/link modes we don’t support? Just because CMake enables it doesn’t mean we want to start supporting it.

   I agree. It seems that rather than just porting what we currently build for MSVC, and adding support for new things later (once we’ve decided what to do), we’ve instead ported master + assumed support for a number of other things that we don’t otherwise test/maintain, or use to ship releases.

   Just because CMake might support various different Windows linkage models, or runtimes, etc, I think there’s still a discussion to be had around if we as a project actually want to actively support all these things, want to test them in CI etc.

   Hopefully we can spend some more time improving the release builds we actually ship to Windows users, i.e #30210, before we worry too much about supporting the matrix of different ways to use Visual Studio to compile Bitcoin Core.

10. fanquake merged this on Jul 20, 2024
11. fanquake closed this on Jul 20, 2024

    ---

Contributors
hebasto DrahtBot sipa sipsorcery theuni fanquake