Skip to content

Conversation

purpleKarrot
Copy link
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
Copy link
Contributor

DrahtBot commented Oct 9, 2025

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 for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #33455 (CPack by purpleKarrot)
  • #32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
  • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
  • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
  • #10102 (Multiprocess bitcoin by ryanofsky)

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
Copy link
Member

fanquake commented Oct 9, 2025

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

The property only has an effect when building for WIN32.
Checking for WIN32 before setting the property is redundant.
CMake ignores .rc and .manifest files when not building for WIN32.
CMake ignores .rc files when compiling for non-Windows platform.
Checking for WIN32 before adding an .rc file to sources is redundant.
@purpleKarrot
Copy link
Contributor Author

This doesn't Guix build:

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

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Member

hebasto commented Oct 10, 2025

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:

  1. 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?

  2. 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants