Re: [PATCH] Makefile: fix parallel build race

From

Đoàn Trần Công Danh <congdanhqx@gmail.com>

Date

Nov 18, 2021, 01:19 UTC

Message-ID

<YZWqK38NRjD7aPOG@danh.dev>

In-Reply-To

<211118.86tuga5o68.gmgdl@evledraar.gmail.com>

On 2021-11-18 00:56:35+0100, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> 
> On Thu, Nov 18 2021, Johannes Schindelin wrote:
> 
> > On Tue, 16 Nov 2021, Jeff King wrote:
> >
> >> I wondered if contrib/buildsystems/CMakeLists would need a similar
> >> fixup, but it doesn't have any generated header dependencies at all (not
> >> for hook-list.h, but not for the existing command-list.h). So I'll
> >> assume it's fine (as did cfe853e66b).
> >
> > The strategy we take in our CMake-based configuration is for files like
> > hook-list.h to be generated at _configure_ time, i.e. before the build
> > definition file is written, i.e. well before the build. That's why there
> > is no explicit dependency, it's not necessary.
> 
> It is necessary, otherwise how will it know to re-generate the
> hook-list.h if its source of truth changes? I.e. if we add a new
> hook. Ditto for a new built-in, config variable etc.
> 
> I understand that the answer is that cmake (or at least our use of it)
> doesn't even try to solve the same problem as the Makefile does, i.e. to
> declare dependencies and to be capable of incremental builds.

> It's more of a one-shot command where you'll need to run its equivalent
> of "make clean" before you recompile.

----- 8< ----

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 6d7bc16d05..a612217dd9 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -52,9 +52,10 @@ cmake_minimum_required(VERSION 3.14)
 #set the source directory to root of git
 set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..)
 
+if(WIN32)
 option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies.  Only applicable to Windows platforms" ON)
-if(NOT WIN32)
-	set(USE_VCPKG OFF CACHE BOOL FORCE)
+else()
+	set(USE_VCPKG OFF)
 endif()
 
 if(NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS)
----- >8 ----

Even after it's applied, the linking step is failing.
(seems to not link with compat/linux/procinfo.o, I didn't dig further)

The traditional method to list source files in CMake (and meson)
is listing them all in the CMakeLists.txt (or meson.build).
With manual listing like that, we can avoid the current complicated
logic to parse Makefile. The bigger benefit from listing manually is:
CMake will generate an implicit dependency to CMakeLists.txt,
hence, whenever a source/header files was added/removed,
cmake will told to re-run configuring steps.

If you're interested on moving on that direction, I can provide
some patches to make the cmake buildsystem a bit less messy,
I'm not a fan of CMake, don't count too much on me, though.

[1]: https://cmake.org/cmake/help/v3.16/command/set.html#set-cache-entry

-- 
Danh