Skip to content

build: improve cxx and linker flag caching #30732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

build: improve cxx and linker flag caching #30732

wants to merge 4 commits into from

Conversation

l0rinc
Copy link
Contributor

@l0rinc l0rinc commented Aug 28, 2024 •
edited
Loading

Migration of hebasto#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:

-- Performing Test CXX_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER
-- Performing Test CXX_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER - Success
-- Performing Test LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER__WL__FATAL_WARNINGS_A797852B32447D9D3594B7F06EFE65790DAAEFCBEFCE4144D7B1F480F6FA4B6A
-- Performing Test LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER__WL__FATAL_WARNINGS_A797852B32447D9D3594B7F06EFE65790DAAEFCBEFCE4144D7B1F480F6FA4B6A - Success
-- Performing Test FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION
-- 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
Copy link
Contributor

DrahtBot commented Aug 28, 2024 •
edited
Loading

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 l0rinc force-pushed the l0rinc/hash-collisions branch from 516e8d2 to 645809d Compare August 28, 2024 13:07
@l0rinc l0rinc mentioned this pull request Aug 28, 2024
Closed
@l0rinc l0rinc changed the title cmake: Improve cxx and linker flag caching cmake: improve cxx and linker flag caching Aug 28, 2024
@maflcko maflcko closed this Aug 28, 2024
@maflcko maflcko reopened this Aug 28, 2024
@maflcko
Copy link
Member

maflcko commented Aug 28, 2024

(sorry, wrong button)

@l0rinc l0rinc force-pushed the l0rinc/hash-collisions branch from 645809d to 4f06b02 Compare August 28, 2024 15:25
@l0rinc l0rinc marked this pull request as ready for review August 29, 2024 10:38
@fanquake fanquake requested a review from hebasto August 29, 2024 13:46
@DrahtBot
Copy link
Contributor

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

File commit 4ee1940
(master)		 commit b7c6b79
(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...

@l0rinc l0rinc changed the title cmake: improve cxx and linker flag caching build: improve cxx and linker flag caching Aug 29, 2024
@DrahtBot DrahtBot mentioned this pull request Sep 8, 2024
Merged
@l0rinc l0rinc force-pushed the l0rinc/hash-collisions branch from 4f06b02 to 7c1640c Compare September 11, 2024 20:27
@l0rinc l0rinc force-pushed the l0rinc/hash-collisions branch from 7c1640c to bd0053e Compare September 12, 2024 09:40
@l0rinc
Unify try_append_linker_flag naming with try_append_cxx_flags
c133c4a
@l0rinc
Remove unused SOURCE from try_append_cxx_flags
e6a2054
@l0rinc
Use full source hash when try_append_linker_flag SOURCE is specified
de3ee34
@l0rinc
Include the *werror_flag in try_append_linker_flag cache
2a97482 
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)
@l0rinc l0rinc force-pushed the l0rinc/hash-collisions branch from bd0053e to 2a97482 Compare September 13, 2024 07:25
@hebasto
Copy link
Member

hebasto commented Sep 16, 2024

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?

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

l0rinc commented Sep 16, 2024 •
edited
Loading

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

cc @hodlinator

@l0rinc l0rinc closed this Jan 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hebasto hebasto Awaiting requested review from hebasto

@theuni theuni Awaiting requested review from theuni

@m3dwards m3dwards Awaiting requested review from m3dwards

@fanquake fanquake Awaiting requested review from fanquake

Assignees
No one assigned
Labels
Build system
Projects
No open projects
CMake
Status: CMake follow-ups
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@l0rinc @DrahtBot @maflcko @hebasto @achow101