[rfc] build: Reject unclean configure?

maflcko commented at 8:38 am on February 24, 2025: member

When re-configuring a build, it is possible to do so in an unclean way on a stale build folder.

The average person initiating such a build does not know when a stale build is safe and when not. Time is wasted for the builder when a broken build silently succeeds and then leads to a nonsensical error later on. Just to give the last two examples of the last few days: (https://github.com/bitcoin/bitcoin/issues/31922#issuecomment-2673888781, #31933 (review), …)

The benefit of an unclean build is unclear, given that it merely saves a few seconds of a build that normally happens in the background anyway, at the risk of wasting minutes or hours if something is overlooked.

Thus, a build configure should be rejected if the build directory exists.

(Obviously, re-builds on a build directory that do not require a re-configure are not affected)

maflcko added the label Brainstorming on Feb 24, 2025

maflcko added the label Build system on Feb 24, 2025

maflcko commented at 3:03 pm on February 26, 2025: member

cc @ryanofsky I believe this is an alternative way to ease your concern from #31161#pullrequestreview-2644640989, assuming that 31161 triggers a re-configure.

ryanofsky commented at 7:19 pm on February 26, 2025: contributor

cc @ryanofsky I believe this is an alternative way to ease your concern from #31161 (review), assuming that 31161 triggers a re-configure.

Thanks, this is interesting and I would agree that if we had a mechanism that would make it an error for the configure step to run after any build step had run, that would make the concern I had in #31161 moot. However, I think that change would also break use-cases I think are useful, like being able to update and switch branches and run make. I’m not sure exactly what changes you might have in mind though, and if there are other ways of detecting “unclean” configurations.

I do think the case where you switch branches while keeping the build is different from the case where you change cmake options while keeping the build. I would expect the former to basically always work and only the latter to be unreliable.

hodlinator commented at 7:54 am on February 27, 2025: contributor

Maybe one could have a I_KNOW_WHAT_I_AM_DOING environment variable for developers knowledgeable with the intricacies of the build process, that disables the default-enabled error for this?

maflcko commented at 2:17 pm on February 27, 2025: member

Yeah, I’d say a simple dev-only test-only -DALLOW_CONFIGURE_ON_UNCLEAN_BUILD_DIR=ON should address any concern where someone really does want to do it. The option could be mentioned in the error message that rejects an unclean build.

However, for most other developers (or non-developers), the option would be turned OFF by default and then hopefully reduce the wasted time and bug reports. (Another report from today: #31959 (comment) …)

maflcko commented at 3:52 pm on February 27, 2025: member

@ryanofsky The diff I am proposing would be:

 0diff --git a/CMakeLists.txt b/CMakeLists.txt
 1index 94464907de..79ea015b78 100644
 2--- a/CMakeLists.txt
 3+++ b/CMakeLists.txt
 4@@ -126,6 +126,16 @@ cmake_dependent_option(BUILD_WALLET_TOOL "Build bitcoin-wallet tool." ${BUILD_TE
 5 
 6 option(ENABLE_HARDENING "Attempt to harden the resulting executables." ON)
 7 option(REDUCE_EXPORTS "Attempt to reduce exported symbols in the resulting executables." OFF)
 8+
 9+option(ALLOW_CONFIGURE_ON_UNCLEAN_BUILD_DIR "Allow configure on a build directory that was already configured previously." OFF)
10+set(BUILD_MARKER_FILE "${CMAKE_BINARY_DIR}/.mark_configure")
11+if(EXISTS "${BUILD_MARKER_FILE}")
12+  if(NOT ALLOW_CONFIGURE_ON_UNCLEAN_BUILD_DIR)
13+    message(FATAL_ERROR "The build directory exists. This can lead to difficult to debug issues later on, so it is recommended to remove the build directory and start a clean build. If you really want to bypass this error, you can set -DALLOW_CONFIGURE_ON_UNCLEAN_BUILD_DIR=ON.")
14+  endif()
15+endif()
16+file(WRITE "${BUILD_MARKER_FILE}" "_")
17+
18 option(WERROR "Treat compiler warnings as errors." OFF)
19 option(WITH_CCACHE "Attempt to use ccache for compiling." ON)
20

purpleKarrot commented at 3:52 pm on February 28, 2025: contributor

Concept NACK. Please don’t introduce hacks like this just to hide the underlying problems.

The necessity to nuke the build directory and do a clean configure should be considered a bug in the project. That bug should be identified and fixed instead of enforcing to nuke the build directory over and over during a rebase or after switching branches.

Maybe one could have a I_KNOW_WHAT_I_AM_DOING environment variable …

Working on multiple cmake projects in parallel, it would be nice if all projects supported the same workflow, so knowledge acquired in one place could be applied everywhere else. But this only works as long as no project introduces their project specific hacks and workarounds with good (but naive) intentions like “The average person … does not know …”.

hodlinator commented at 9:27 am on March 5, 2025: contributor

@purpleKarrot maybe you could provide guidance on how to properly solve issues like #31959 or one of the others the project is experiencing, which lead to this RFC?

hebasto commented at 12:11 pm on March 5, 2025: member

@purpleKarrot maybe you could provide guidance on how to properly solve issues like #31959 or one of the others the project is experiencing, which lead to this RFC?

It would also be helpful if someone provided reliable steps to reproduce those issues.

fanquake commented at 1:06 pm on March 6, 2025: member

reliable steps to reproduce those issues.

For example, configuring with GCC + thread sanitizer (where libtsan is missing). After installing the missing dependency, and re-running CMake, even though the check is being re-run, CMake doesn’t “fix” itself, and requires removing the build directory entirely. i.e:

 0cmake -B build -DSANITIZERS=thread
 1-- The CXX compiler identification is GNU 14.2.1
 2-- Detecting CXX compiler ABI info
 3-- Detecting CXX compiler ABI info - done
 4-- Check for working CXX compiler: /usr/bin/c++ - skipped
 5-- Detecting CXX compile features
 6-- Detecting CXX compile features - done
 7-- Setting build type to "RelWithDebInfo" as none was specified
 8-- Performing Test CXX_SUPPORTS__WERROR
 9-- Performing Test CXX_SUPPORTS__WERROR - Success
10-- Performing Test CXX_SUPPORTS__G3
11-- Performing Test CXX_SUPPORTS__G3 - Success
12-- Performing Test LINKER_SUPPORTS__G3
13-- Performing Test LINKER_SUPPORTS__G3 - Success
14-- Performing Test CXX_SUPPORTS__FTRAPV
15-- Performing Test CXX_SUPPORTS__FTRAPV - Success
16-- Performing Test LINKER_SUPPORTS__FTRAPV
17-- Performing Test LINKER_SUPPORTS__FTRAPV - Success
18-- Found SQLite3: /usr/include (found suitable version "3.46.1", minimum required is "3.7.17")
19-- Performing Test LINKER_SUPPORTS__WL___FATAL_WARNINGS
20-- Performing Test LINKER_SUPPORTS__WL___FATAL_WARNINGS - Success
21-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
22-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
23-- Found Threads: TRUE
24-- Performing Test CXX_SUPPORTS__FSANITIZE_THREAD
25-- Performing Test CXX_SUPPORTS__FSANITIZE_THREAD - Success
26-- Performing Test LINKER_SUPPORTS__FSANITIZE_THREAD_a797
27-- Performing Test LINKER_SUPPORTS__FSANITIZE_THREAD_a797 - Failed
28CMake Error at CMakeLists.txt:381 (message):
29  Linker did not accept requested flags, you are missing required libraries.
30
31
32-- Configuring incomplete, errors occurred!
33# dnf install libtsan
34Updating and loading repositories:
35Repositories loaded.
36Package                                                                                             Arch                  Version                                                                                             Repository                                                   Size
37Installing:
38 libtsan                                                                                            aarch64               14.2.1-7.fc41                                                                                       updates                                                   1.3 MiB
39
40Transaction Summary:
41 Installing:         1 package
42
43Total size of inbound packages is 423 KiB. Need to download 423 KiB.
44After this operation, 1 MiB extra will be used (install 1 MiB, remove 0 B).
45Is this ok [y/N]: y
46[1/1] libtsan-0:14.2.1-7.fc41.aarch64                                                                                                                                                                                                                  100% |   3.2 MiB/s | 423.3 KiB |  00m00s
47-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
48[1/1] Total                                                                                                                                                                                                                                            100% | 620.7 KiB/s | 423.3 KiB |  00m01s
49Running transaction
50[1/3] Verify package files                                                                                                                                                                                                                             100% |   1.0 KiB/s |   1.0   B |  00m00s
51[2/3] Prepare transaction                                                                                                                                                                                                                              100% |  76.0   B/s |   1.0   B |  00m00s
52[3/3] Installing libtsan-0:14.2.1-7.fc41.aarch64                                                                                                                                                                                                       100% |  51.1 MiB/s |   1.3 MiB |  00m00s
53Complete!
54# cmake -B build -DSANITIZERS=thread
55CMake Error at CMakeLists.txt:381 (message):
56  Linker did not accept requested flags, you are missing required libraries.
57
58
59-- Configuring incomplete, errors occurred!
60# rm -rf build
61# cmake -B build -DSANITIZERS=thread
62-- The CXX compiler identification is GNU 14.2.1
63-- Detecting CXX compiler ABI info
64-- Detecting CXX compiler ABI info - done
65-- Check for working CXX compiler: /usr/bin/c++ - skipped
66-- Detecting CXX compile features
67-- Detecting CXX compile features - done
68-- Setting build type to "RelWithDebInfo" as none was specified
69-- Performing Test CXX_SUPPORTS__WERROR
70-- Performing Test CXX_SUPPORTS__WERROR - Success
71-- Performing Test CXX_SUPPORTS__G3
72-- Performing Test CXX_SUPPORTS__G3 - Success
73-- Performing Test LINKER_SUPPORTS__G3
74-- Performing Test LINKER_SUPPORTS__G3 - Success
75-- Performing Test CXX_SUPPORTS__FTRAPV
76-- Performing Test CXX_SUPPORTS__FTRAPV - Success
77-- Performing Test LINKER_SUPPORTS__FTRAPV
78-- Performing Test LINKER_SUPPORTS__FTRAPV - Success
79-- Found SQLite3: /usr/include (found suitable version "3.46.1", minimum required is "3.7.17")
80-- Performing Test LINKER_SUPPORTS__WL___FATAL_WARNINGS
81-- Performing Test LINKER_SUPPORTS__WL___FATAL_WARNINGS - Success
82-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
83-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
84-- Found Threads: TRUE
85-- Performing Test CXX_SUPPORTS__FSANITIZE_THREAD
86-- Performing Test CXX_SUPPORTS__FSANITIZE_THREAD - Success
87-- Performing Test LINKER_SUPPORTS__FSANITIZE_THREAD_a797
88-- Performing Test LINKER_SUPPORTS__FSANITIZE_THREAD_a797 - Success

purpleKarrot commented at 10:55 pm on March 6, 2025: contributor

even though the check is being re-run

No, the check is not rerun, the result is cached.

In this particular case, where check_cxx_source_compiles is used not to check for compiler capability, but for checking the availability of dependencies, where it is expected that users will want to rerun the check after fixing the problem, it may make sense to reset the cache entry by adding the following in TryAppendLinkerFlag.cmake:

0if(NOT ${result})
1  unset(${result} CACHE)
2endif()

Note that the inverse is possible too: Users could remove necessary dependencies after their existence has been checked and cached. Trying to protect the project against those cases means questioning the concept of a cache as such.

hebasto commented at 6:56 am on March 7, 2025: member

even though the check is being re-run

No, the check is not rerun, the result is cached.

In this particular case, where check_cxx_source_compiles is used not to check for compiler capability, but for checking the availability of dependencies, where it is expected that users will want to rerun the check after fixing the problem, it may make sense to reset the cache entry by adding the following in TryAppendLinkerFlag.cmake:

if(NOT ${result}) unset(${result} CACHE) endif()

Note that the inverse is possible too: Users could remove necessary dependencies after their existence has been checked and cached. Trying to protect the project against those cases means questioning the concept of a cache as such.

FWIW, the find_package() command creates cache variables when it succeeds. If the dependency package is later removed from the system, a subsequent cmake re-run won’t detect the change.

fanquake commented at 10:17 am on March 7, 2025: member

No, the check is not rerun, the result is cached.

Sure, but from the perspective of the builder, if it’s producing output from the check, they’ll likely think it is being re-run, and shortly after they’ll open an issue in our repo, asking why the build still doesn’t work, even after they’ve fixed the missing dependency (given no guidance that the build directory should be removed).

I don’t think it’s useful for CMake to cache results that will leave a user with an always failing build, so agree that we should do something here.

purpleKarrot commented at 7:43 pm on March 7, 2025: contributor

As @hebasto pointed out, the find_package() command creates cache variables when it succeeds. I think this is the behaviour we want for TryAppendLinkerFlag.cmaketoo, which means the cache variable should be dropped on failure.

hebasto commented at 4:58 pm on March 10, 2025: member

reliable steps to reproduce those issues.

For example, configuring with GCC + thread sanitizer (where libtsan is missing). After installing the missing dependency, and re-running CMake, even though the check is being re-run, CMake doesn’t “fix” itself, and requires removing the build directory entirely.

Fixed in #32027.

maflcko commented at 1:54 pm on March 16, 2025: member

Users could remove necessary dependencies after their existence has been checked and cached. Trying to protect the project against those cases means questioning the concept of a cache as such.

FWIW, the find_package() command creates cache variables when it succeeds. If the dependency package is later removed from the system, a subsequent cmake re-run won’t detect the change.

If the cache is known to possibly result in a (silent) break on any change to installed packages, it just doesn’t seem worth it. I suspect that updating packages (c.f. #31959 (comment)) happens daily or weekly, so I fail to see how anyone building equally often can even benefit from the cache.

Anyone building several times a day and really wanting to use the cache can trivially set ALLOW_CONFIGURE_ON_UNCLEAN_BUILD_DIR.

If ccache was similarly broken and could fail on any package change or update, it’d be unacceptable (at least to me).

maflcko commented at 1:26 pm on March 18, 2025: member

Another instance where a stale cmake cache caused issues after a brew? update: #32084 (comment)

hebasto commented at 7:44 pm on March 19, 2025: member

FWIW, at the end of configuration, Qt 6 prints:

0If reconfiguration fails for some reason, try removing 'CMakeCache.txt' from the build directory
1Alternatively, you can add the --fresh flag to your CMake flags.

maflcko commented at 8:11 pm on March 19, 2025: member

If reconfiguration fails for some reason, try removing ‘CMakeCache.txt’ from the build directory Alternatively, you can add the –fresh flag to your CMake flags.

I don’t think I’ve seen issues where re-configuration fails. So far the linked issues here were about linker errors.

[rfc] build: Reject unclean configure? #31942