cmake: Use builtin support for .manifest files #33585

purpleKarrot commented at 3:44 PM on October 9, 2025: contributor

Remove some redundant logic from the CMake code:

The WIN32_EXECUTABLE target property only has an effect when building for WIN32. Checking WIN32 is redundant.
CMake has builtin support for .rc and .manifest files. Both may be added to sources unconditionally. They only have an effect when building for WIN32.

DrahtBot added the label Build system on Oct 9, 2025

DrahtBot commented at 3:44 PM on October 9, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33585.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	151henry151

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32387 (ipc: add windows support by ryanofsky)
#31507 (build: Use clang-cl to build on Windows natively by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake commented at 4:04 PM on October 9, 2025: member

This doesn't Guix build:

[100%] Built target bitcoin-qt
Running symbol and dynamic library checks...
/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin.exe: failed APPLICATION_MANIFEST
/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoind.exe: failed APPLICATION_MANIFEST
/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin-qt.exe: failed APPLICATION_MANIFEST
/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin-cli.exe: failed APPLICATION_MANIFEST
/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin-tx.exe: failed APPLICATION_MANIFEST
/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin-util.exe: failed APPLICATION_MANIFEST
/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin-wallet.exe: failed APPLICATION_MANIFEST
/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/test_bitcoin.exe: failed APPLICATION_MANIFEST
make[3]: *** [CMakeFiles/check-symbols.dir/build.make:71: CMakeFiles/check-symbols] Error 1
make[2]: *** [CMakeFiles/Makefile2:388: CMakeFiles/check-symbols.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:395: CMakeFiles/check-symbols.dir/rule] Error 2
make: *** [Makefile:179: check-symbols] Error 2

purpleKarrot force-pushed on Oct 9, 2025

purpleKarrot commented at 7:23 PM on October 9, 2025: contributor

This doesn't Guix build:

I added a workaround for upstream issue 23244. It should pass now.

hebasto commented at 7:50 AM on October 10, 2025: member

I agree on the refactoring first and last commits.

The mentioned upstream issue was exactly the reason why I implemented manifest handling this way in #32396.

Is there any CMake's docs regarding handling .manifest files?

purpleKarrot commented at 8:10 AM on October 10, 2025: contributor

The mentioned upstream issue was exactly the reason why I implemented manifest handling this way in #32396.

Got it. I think the advantage of using the workaround conditionally with a comment to the upstream issue is that it is self-documenting and simplifies future refactoring once the issue is solved upstream. If the workaround is used unconditionally for all platforms, it is not obvious to future contributors that this is in fact a workaround, and they may fall into the same trap as I did.

Is there any CMake's docs regarding handling .manifest files?

https://cmake.org/cmake/help/latest/release/3.4.html#other

CMake itself uses a similar condition: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/11112/diffs

hebasto commented at 11:16 AM on October 10, 2025: member

Is there any CMake's docs regarding handling .manifest files?

https://cmake.org/cmake/help/latest/release/3.4.html#other

Thank you for the link!

The release notes mentioned above state:

Manifest files ... will be merged with linker-generated manifests and embedded in the binary.

This PR enables linker-generated manifests by removing the /MANIFEST:NO linker flag. However, I still have a couple of questions:

What is the content of the linker-generated manifest? What is the logic behind merging ${target}.manifest with a linker-generated manifest? Why do we need linker-generated manifests?
I'm not entirely sure that any fix of https://gitlab.kitware.com/cmake/cmake/-/issues/23244 will follow the same logic for MinGW as for MSVC. Wouldn't it be more prudent to keep the MSVC-specific /MANIFEST:NO linker flag and maintain a single code path for both MinGW as for MSVC in the add_windows_application_manifest() function?

fanquake commented at 10:18 AM on February 9, 2026: member

However, I still have a couple of questions: @purpleKarrot are you able to followup to the Qs here?

purpleKarrot commented at 1:59 PM on February 9, 2026: contributor

What is the content of the linker-generated manifest? What is the logic behind merging ${target}.manifest with a linker-generated manifest? Why do we need linker-generated manifests?

I am not a Windows expert, but according to my research, the linker generated manifest encodes runtime dependencies and compatibility metadata. Merging ${target}.manifest with it allows us to set custom entries while preserving the required runtime binding. Without the linker-generated manifest, it depends on the runtime environment which CRT will be used.

I'm not entirely sure that any fix of https://gitlab.kitware.com/cmake/cmake/-/issues/23244 will follow the same logic for MinGW as for MSVC.

