build: improve cxx and linker flag caching

l0rinc commented at 1:05 pm on August 28, 2024: contributor

Migration of https://github.com/hebasto/bitcoin/pull/340

try_append_cxx_flags and try_append_linker_flag are CMake functions used to test and conditionally apply compiler and linker flags to ensure compatibility with the build environment.

Currently the only case when the SOURCE parameter was overwritten was bitcoin/bitcoin@8b6f1c4/CMakeLists.txt#L383-L389. So currently the values would be unique without the hash, but overwriting a source in the future would cause a false hit, so I kept the full (uppercase) hash - and also added the werror_flag to the cache key, since the result of the compilation depends on it as well.

After this change we will have logs such as:

0-- Performing Test CXX_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER
1-- Performing Test CXX_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER - Success
2-- Performing Test LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER__WL__FATAL_WARNINGS_A797852B32447D9D3594B7F06EFE65790DAAEFCBEFCE4144D7B1F480F6FA4B6A
3-- Performing Test LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER__WL__FATAL_WARNINGS_A797852B32447D9D3594B7F06EFE65790DAAEFCBEFCE4144D7B1F480F6FA4B6A - Success
4-- Performing Test FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION
5-- Performing Test FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION - Success

On top of that, try_append_cxx_flags currently doesn’t even need a SOURCE parameter - I’ve removed it for now, we can add it back once we need it.

DrahtBot commented at 1:05 pm on August 28, 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. A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

l0rinc force-pushed on Aug 28, 2024

l0rinc renamed this:
~~cmake: Improve cxx and linker flag caching~~
cmake: improve cxx and linker flag caching
on Aug 28, 2024

maflcko closed this on Aug 28, 2024

maflcko reopened this on Aug 28, 2024

maflcko commented at 1:20 pm on August 28, 2024: member

(sorry, wrong button)

l0rinc force-pushed on Aug 28, 2024

maflcko added the label DrahtBot Guix build requested on Aug 29, 2024

l0rinc marked this as ready for review on Aug 29, 2024

fanquake requested review from hebasto on Aug 29, 2024

DrahtBot commented at 6:01 pm on August 29, 2024: contributor

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

File	commit 4ee1940e849efe8fb1510b11d78399231df4b578(master)	commit b7c6b795476238f24a8b07c90f68a2ec44d997ef(master and this pull)
SHA256SUMS.part	`f3173a955837e725...`	`1d11514803e4c6d1...`
*-aarch64-linux-gnu-debug.tar.gz	`ca91ba66849cd58f...`	`cf6050c858455dd7...`
*-aarch64-linux-gnu.tar.gz	`5f891b41807d5124...`	`a2d2fd5625d5361e...`
*-arm-linux-gnueabihf-debug.tar.gz	`5a7f53b22d19af93...`	`4d898b3581347953...`
*-arm-linux-gnueabihf.tar.gz	`3cebdcfaab00b068...`	`9537815dc1f07176...`
*-arm64-apple-darwin-unsigned.tar.gz	`dbfb6b46a8ba5601...`	`3da37f7bfbb772c9...`
*-arm64-apple-darwin-unsigned.zip	`bbf23c3e0a6bb897...`	`3c8f6389886c8a22...`
*-arm64-apple-darwin.tar.gz	`fcf2ac36cb51fc58...`	`6212f651d9e43c2f...`
*-powerpc64-linux-gnu-debug.tar.gz	`5384d0a1095cecac...`	`10dbdadfdfd53a88...`
*-powerpc64-linux-gnu.tar.gz	`0c1db8af0050158d...`	`a43d5a1eb7f2d56d...`
*-riscv64-linux-gnu-debug.tar.gz	`662af641aae75368...`	`407ce93fd51e83cf...`
*-riscv64-linux-gnu.tar.gz	`4a1d77dfe3bf89ea...`	`f5e6d4f3a25dcf83...`
*-x86_64-apple-darwin-unsigned.tar.gz	`60cf6ab775c9521a...`	`ec47e86a151aacd2...`
*-x86_64-apple-darwin-unsigned.zip	`995e263afed6fad5...`	`d0de2c04d102e952...`
*-x86_64-apple-darwin.tar.gz	`71db56e0ac166e7b...`	`679a46336332f1f2...`
*-x86_64-linux-gnu-debug.tar.gz	`989ef754130c891f...`	`a535ce880e806232...`
*-x86_64-linux-gnu.tar.gz	`122343653b28d0df...`	`d92b05239ca57558...`
*.tar.gz	`2703b1ee9e245685...`	`642ff1b5b76a4320...`
guix_build.log	`e0e95dae7d26fe3d...`	`34d8d5f977039059...`
guix_build.log.diff		`e09eaf7b95412c9f...`

DrahtBot removed the label DrahtBot Guix build requested on Aug 29, 2024

DrahtBot added the label Build system on Aug 29, 2024

l0rinc renamed this:
~~cmake: improve cxx and linker flag caching~~
build: improve cxx and linker flag caching
on Aug 29, 2024

l0rinc force-pushed on Sep 11, 2024

DrahtBot added the label CI failed on Sep 12, 2024

l0rinc force-pushed on Sep 12, 2024

Unify try_append_linker_flag naming with try_append_cxx_flags c133c4a17d

Remove unused SOURCE from try_append_cxx_flags e6a2054eda

Use full source hash when try_append_linker_flag SOURCE is specified de3ee34b50

Include the *werror_flag in try_append_linker_flag cache

We'll get keys such as:
> LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER__WL__FATAL_WARNINGS_A797852B32447D9D3594B7F06EFE65790DAAEFCBEFCE4144D7B1F480F6FA4B6A

This change will trigger a recompilation when `working_linker_werror_flag` are changed (instead of a false cache hit)

2a97482f78

l0rinc force-pushed on Sep 13, 2024

DrahtBot removed the label CI failed on Sep 13, 2024

hebasto commented at 3:41 pm on September 16, 2024: member

Adding a source hash to the cache variable name became necessary at some point during the development of the CMake staging branch, as checks for the same flags could occur with different sources in a single cmake invocation. Is this scenario possible in the master branch? If not, perhaps this functionality can be dropped?

2a97482f7892ee2b55955850c92ffd985a58c0a0 I’m still not convinced about the last commit, as the situation “when working_linker_werror_flag are changed” never occurs during the regular use of the build system. Imposing a general requirement that the build system must refresh itself properly after any modification the build system itself seems unusual. The modified build system is effectively a different build system and should start with a clean environment.

l0rinc commented at 3:57 pm on September 16, 2024: contributor

Thanks for checking @hebasto!

Is this scenario possible in the master branch? If not, perhaps this functionality can be dropped?

There’s only a single case where SOURCE is defined now - in which case the related flags were also unique: LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER__WL__FATAL_WARNINGS_

We could drop the hash, but that means that whoever calls that method again has to change the implementation, since it’s not safe to call it without knowing who else called it, right?

I’m still not convinced about the last commit, as the situation “when working_linker_werror_flag are changed” never occurs during the regular use of the build system.

Can’t this occur on branch changes as well, if one of them has a modification in this area? I’m not usually cleaning the caches between branch changes…

achow101 requested review from theuni on Oct 15, 2024

achow101 requested review from m3dwards on Oct 15, 2024

achow101 requested review from fanquake on Oct 15, 2024

achow101 commented at 4:01 pm on October 15, 2024: member

cc @hodlinator

build: improve cxx and linker flag caching #30732

Code Coverage

Reviews

Conflicts

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