EthanHeilman commented at 9:31 pm on March 26, 2023: contributor

This PR is designed to address the issue #27332. The MSVC build is failing because of two bugs in how the build is configured.

The issue

When running msbuild build_msvc\bitcoin.sln -property:Configuration=Release -maxCpuCount -verbosity:minimal the build fails with following two errors.

C:\Users\e0\Documents\GitHub\bitcoin\src\httpserver.cpp(637,9): error C2664: 'void evhttp_connection_get_peer(evhttp_connection *,const char **,uint16_t *)': cannot convert argument 2 from 'char **' to 'const char **' [C:\Users\e0\Documents\GitHub\bitcoin\build_msvc\libbitcoin_node\libbitcoin_node .vcxproj]

This error is occurs because bitcoin is using the wrong function signature for evhttp_connection_get_peer in libevent. In automake builds, configure.ac inspects the version of libevent it is building against and then defines HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR to flag the source code to use the correct signature. In MSVC build there does not appear to be a mechanism to do this. So it uses the wrong signature and fails. See the PR #23607 for when this logic was added to automake builds.

event.lib(evutil_rand.c.obj) : error LNK2019: unresolved external symbol BCryptGenRandom referenced in function arc4_seed [C:\Users\e0\Documents\GitHub\bitcoin\build_msvc\bitcoin-cli\bitcoin-cli.vcxproj] C:\Users\e0\Documents\GitHub\bitcoin\build_msvc\x64\Release\bitcoin-cli.exe : fatal error LNK1120: 1 unresolved externals [C:\Users\e0\Documents\GitHub\bitcoin\build_msvc\bitcoin-cli\bitcoin-cli.vcxproj]

This error is caused by msbuild not being able to find the library bcrypt.lib because it has not been configured to use bcrypt.lib.

Fixes (Initially Proposed)

Edit: This bug was solved using an alternative fix documented below as Fixes (Actual)

While for automake builds a macro is being define to configure the current function signature for evhttp_connection_get_peer in libevent, this macro is not being defined for MSVC builds.

This PR addresses this issue by assuming more recent version of libevent is installed and always defining HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR. This logic is more brittle the automake logic, but someone following the MSVC build instructions should only get the more recent version of libevent.
This PR fixes the bcrypt.lib errors this by setting this library as a dependency in build_msvc/common.init.vcxproj.in.

Fixes (Actual)

This PR Pin the version of libevent to an older version which uses the older function signature and does not require bcrypt as a separate dependency.

If at some point we have not moved to CMake and we wish upgrade libevent we need to perform the actions in Fixes (Initially Proposed). However it is likely Bitcoin will be on CMake before that is neccessary.

DrahtBot commented at 9:31 pm on March 26, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto
Concept ACK	hernanmarino
Stale ACK	sipsorcery

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

in build_msvc/bitcoin_config.h.in:153 in 026aadaea6 outdated

149@@ -150,6 +150,9 @@
150 /* ::wsystem is available */
151 #define HAVE_SYSTEM 1
152 
153+/* Assume that an MSVC build will get dependencies from vcpk and thus use the latest version of libevent */

hernanmarino commented at 10:11 pm on March 26, 2023:

Is there a chance that this is not the case in any situation ?

EthanHeilman commented at 10:59 pm on March 26, 2023:

I don’t know. I would prefer a solution more in line with how this was solved in automake builds but I don’t have time to learn enough about msbuild to implement it and it is currently broken in the default case.

EthanHeilman commented at 11:05 pm on March 26, 2023:

In fact the build in CI is now failing because it expects the other function signature.

C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\httpserver.cpp(635,56): error C2664: 'void evhttp_connection_get_peer(evhttp_connection *,char **,uint16_t *)': cannot convert argument 2 from 'const char **' to 'char **' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_node\libbitcoin_node.vcxproj]

hebasto commented at 9:43 am on March 27, 2023:

0/* Assume that an MSVC build will get dependencies from vcpkg and thus use the latest version of libevent */

EthanHeilman commented at 11:41 pm on March 29, 2023:

Using the latest version of libevent in CI is now fixes the build.

hernanmarino commented at 10:12 pm on March 26, 2023: contributor

Concept ACK, but not really sure about the approach (see review comment). Will try to test later this week.

