ci: Add missed configuration options to "Win64 native" job

hebasto commented at 3:13 pm on August 29, 2024: member

Some build options were overlooked when the CMake staging branch dropped package autodetection.

This PR restores them. Similar to #30740.

hebasto added the label Windows on Aug 29, 2024

hebasto added the label Tests on Aug 29, 2024

DrahtBot commented at 3:13 pm on August 29, 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	fanquake, maflcko

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

Conflicts

No conflicts as of last run.

maflcko commented at 7:15 pm on August 29, 2024: member

0Run wallet_bdb_parser with args ['D:\\a\\bitcoin\\bitcoin\\build\\src\\test\\fuzz\\Release\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/wallet_bdb_parser')]Assertion failed: db, file D:\a\bitcoin\bitcoin\src\wallet\test\fuzz\wallet_bdb_parser.cpp, line 114
1Error processing input "D:\\a\\_temp\\qa-assets\\fuzz_seed_corpus\\wallet_bdb_parser\\19d433fc32c4bc3652124d302b97790d9a9a6cd1"
2
3Assertion failed: db, file D:\a\bitcoin\bitcoin\src\wallet\test\fuzz\wallet_bdb_parser.cpp, line 114
4Error processing input "D:\\a\\_temp\\qa-assets\\fuzz_seed_corpus\\wallet_bdb_parser\\19d433fc32c4bc3652124d302b97790d9a9a6cd1"
5
6Target ['D:\\a\\bitcoin\\bitcoin\\build\\src\\test\\fuzz\\Release\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/wallet_bdb_parser')] failed with exit code 1

hebasto commented at 9:02 pm on August 29, 2024: member

0Run wallet_bdb_parser with args ['D:\\a\\bitcoin\\bitcoin\\build\\src\\test\\fuzz\\Release\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/wallet_bdb_parser')]Assertion failed: db, file D:\a\bitcoin\bitcoin\src\wallet\test\fuzz\wallet_bdb_parser.cpp, line 114
1Error processing input "D:\\a\\_temp\\qa-assets\\fuzz_seed_corpus\\wallet_bdb_parser\\19d433fc32c4bc3652124d302b97790d9a9a6cd1"
2
3Assertion failed: db, file D:\a\bitcoin\bitcoin\src\wallet\test\fuzz\wallet_bdb_parser.cpp, line 114
4Error processing input "D:\\a\\_temp\\qa-assets\\fuzz_seed_corpus\\wallet_bdb_parser\\19d433fc32c4bc3652124d302b97790d9a9a6cd1"
5
6Target ['D:\\a\\bitcoin\\bitcoin\\build\\src\\test\\fuzz\\Release\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/wallet_bdb_parser')] failed with exit code 1

Apparently, #28074 and #26606 forgot to update build_msvc/fuzz/fuzz.vcxproj, which resulted in src/wallet/test/fuzz/crypter.cpp and src/wallet/test/fuzz/wallet_bdb_parser.cpp were not compiled in the fuzz.exe binary when building with the legacy MSVC build system.

hebasto force-pushed on Aug 31, 2024

hebasto commented at 5:44 am on August 31, 2024: member

The problematic wallet_bdb_parser has been disabled.

in src/wallet/test/fuzz/CMakeLists.txt:22 in 00ec0acbb1 outdated

18+  # See: https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2322763212
19+  target_sources(fuzz
20+    PRIVATE
21+      wallet_bdb_parser.cpp
22+  )
23+endif()

maflcko commented at 7:16 am on September 2, 2024:

Not sure if the build system is the right place for a temporary workaround such as this. I expect the fuzz target needs to be updated, so wouldn’t it be better if the temporary workaround was in the fuzz target as well? This way, only a single file has to be touched in a possible follow-up. Also, the code would still be compiled, but not run on msvc.

hebasto commented at 11:32 am on September 2, 2024:

Thanks! Your proposal has been accepted.

hebasto force-pushed on Sep 2, 2024

maflcko commented at 10:18 am on September 2, 2024: member

review ACK 60f21202e038960bca859a86451181277856ef61

in .github/workflows/ci.yml:185 in 60f21202e0 outdated

185@@ -186,7 +186,7 @@ jobs:
186 
187       - name: Generate build system
188         run: |
189-          cmake -B build --preset vs2022-static -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_INSTALLATION_ROOT\scripts\buildsystems\vcpkg.cmake" -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWERROR=ON
190+          cmake -B build --preset vs2022-static -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_INSTALLATION_ROOT\scripts\buildsystems\vcpkg.cmake" -DWITH_BDB=ON -DWITH_MINIUPNPC=ON -DWITH_ZMQ=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWERROR=ON

fanquake commented at 1:49 pm on September 4, 2024:

From https://github.com/bitcoin/bitcoin/actions/runs/10665138943/job/29557888413#step:8:1:

