Remove some redundant logic from the CMake code:
WIN32_EXECUTABLE target property only has an effect when building for WIN32. Checking WIN32 is redundant..rc and .manifest files. Both may be added to sources unconditionally. They only have an effect when building for WIN32.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33585.
See the guideline for information on the review process. A summary of reviews will appear here.
Reviewers, this pull request conflicts with the following ones:
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.
This doesn’t Guix build:
0[100%] Built target bitcoin-qt
1Running symbol and dynamic library checks...
2/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin.exe: failed APPLICATION_MANIFEST
3/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoind.exe: failed APPLICATION_MANIFEST
4/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin-qt.exe: failed APPLICATION_MANIFEST
5/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin-cli.exe: failed APPLICATION_MANIFEST
6/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin-tx.exe: failed APPLICATION_MANIFEST
7/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin-util.exe: failed APPLICATION_MANIFEST
8/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/bitcoin-wallet.exe: failed APPLICATION_MANIFEST
9/distsrc-base/distsrc-3277798895d9-x86_64-w64-mingw32/build/bin/test_bitcoin.exe: failed APPLICATION_MANIFEST
10make[3]: *** [CMakeFiles/check-symbols.dir/build.make:71: CMakeFiles/check-symbols] Error 1
11make[2]: *** [CMakeFiles/Makefile2:388: CMakeFiles/check-symbols.dir/all] Error 2
12make[1]: *** [CMakeFiles/Makefile2:395: CMakeFiles/check-symbols.dir/rule] Error 2
13make: *** [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.
This doesn’t Guix build:
I added a workaround for upstream issue 23244. It should pass now.
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
.manifestfiles?
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
Is there any CMake’s docs regarding handling
.manifestfiles?
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?