I don't think it could be considered a fix if it does not follow the same logic.

Wouldn't it be more prudent to keep the MSVC-specific /MANIFEST:NO linker flag and maintain a single code path for both MinGW as for MSVC in the add_windows_application_manifest() function?

I don't think so. The two toolchains already differ in ABI, CRT, exception handling, TLS, and linker semantics. Disabling manifests does not normalize any of that. All it does is remove explicit dependency metadata on Windows and force the loader to fall back to legacy, environment-dependent resolution. That reduces determinism even within the same toolchain, let alone across different ones.

in cmake/module/AddWindowsResources.cmake:14 in 58db133cb6 outdated

  17 | -    configure_file(${PROJECT_SOURCE_DIR}/cmake/windows-app.manifest.in ${target}.manifest USE_SOURCE_PERMISSIONS)
  18 | +  configure_file(${PROJECT_SOURCE_DIR}/cmake/windows-app.manifest.in ${target}.manifest)
  19 | +  if(MSVC)
  20 | +    target_sources(${target} PRIVATE ${target}.manifest)
  21 | +  else()
  22 | +    # TODO: Remove when upstream issue is fixed:

fanquake commented at 7:07 AM on March 23, 2026:

I can't tell if https://gitlab.kitware.com/cmake/cmake/-/issues/23244 was fixed by https://gitlab.kitware.com/cmake/cmake/-/merge_requests/11112, or not, given the issue is still open. If it was, then we can update this was a CMake version for which the workaround can be removed.

purpleKarrot commented at 3:42 PM on March 26, 2026:

It is not a fix for CMake users. It is just the same workaround for CMake's own codebase.

fanquake commented at 4:05 PM on March 26, 2026:

Ok, so https://gitlab.kitware.com/cmake/cmake/-/work_items/23244 remains unfixed.

DrahtBot added the label Needs rebase on Mar 30, 2026

sedited commented at 8:30 AM on June 5, 2026: contributor

@purpleKarrot can you rebase this?

purpleKarrot force-pushed on Jun 5, 2026

DrahtBot removed the label Needs rebase on Jun 5, 2026

maflcko added the label DrahtBot Guix build requested on Jul 21, 2026

DrahtBot commented at 11:07 PM on July 21, 2026: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit b36c2d78a3ad4940e1d5eb466b0e6650b1001ee3<br>(master)	commit 2db33afa322c73642aaccf184924c23c14aaaa72<br>(pull/33585/merge)
*-aarch64-linux-gnu-debug.tar.gz	`6128720f5624239e...`	`72e4e70ea34bd005...`
*-aarch64-linux-gnu.tar.gz	`80deb1d91e896c04...`	`e7f87f6a534d6415...`
*-arm-linux-gnueabihf-debug.tar.gz	`8fe53c633f8b343b...`	`4c622d3544242fcc...`
*-arm-linux-gnueabihf.tar.gz	`26311f6942c6d252...`	`bc65498a7dcc2114...`
*-arm64-apple-darwin-codesigning.tar.gz	`667ce231d435ca7b...`	`9321d70b0c86880d...`
*-arm64-apple-darwin-unsigned.tar.gz	`18be5faecfabb8dd...`	`a6527d1102481202...`
*-arm64-apple-darwin-unsigned.zip	`45fa4aff38c3a7e5...`	`2e2b661caaece2be...`
*-powerpc64-linux-gnu-debug.tar.gz	`bb9f1bc3813b2f95...`	`8ce7ce0b795a0fee...`
*-powerpc64-linux-gnu.tar.gz	`c15be676f484ca6d...`	`3cad2c4bc11a8404...`
*-riscv64-linux-gnu-debug.tar.gz	`3930acbb4431117d...`	`81a412b7422f78aa...`
*-riscv64-linux-gnu.tar.gz	`e097d9332be88ef4...`	`44207e53859a7821...`
*-win64-codesigning.tar.gz	`d4135b3495c68457...`	`0c7c56e0c7766f6c...`
*-win64-debug.zip	`6d0dad6f35d48310...`	`4194af6c81e362db...`
*-win64-setup-unsigned.exe	`e1d25ad01a7a4f10...`	`f20a6f7e0f605283...`
*-win64-unsigned.zip	`88a96661a9f51a05...`	`22c88a16c97c0f66...`
*-x86_64-apple-darwin-codesigning.tar.gz	`371cb8c51da31987...`	`b50fe31748ee2267...`
*-x86_64-apple-darwin-unsigned.tar.gz	`b5cf1dd2abc032c8...`	`682bbd0d43538025...`
*-x86_64-apple-darwin-unsigned.zip	`658c69b4d293e9db...`	`70295edf8024223e...`
*-x86_64-linux-gnu-debug.tar.gz	`7f893e0ece08a3d2...`	`0abf5dd40f9a7695...`
*-x86_64-linux-gnu.tar.gz	`e90434b160e05674...`	`7ded5d664acb3d5f...`
*.tar.gz	`c9e46fbe8fd423f5...`	`bf28c284c0f53b7e...`
SHA256SUMS.part	`21f78bd45146f5b8...`	`1c2a42d485f9a751...`
guix_build.log	`00500b67f7e4cc1b...`	`f66c28868bf3e26b...`
guix_build.log.diff		`f4aa40351d99b50d...`

