cmake: Include the *werror_flag in TryAppend*Flags compiler cache

[l0rinc]

Trigger recompilation when working_linker_werror_flagor working_compiler_werror_flag are changed since it can affect the outcome.

See: https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711409392

[hebasto]

Please rebase. The staging branch has been just updated in https://github.com/hebasto/bitcoin/pull/318.

[l0rinc]

Please rebase. The staging branch has been just updated in #318.

done

[hebasto]

Trigger recompilation when working_linker_werror_flagor working_compiler_werror_flag are changed

What are the use cases for that?

[l0rinc]

Compiling the same code with different flags

[hebasto]

Compiling the same code with different flags

Flags that make a compiler/linker treat warnings as errors are specific to a particular toolchain. It does not depend on the user, does it?

[l0rinc]

I'm not exactly sure what you're referring to. This fixed my observation about https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711409392 where adding the -no_warn_duplicate_libraries flag in the source code only fixed fuzzing after I manually deleted the build_fuzz folder (i.e. compilation wasn't reattempeted with different flags). After this PR these kinds of edits are detected and the cache is properly invalidated.

[hebasto]

The build system is not intended to be modified by regular users or developers unless they are working on the build system itself. Users can modify compiler and linker flags using standard CMake variables or, in special cases, project-specific APPEND_*FLAGS.

[l0rinc]

unless they are working on the build system itself

Yes, that's what happened to me, hence this PR.

I have also opened https://gitlab.kitware.com/cmake/cmake/-/issues/26207, though I still think the issue is on our side, we should include all inputs into the cache key (if we want proper cache invalidation), not just the source code.

[maflcko]

What are the exact steps to reproduce? Linking to a thread with tens of comments (hidden by Github) is useful, but not a replacment to explain what you are trying to fix. Without an explanation, it is harder to review this or to motivate to review this.

[l0rinc]

What are the exact steps to reproduce?

While trying to understand why fuzzing wasn't working with cmake on my mac, I have noticed that changing

try_append_linker_flag("-Wl,-fatal_warnings" VAR working_linker_werror_flag)

to e.g.

try_append_linker_flag("-Wl,-no_warn_duplicate_libraries,-fatal_warnings" VAR working_linker_werror_flag)

i.e. adding -no_warn_duplicate_libraries to the working_linker_werror_flag didn't retrigger the previous failing compilation - but deleting the build folder and retrying did (see https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710280041).

[l0rinc]

@pablomartin4btc, thanks for reviewing!

To be able to reproduce it, try to fuzz again by first reproducing the error, after which don't manually invalidate any cache (e.g. by deleting the build_fuzz folder), but rather:

diff --git a/cmake/module/TryAppendLinkerFlag.cmake b/cmake/module/TryAppendLinkerFlag.cmake
--- a/cmake/module/TryAppendLinkerFlag.cmake	(revision 687a10ba93917646440993804998deaf58ae3f6b)
+++ b/cmake/module/TryAppendLinkerFlag.cmake	(date 1724428115452)
@@ -79,7 +79,7 @@
 if(MSVC)
   try_append_linker_flag("/WX" VAR working_linker_werror_flag)
 elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
-  try_append_linker_flag("-Wl,-fatal_warnings" VAR working_linker_werror_flag)
+  try_append_linker_flag("-Wl,-no_warn_duplicate_libraries,-fatal_warnings" VAR working_linker_werror_flag)
 else()
   try_append_linker_flag("-Wl,--fatal-warnings" VAR working_linker_werror_flag)
 endif()

and rerun the

cmake -B build_fuzz -DCMAKE_C_COMPILER="clang" -DCMAKE_CXX_COMPILER="clang++" -DBUILD_FOR_FUZZING=ON -DSANITIZERS=undefined,address,fuzzer

command with/without the caching fix introduces in this commit. It should still fail first, but if you delete the build folder and rerun, it should pass, demonstrating that working_linker_werror_flag should be part of the cache key as an input for proper automatic cache invalidation.

[pablomartin4btc]

Ok, if I repro de error, update cmake/module/TryAppendLinkerFlag.cmake and rerun, it passes. Now, with this PR/ branch, even if it fails the first time, deleting the folder and rerunning it not passes. What I'm doing wrong here?

[l0rinc]

Could you please share the commands and diff you were using?

[pablomartin4btc]

Could you please share the commands and diff you were using?

git clone --depth=1 --branch l0rinc/werror_flag https://github.com/l0rinc//bitcoin.git bitcoin-cmake
cd bitcoin-make
cmake -B build_fuzz -DCMAKE_C_COMPILER="clang" -DCMAKE_CXX_COMPILER="clang++" -DBUILD_FOR_FUZZING=ON -DSANITIZERS=undefined,address,fuzzer
...
-- 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_339c
-- Performing Test LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER_339c - Failed
CMake Error at CMakeLists.txt:393 (message):
  Linker did not accept requested flags, you are missing required libraries.


-- Configuring incomplete, errors occurred!

rm -rf build_fuzz

cmake -B build_fuzz -DCMAKE_C_COMPILER="clang" -DCMAKE_CXX_COMPILER="clang++" -DBUILD_FOR_FUZZING=ON -DSANITIZERS=undefined,address,fuzzer
...
-- 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_339c
-- Performing Test LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER_339c - Failed
CMake Error at CMakeLists.txt:393 (message):
  Linker did not accept requested flags, you are missing required libraries.


-- Configuring incomplete, errors occurred!

[l0rinc]

The PR itself does not directly fix the issue of Linker did not accept requested flags. Instead, it addresses the problem where even after applying the fix (i.e., -no_warn_duplicate_libraries), it only worked if the cache was invalidated (i.e., by deleting the build_fuzz folder).

After this change, applying the fix (as shown in the diff above) should automatically invalidate the cache and work without needing to delete te build_fuzz folder.

[pablomartin4btc]

Ok got you now, thanks!

I still had to re-read the PR to find some important statements (eg "After this PR these kinds of edits are detected and the cache is properly invalidated.").

So still we need to change try_append_linker_flag adding -no_warn_duplicate_libraries. Perhaps you need to clarify it in the description of the PR, now it obvious to me too but got confused, sorry, and that this happens only on macOS (so far) or you need to change the try_append_linker_flag line for the specific platform.

If I do that (try_append*) in cmake_staging or #30454, without making it fail first, it compiles, but sure you don't in advance if that's going to happend and I see the only way there, if it fails first, it's deleting the build folder (also perhaps adding it to the PR description).

Since I'm here I'm checking some runtime errors I got on macOS running the fuzz so I need to check it against autotools and other OSs.

[l0rinc]

Thanks @pablomartin4btc, updated the readme.

[l0rinc]

Might be invalidated by https://github.com/hebasto/bitcoin/pull/340

[hebasto]

Since https://github.com/bitcoin/bitcoin/pull/30454 has been merged, further development in this repository no longer makes sense. Please consider moving this PR to the main repository.