0Preset CMake variables:
1  BUILD_GUI="ON"
2  VCPKG_TARGET_TRIPLET="x64-windows-static"
3  WITH_NATPMP="OFF"
4  WITH_QRENCODE="OFF"

Can you explain why some options are passed on the command line, and why some are put into CMakePresets.json; what determines where each option belongs? I’d think it’d be better to have a single source of truth for the CI config.

Also, I see compared to 28.x, we’ve added new packages since porting to CMake, i.e miniupnpc, so why not also add libqrencode, or is there a reason it’s hardcoded to OFF?

fanquake commented at 1:53 pm on September 4, 2024:

For example, I can imagine someone changing CMakePresets.json, without realising that also partially controls the CI, and then we (silently) lose Qt compilation in this build.

hebasto commented at 10:20 am on September 6, 2024:

WITH_NATPMP is OFF because the libnatpmp package is not available in vcpkg.

WITH_QRENCODE is OFF because I faced build errors for this package or its dependencies in the past (I can’t recall the exact issue). I’ve tested it recently and I got another build error, which is weird, because it happens in the main build system:

0 Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)

It needs to be investigated further.

fanquake commented at 10:33 am on September 6, 2024:

WITH_NATPMP is OFF WITH_QRENCODE is OFF

It seems like for at least libnatpmp, it’d be better to just set nothing, rather than hardcoding to OFF. As a package that doesn’t exist can’t cause issues, and I assume it could exist at some point, or the user could vendor it themselves, and then they would just need to override this (it also can’t cause issues unless we opt into using it, even if it somehow ended up installed).

For libqrencode, if it’s going to cause build issues, if the user has it and Qt installed, it’d probably be good to document that as the reason it is disabled.

hebasto commented at 10:43 am on September 6, 2024:

For libqrencode, if it’s going to cause build issues, if the user has it and Qt installed, it’d probably be good to document that as the reason it is disabled.

Any suggestions about the place for such a note? Asking because the JSON format has no comments, and CMake’s parser does not allow extra fields, such as "_comment:".

fanquake commented at 10:46 am on September 6, 2024:

In suggestions about the place for such a note.

I guess somewhere in the Windows build docs? Note, still wondering in regards to:

Can you explain why some options are passed on the command line, and why some are put into CMakePresets.json; what determines where each option belongs? I’d think it’d be better to have a single source of truth for the CI config.

hebasto commented at 10:49 am on September 6, 2024:

Presets contain options that seem reasonable in most cases.

fanquake commented at 10:56 am on September 6, 2024:

Presets contain options that seem reasonable in most cases.

Sure, but that’s too vauge to use as reasoning for where CI configuration should live, and doesn’t solve this issue:

For example, I can imagine someone changing CMakePresets.json, without realising that also partially controls the CI, and then we (silently) lose Qt compilation in this build.

I’m not sure the configuration of the CI, should be (unexplicitly) coupled to the CMake Preset. This isn’t specific to the GUI option, it holds for all options. Developers modifying these files need to know what lives where, and why. If we want to use Presets for the CI, that’s possibly also fine, but they should be dedicated presets, so it’s clear what they are for.

hebasto commented at 11:01 am on September 6, 2024:

If we want to use Presets for the CI, that’s possibly also fine, but they should be dedicated presets, so it’s clear what they are for.

You highlighted a problem that is relevant to all CI jobs and is wider than this PR goal. Can we leave it for a follow up dedicated discussion/PR?

fanquake commented at 11:06 am on September 6, 2024:

Seems like we could just add the relevant options from the preset to the ci config file for now, which makes the config clear, and removes the possibility for external breakage? If at some point we switch to presets, they would all them be moved to a dedicated CI preset.

hebasto commented at 11:21 am on September 6, 2024:

In suggestions about the place for such a note.

I guess somewhere in the Windows build docs?

Thanks! Done.

Seems like we could just add the relevant options from the preset to the ci config file for now

Implemented.

hebasto force-pushed on Sep 6, 2024

ci: Add missed configuration options to "Win64 native" job e07a3ede52

fuzz: Don't compile BDB-specific code on MSVC in `wallet_bdb_parser.cpp` c07fdd6546

doc: Update and amend MSVC build guide ee22bf55e3

hebasto force-pushed on Sep 6, 2024

fanquake approved

fanquake commented at 11:28 am on September 6, 2024: member

ACK ee22bf55e39e4a40c31465e8ef2663eb7fefc783

DrahtBot requested review from maflcko on Sep 6, 2024

maflcko commented at 12:45 pm on September 6, 2024: member

review-only ACK ee22bf55e39e4a40c31465e8ef2663eb7fefc783

fanquake merged this on Sep 6, 2024

fanquake closed this on Sep 6, 2024

hebasto deleted the branch on Sep 6, 2024

ci: Add missed configuration options to “Win64 native” job #30755

Code Coverage

Reviews

Conflicts