DrahtBot removed the label DrahtBot Guix build requested on Jul 21, 2026

cmake: Unconditionally set WIN32_EXECUTABLE target property

The property only has an effect when building for WIN32.
Checking for WIN32 before setting the property is redundant.

37e375861d

cmake: Use builtin support for .manifest files

CMake ignores .rc and .manifest files when not building for WIN32.

06ee5382e1

cmake: Unconditionally add .rc files to sources

CMake ignores .rc files when compiling for non-Windows platform.
Checking for WIN32 before adding an .rc file to sources is redundant.

13e6429a7d

purpleKarrot force-pushed on Jul 27, 2026

purpleKarrot commented at 2:34 PM on July 27, 2026: contributor

@purpleKarrot can you rebase this?

Done.

151henry151 commented at 9:16 PM on July 27, 2026: contributor

Tested ACK 13e6429a7d2fa258e6dac270a414c4b48a39e64c

Reviewed the three commits. Linux Release build is fine with the unconditional .rc / generated *-manifest.rc sources (CMake does not compile them on non-WIN32). CI mingw artifacts and a local Guix HOSTS=x86_64-w64-mingw32 build both pass APPLICATION_MANIFEST / symbol-check for the main apps, including bitcoin-qt and test_bitcoin. Dumped Guix and CI mingw PE manifests keep activeCodePage UTF-8, asInvoker, and assemblyIdentity from windows-app.manifest.in.

On the MSVC path, Windows native VS CI’s check_manifests on this tip (run 30275700389) dumps the embedded bitcoind.exe manifest via mt.exe (other exes are only -validate_manifest’d). That dump contains exactly our windows-app.manifest.in entries — assemblyIdentity, asInvoker, UTF-8 activeCodePage — with no linker-added sections in this configuration (expected — the modern CRT isn’t a SxS assembly, so the default linker manifest overlaps our template). That’s consistent with purpleKarrot’s Feb 9 explanation and the CMake 3.4 release-note behavior (custom manifests merge with linker-generated ones). The documented MinGW .rc workaround for CMake #23244 looks like the right tradeoff versus a single .rc path for both toolchains.

Guix build (<code>HOSTS=x86_64-w64-mingw32</code>)

x86_64
c56f846e1411e094ac9757ab781a7a3819d354c476f5fa22f9ff4a1f9d1d6e25  guix-build-13e6429a7d2f/output/dist-archive/bitcoin-13e6429a7d2f.tar.gz
2b81e4a1ad4bb46cc8953dcd4394d7793ac9e063735526cac5519bd14d339840  guix-build-13e6429a7d2f/output/x86_64-w64-mingw32/SHA256SUMS.part
7aa1e645cf4c00307aa3b37dca66b7e58d0f38de6f8e6334bba6b013aa6eee38  guix-build-13e6429a7d2f/output/x86_64-w64-mingw32/bitcoin-13e6429a7d2f-win64-codesigning.tar.gz
87451bc502e9f7d26846209d975e0209bb9843725a91fe00b8b2f7c26880d9a1  guix-build-13e6429a7d2f/output/x86_64-w64-mingw32/bitcoin-13e6429a7d2f-win64-debug.zip
82aeefc04ab22c96046790a311b90745854c82382504e13b91f8a084dd4d2d81  guix-build-13e6429a7d2f/output/x86_64-w64-mingw32/bitcoin-13e6429a7d2f-win64-setup-unsigned.exe
85346641763ec0c6dd8cba3d29bdb16ce9e30ef2e4836254dcb62691571bad6e  guix-build-13e6429a7d2f/output/x86_64-w64-mingw32/bitcoin-13e6429a7d2f-win64-unsigned.zip