in build_msvc/common.init.vcxproj.in:98 in 026aadaea6 outdated

94@@ -95,7 +95,7 @@
95     </ClCompile>
96     <Link>
97       <SubSystem>Console</SubSystem>
98-      <AdditionalDependencies>Iphlpapi.lib;ws2_32.lib;Shlwapi.lib;kernel32.lib;user32.lib;gdi32.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies)</AdditionalDependencies>
99+      <AdditionalDependencies>bcrypt.lib;Iphlpapi.lib;ws2_32.lib;Shlwapi.lib;kernel32.lib;user32.lib;gdi32.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies)</AdditionalDependencies>

hebasto commented at 9:43 am on March 27, 2023:

Is it a new dependency introduced by the recent libevent version?

EthanHeilman commented at 6:44 pm on March 27, 2023:

As far as I can tell the bcrypt.lib dependency was introduced into libevent in 2020. https://github.com/libevent/libevent/commit/138a408c771c21668bcc605c2df00d0384d476d8

EthanHeilman commented at 4:34 pm on April 5, 2023:

With 2.1.12#7 we don’t need to include bcrypt.lib as an additional dependency and so I have removed it. If we upgrade libevent we will need to add this.

hebasto commented at 9:43 am on March 27, 2023: member

In fact the build in CI is now failing because it expects the other function signature.

The recent vcpkg release has updated the libevent:

libevent 2.1.12#7 -> 2.1.12+20230128#0

To fix CI build we have to:

 0--- a/.cirrus.yml
 1+++ b/.cirrus.yml
 2@@ -103,7 +103,7 @@ task:
 3   env:
 4     PATH: 'C:\jom;C:\Python39;C:\Python39\Scripts;C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\MSBuild\Current\Bin;%PATH%'
 5     PYTHONUTF8: 1
 6-    CI_VCPKG_TAG: '2023.01.09'
 7+    CI_VCPKG_TAG: '2023.02.24'
 8     VCPKG_DOWNLOADS: 'C:\Users\ContainerAdministrator\AppData\Local\vcpkg\downloads'
 9     VCPKG_DEFAULT_BINARY_CACHE: 'C:\Users\ContainerAdministrator\AppData\Local\vcpkg\archives'
10     CCACHE_DIR: 'C:\Users\ContainerAdministrator\AppData\Local\ccache'

For example see https://github.com/bitcoin/bitcoin/commit/1039ed44fb445fb4e58e1990bd07be15b1dbdd15.

EthanHeilman force-pushed on Mar 28, 2023

EthanHeilman commented at 11:40 pm on March 29, 2023: contributor

@hebasto Thanks for the help for getting the CI/CD fixed. The fact that the version of libevent was being pinned to the earlier version explains why testing didn’t catch this.

hebasto commented at 12:49 pm on March 30, 2023: member

The fact that the version of libevent was being pinned to the earlier version explains why testing didn’t catch this.

To be precise, the vcpkg version is pinned now. Maybe actual pinning libevent version, similar to https://github.com/bitcoin/bitcoin/commit/20b6c871178f20661b849ad5677bd8ecae55cf19, could provide a better user experience of native building on Windows? Otherwise, with the current suggested approach, Windows users will be forced to update their vcpkg installations.

cc @sipsorcery

EthanHeilman commented at 3:52 pm on March 30, 2023: contributor

@hebasto Thats a great point. I had only been thinking of users that are in the same boat as me, doing a fresh MSVC build, but you are right, most users are likely building against pre-installed packages. I don’t think my PR should be merged until it either:

Adding the ability to define the function signature based on the actual version of libevent installed OR
Pinning the version of libevent in vcpkg.json so that users with old dependencies upgrade when they run build

I’m fairly allergic to the idea of build scripts inspecting function signatures and then defining macros based on that. I understand it makes sense sometimes, but thankfully I don’t think we need to do it in this case. I’d advocate for version pinning in vcpkg.json for the follow reasons:

Version pinning is a more predictable and straightforward approach.
It also enables the ability to force upgrade when dependencies have vulnerabilities.
Makes sure everyone is building from the same dependencies which helps identify root causes of bugs.
It lets us ensure that the CI is using the same version of dependencies as someone building from scratch. Likely version pinning would have caught this bug in tests.

When I get free from work tonight I’ll add the version to vcpkg.json in the PR.

