Fixes compile errors in MSVC build #27332 #27335

EthanHeilman · 2023-03-26T21:31:05Z

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 · 2023-03-26T21:31:07Z

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.

hernanmarino

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

hernanmarino · 2023-03-26T22:11:43Z

build_msvc/bitcoin_config.h.in

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

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.

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]

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

hebasto

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:

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

For example see 1039ed4.

build_msvc/bitcoin_config.h.in

hebasto · 2023-03-27T09:43:36Z

build_msvc/common.init.vcxproj.in

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

As far as I can tell the bcrypt.lib dependency was introduced into libevent in 2020. libevent/libevent@138a408

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.

EthanHeilman · 2023-03-29T23:40:22Z

@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 · 2023-03-30T12:49:36Z

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 20b6c87, 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 · 2023-03-30T15:52:19Z

@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 · 2023-03-30T18:24:43Z

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 · 2023-04-03T12:23:37Z

@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 · 2023-04-03T21:01:11Z

tACK 2843696.

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.

hebasto · 2023-04-05T10:35:28Z

build_msvc/vcpkg.json

+  "overrides": [
+    {
+      "name": "libevent",
+      "version": "2.1.12+20230128"


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.

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.

build_msvc/vcpkg.json

EthanHeilman · 2023-04-05T16:32:31Z

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

hebasto

ACK c40c9edf9266bac0b3553a3d6c5b97f94c34312d

hebasto · 2023-04-05T17:52:39Z

build_msvc/vcpkg.json

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:

Suggested change

"builtin-baseline": "b86c0c35b88e2bf3557ff49dc831689c2f085090",

"builtin-baseline": "f14984af3738e69f197bf0e647a8dca12de92996",

+ Pins the compatible version of libevent in vcpkg

hebasto

re-ACK 6a9a4d1

6a9a4d1 Fixes compile errors in MSVC build bitcoin#27332 (Ethan Heilman) Pull request description: This PR is designed to address the issue bitcoin#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:minima`l 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 bitcoin#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 ==== 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. 1. 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. 2. This PR fixes the bcrypt.lib errors this by setting this library as a dependency in build_msvc/common.init.vcxproj.in. ACKs for top commit: hebasto: re-ACK 6a9a4d1 Tree-SHA512: 69d2cb6a62ecca976540a39e5f83a4e5386d920a564761cedc5127d82e5fa66ced7fcde3e5e5eb3bd6cde1cc78f843e94aa2789f02bd3e3414118030017d0387

6d05c4f msvc: Specify `boost-date-time` package explicitly (Hennadii Stepanov) 1f97e51 msvc: Update vcpkg manifest baseline up to "2023.08.09 Release" (Hennadii Stepanov) 2d2ef2f msvc: No need to specify the default feature for `libevent` package (Hennadii Stepanov) Pull request description: Last time we updated dependency packages used when compiling with MSVC in #26891. Then we [switched](#27335) to specifying the `builtin-baseline` in the `vcpkg.json` manifest file, which made checking out the entire vcpkg repository to a specific commit or tag unneeded. This PR updates the manifest baseline from [2023.01.09](https://github.com/microsoft/vcpkg/releases/tag/2023.01.09) to [2023.08.09](https://github.com/microsoft/vcpkg/releases/tag/2023.08.09): - berkeleydb: 4.8.30#8 --> 4.8.30#9 - boost: 1.81.0 --> 1.82.0#2 - sqlite3: 3.40.0#1 --> 3.42.0#1 - zeromq: 4.3.4#6 --> 2023-06-20#1 The most recent https://github.com/microsoft/vcpkg/releases/tag/2023.11.20 tag is still unavailable in the vcpkg installation in the current GHA Windows image (the head commit is microsoft/vcpkg@2b14b60 only). The other https://github.com/microsoft/vcpkg/releases/tag/2023.10.19 tag [introduces](boostorg/process@0c42a58) a warning C4297 in the Boost.Process 1.83 that is [fixed](boostorg/process#340) in the upcoming version 1.84. ACKs for top commit: fanquake: ACK 6d05c4f Tree-SHA512: c71c46c13ad5a6d39ae72982b7765f690e7391092e4983887a53dceeee507c8fc256e887351bbe35c051bf88d2babeb283e17a2588ee86ec9020c4ba426bfcd7

hernanmarino reviewed Mar 26, 2023

View reviewed changes

hebasto reviewed Mar 27, 2023

View reviewed changes

EthanHeilman force-pushed the fixmsvc branch from 026aada to 6649bed Compare March 28, 2023 21:49

EthanHeilman force-pushed the fixmsvc branch from 6649bed to 2843696 Compare April 3, 2023 01:16

hebasto reviewed Apr 5, 2023

View reviewed changes

EthanHeilman force-pushed the fixmsvc branch 2 times, most recently from b2558a1 to c40c9ed Compare April 5, 2023 16:30

hebasto approved these changes Apr 5, 2023

View reviewed changes

DrahtBot requested a review from sipsorcery April 5, 2023 17:54

Fixes compile errors in MSVC build bitcoin#27332

6a9a4d1

+ Pins the compatible version of libevent in vcpkg

EthanHeilman force-pushed the fixmsvc branch from c40c9ed to 6a9a4d1 Compare April 5, 2023 20:50

hebasto approved these changes Apr 5, 2023

View reviewed changes

fanquake linked an issue Apr 6, 2023 that may be closed by this pull request

Bitcoin fails to build on MSVC: Libevent and BCryptGenRandom errors #27332

Closed

1 task

fanquake merged commit 06fb95b into bitcoin:master Apr 6, 2023

hebasto mentioned this pull request Apr 6, 2023

build: Add CMake-based build system (5 of N) hebasto/bitcoin#13

Merged

hebasto mentioned this pull request Nov 25, 2023

msvc: Update vcpkg manifest #28938

Merged

bitcoin locked and limited conversation to collaborators Apr 5, 2024

	"builtin-baseline": "b86c0c35b88e2bf3557ff49dc831689c2f085090",
	"builtin-baseline": "f14984af3738e69f197bf0e647a8dca12de92996",

Fixes compile errors in MSVC build #27332 #27335

Fixes compile errors in MSVC build #27332 #27335

Uh oh!

Conversation

EthanHeilman commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The issue

Fixes (Initially Proposed)

Fixes (Actual)

Uh oh!

DrahtBot commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EthanHeilman commented Mar 29, 2023

Uh oh!

hebasto commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EthanHeilman commented Mar 30, 2023

Uh oh!

hebasto commented Mar 30, 2023

Uh oh!

EthanHeilman commented Apr 3, 2023

Uh oh!

sipsorcery commented Apr 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EthanHeilman commented Apr 5, 2023

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

EthanHeilman commented Mar 26, 2023 •

edited

Loading

DrahtBot commented Mar 26, 2023 •

edited

Loading

hebasto left a comment •

edited

Loading

hebasto Mar 27, 2023 •

edited

Loading

hebasto commented Mar 30, 2023 •

edited

Loading