Re: [PATCH] Makefile: fix parallel build race

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Nov 18 2021, Đoàn Trần Công Danh wrote:

> 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.
>
> If used correctly, with correct dependencies link, cmake is fully
> capable to regenerate hook-list.h upon its source mtime changed.
>
>> It's more of a one-shot command where you'll need to run its equivalent
>> of "make clean" before you recompile.
>
> However, the current CMakeLists.txt has a bigger problem: it won't
> re-run itself when a source file has been added or removed.
> It couldn't be configured on Linux system, except with this diff
> applied (because CMake documentation mandated <docstring> in
> (set CACHE FORCE) [1]):
>
> ----- 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

I think getting it working on non-Windows if we're going to keep it
(which looks to be the case) would be very useful.

I think you should look at the WIP patches from & coordinate with
Phillip Wood, who has WIP patches in that direction. See:
https://lore.kernel.org/git/24482f96-7d87-1570-a171-95ec182f6091@gmail.com/