hebasto commented at 6:24 pm on March 30, 2023: member

I’m fairly allergic to the idea of build scripts inspecting function signatures and then defining macros based on that.

FWIW, the coming CMake-based build system will do it :)

EthanHeilman force-pushed on Apr 3, 2023

EthanHeilman commented at 12:23 pm on April 3, 2023: contributor

@hernanmarino I have libevent pinned to the version with the matching signature function. Let me know if you want anything else changed in this PR.

sipsorcery commented at 9:01 pm on April 3, 2023: member

tACK 28436965b6422bd92d23033a5ddbd60a6c183cd7.

The version pinning is nice. It was badly missed feature when we first started relying on vcpkg. We pinned the version of vcpkg instead but individual package pinning should be more flexible.

in build_msvc/vcpkg.json:21 in 28436965b6 outdated

12@@ -13,5 +13,12 @@
13       "features": ["thread"]
14     },
15     "zeromq"
16+  ],
17+  "builtin-baseline": "b86c0c35b88e2bf3557ff49dc831689c2f085090",
18+  "overrides": [
19+    {
20+      "name": "libevent",
21+      "version": "2.1.12+20230128"

hebasto commented at 10:35 am on April 5, 2023:

Pinning to the latest available version looks suboptimal as it still forces users to upgrade their vcpkg installation.

OTOH, pinning to the previous one, for example, 2.1.12#7, could be a better option for an additional reason: no need to change build_msvc/bitcoin_config.h.in and build_msvc/common.init.vcxproj.in.

EthanHeilman commented at 2:14 pm on April 5, 2023:

I think that is a fair point. The issue is that if we ever need to upgrade libevent due to security vulnerability it will break and you’ll have to figure this out again and change build_msvc/bitcoin_config.h.in. This addresses that issue today rather then at some point in the future. That said, if we expect to move to CMake soon, then it makes sense to move to the previous version and get the minimum diff.

I’ll change this to 2.1.12#7, but I’m open to changing to back to 2.1.12+20230128.

in build_msvc/vcpkg.json:24 in 28436965b6 outdated

20+      "name": "libevent",
21+      "version": "2.1.12+20230128"
22+    }
23   ]
24-}
25+}

hebasto commented at 10:36 am on April 5, 2023:

Missed EOL?

EthanHeilman force-pushed on Apr 5, 2023

EthanHeilman commented at 4:32 pm on April 5, 2023: contributor

Pushed this on my lunch break. With @hebasto’s help this is now a single file, 8 line change. =)

in build_msvc/vcpkg.json:17 in c40c9edf92 outdated

12@@ -13,5 +13,12 @@
13       "features": ["thread"]
14     },
15     "zeromq"
16+  ],
17+  "builtin-baseline": "b86c0c35b88e2bf3557ff49dc831689c2f085090",

hebasto commented at 5:52 pm on April 5, 2023:

This baseline implies package versions as they were at 2022.02.23 tag, which means older ones than they are in our master branch for now. For details, see https://cirrus-ci.com/task/4695704203952128.

Suggesting to update it up to the 2023.01.09 tag:

0  "builtin-baseline": "f14984af3738e69f197bf0e647a8dca12de92996",

EthanHeilman commented at 8:50 pm on April 5, 2023:

done

hebasto approved

hebasto commented at 5:54 pm on April 5, 2023: member

ACK c40c9edf9266bac0b3553a3d6c5b97f94c34312d

DrahtBot requested review from sipsorcery on Apr 5, 2023

Fixes compile errors in MSVC build #27332

+ Pins the compatible version of libevent in vcpkg

6a9a4d13b2

EthanHeilman force-pushed on Apr 5, 2023

hebasto approved

hebasto commented at 11:28 pm on April 5, 2023: member

re-ACK 6a9a4d13b2b22632e0acd4f86f7bac238294ba42

fanquake merged this on Apr 6, 2023

fanquake closed this on Apr 6, 2023

sidhujag referenced this in commit e5f2992bfa on Apr 6, 2023

fanquake referenced this in commit c7f0e97383 on Nov 30, 2023

bitcoin locked this on Apr 5, 2024

Fixes compile errors in MSVC build #27332 #27335

The issue

Fixes (Initially Proposed)

Fixes (Actual)

Reviews