| Message ID | 7d82342089a80b19e54ac8997d5765a33951499f.1637112066.git.congdanhqx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: fix parallel build race | expand |
On Wed, Nov 17, 2021 at 08:25:55AM +0700, Đoàn Trần Công Danh wrote: > * builtin/bugreport.c includes hook-list.h, hence generated files from > it must depend on hook-list.h Good catch. This is trivially reproducible with: make clean make builtin/bugreport.o The problem comes from cfe853e66b (hook-list.h: add a generated list of hooks, like config-list.h, 2021-09-26), as you might expect. > diff --git a/Makefile b/Makefile > index 241dc322c0..413503b488 100644 > --- a/Makefile > +++ b/Makefile > @@ -2222,6 +2222,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS) > > help.sp help.s help.o: command-list.h > hook.sp hook.s hook.o: hook-list.h > +builtin/bugreport.sp builtin/bugreport.s builtin/bugreport.o: hook-list.h This fix looks correct. I grepped for other similar cases, but this is the only file that needs it. Curiously, the existing hook.c does not seem to include hook-list.h, even though you can see a dependency in the context above. Nor does help.c, which gained a similar dependency in cfe853e66b. Those seem superfluous, but maybe I'm missing something. 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). -Peff
On Tue, Nov 16, 2021 at 10:18:14PM -0500, Jeff King wrote: > On Wed, Nov 17, 2021 at 08:25:55AM +0700, Đoàn Trần Công Danh wrote: > > > * builtin/bugreport.c includes hook-list.h, hence generated files from > > it must depend on hook-list.h > > Good catch. This is trivially reproducible with: > > make clean > make builtin/bugreport.o > > The problem comes from cfe853e66b (hook-list.h: add a generated list of > hooks, like config-list.h, 2021-09-26), as you might expect. > > > diff --git a/Makefile b/Makefile > > index 241dc322c0..413503b488 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -2222,6 +2222,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS) > > > > help.sp help.s help.o: command-list.h > > hook.sp hook.s hook.o: hook-list.h > > +builtin/bugreport.sp builtin/bugreport.s builtin/bugreport.o: hook-list.h > > This fix looks correct. I grepped for other similar cases, but this is > the only file that needs it. > > Curiously, the existing hook.c does not seem to include hook-list.h, > even though you can see a dependency in the context above. Nor does > help.c, which gained a similar dependency in cfe853e66b. Those seem > superfluous, but maybe I'm missing something. Neither does builtin/help.c. This was discussed in the subthread starting at https://lore.kernel.org/all/20211115220455.xse7mhbwabrheej4@glandium.org/ and is covered by https://lore.kernel.org/all/patch-v3-19.23-234b4eb613c-20211116T114334Z-avarab@gmail.com/ (to which I responded that the line for hook.o can be removed too) Mike
On Wed, Nov 17 2021, Mike Hommey wrote: > On Tue, Nov 16, 2021 at 10:18:14PM -0500, Jeff King wrote: >> On Wed, Nov 17, 2021 at 08:25:55AM +0700, Đoàn Trần Công Danh wrote: >> >> > * builtin/bugreport.c includes hook-list.h, hence generated files from >> > it must depend on hook-list.h >> >> Good catch. This is trivially reproducible with: >> >> make clean >> make builtin/bugreport.o >> >> The problem comes from cfe853e66b (hook-list.h: add a generated list of >> hooks, like config-list.h, 2021-09-26), as you might expect. >> >> > diff --git a/Makefile b/Makefile >> > index 241dc322c0..413503b488 100644 >> > --- a/Makefile >> > +++ b/Makefile >> > @@ -2222,6 +2222,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS) >> > >> > help.sp help.s help.o: command-list.h >> > hook.sp hook.s hook.o: hook-list.h >> > +builtin/bugreport.sp builtin/bugreport.s builtin/bugreport.o: hook-list.h >> >> This fix looks correct. I grepped for other similar cases, but this is >> the only file that needs it. >> >> Curiously, the existing hook.c does not seem to include hook-list.h, >> even though you can see a dependency in the context above. Nor does >> help.c, which gained a similar dependency in cfe853e66b. Those seem >> superfluous, but maybe I'm missing something. > > Neither does builtin/help.c. This was discussed in the subthread > starting at https://lore.kernel.org/all/20211115220455.xse7mhbwabrheej4@glandium.org/ > and is covered by https://lore.kernel.org/all/patch-v3-19.23-234b4eb613c-20211116T114334Z-avarab@gmail.com/ > (to which I responded that the line for hook.o can be removed too) I've got an updated patch in my just-re-rolled Makefile dependency fixes series for this isuse, which also addresses the needless "hook.{sp,s,o} -> hook-list.h" dependency issue: https://lore.kernel.org/git/patch-v4-19.23-2710f8af6cd-20211117T101807Z-avarab@gmail.com/ Thanks again for looking all of this over & helping to make the Makefile better.
Hi Peff, 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. Ciao, Dscho
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. Correct?
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
Hi,
On Thu, 18 Nov 2021, Đoàn Trần Công Danh wrote:
> [Git's CMake-based build] couldn't be configured on Linux system,
That was an explicit decision in
https://lore.kernel.org/git/xmqq1rmcm6md.fsf@gitster.c.googlers.com/:
Let's not worry about cross-platform and instead stick to Windows
and nothing else for now to expedite the process. As long as it
is advertised as such, nobody would complain that it does not work
on Linux or macOS.
Ciao,
Dscho
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/
On Thu, Nov 18 2021, Johannes Schindelin wrote: > On Thu, 18 Nov 2021, Đoàn Trần Công Danh wrote: > >> [Git's CMake-based build] couldn't be configured on Linux system, > > That was an explicit decision in > https://lore.kernel.org/git/xmqq1rmcm6md.fsf@gitster.c.googlers.com/: > > Let's not worry about cross-platform and instead stick to Windows > and nothing else for now to expedite the process. As long as it > is advertised as such, nobody would complain that it does not work > on Linux or macOS. That was said at a time when the CI didn't have a hard dependency on this cmake integration, which as noted in the recent discussion downthread of [1] made it so that for changes in this area you need to maintain both the Makefile and contrib/buildsystems/CMakeLists.txt in lockstep, least that CI is broken. So we've created a scenario where in order to make certain changes to git.git, you need to either go through a very painfully slow edit/push/test cycle via the CI, with each test taking anywhere between ~5m-60m, depending on what step it might fail at. Or, install your own copy of that proprietary OS locally. In other areas we often don't have any hard line separating such platform-specific code from the rest of the codebase. But for anything else I can think of it's in its own files or ifdefs, and usually doesn't require maintaining dual-implementations in lockstep, except if the interface itself is changing. Or that code is at least in a common language like C, Sh, Perl etc., and doesn't require you to learn a new tool/language just for maintaining the propriterary-specific portability code. So I think it would be most welcome to get patches to contrib/buildsystems/CMakeLists.txt to make it portable, cmake itself is, and AFAICT the only inherently Windows-specific part of it is some small part dealing with generating a file for VS to consume, which presumably can be in some if/else construct within that file (I haven't checked out Phillip Wood's portability patches). 1. https://lore.kernel.org/git/patch-1.1-bbacbed5c95-20211030T223011Z-avarab@gmail.com/
Hi Ævar, On Fri, 19 Nov 2021, Ævar Arnfjörð Bjarmason wrote: > 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. The idea to extend the CMake to more than just Windows is contrary to what Junio said in https://lore.kernel.org/git/xmqq1rmcm6md.fsf@gitster.c.googlers.com/: Let's not worry about cross-platform and instead stick to Windows and nothing else for now to expedite the process. As long as it is advertised as such, nobody would complain that it does not work on Linux or macOS. If that is not enough to tone down opposing opinions (the opinion of the Git maintainer is more important, after all, it's his maintenance burden so he gets to decide), you can also look at this statement from https://lore.kernel.org/git/xmqq8sikblv2.fsf@gitster.c.googlers.com/: I already said that I feel that engineering burden to divert resources for CMake support would be unacceptably high. The only reason we have CMake in addition to the Makefile (and the autoconf-based) setup is that CMake makes it possible to build Git on Windows in the development environment with which the majority of the developers on Windows are familiar: Visual Studio. If it weren't for those developers, for who it would be a ridiculous suggestion to "just go download GNU make", we would not have the CMake based build at all. And I am still agreeing with what Junio further said in the second mail I linked above: [...] it is unclear why it would be beneficial to slow our existing developers down by forcing them to become familiar with CMake. So now we are discussing to extend the CMake build to allow Linux and macOS developers to use it, to, for little to no benefit. We are very much in the situation where we are slowed down by discussing something as non-essential as extending our CMake support beyond Windows, while patches that are provably much more beneficial to a lot more people are left under-reviewed. Even worse: reviewers who _could_ provide high-quality reviews for those patches (which takes a lot of time and diligence), but are as much pressed for time as I am and therefore have to choose wisely how to spend their time, are _actively_ distracted from spending their time more wisely. Can't we please focus on more relevant things again? Pretty please? Ciao, Johannes
On Fri, Nov 19 2021, Johannes Schindelin wrote: > On Fri, 19 Nov 2021, Ævar Arnfjörð Bjarmason wrote: > >> 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. > > The idea to extend the CMake to more than just Windows is contrary to what > Junio said in > https://lore.kernel.org/git/xmqq1rmcm6md.fsf@gitster.c.googlers.com/: > > Let's not worry about cross-platform and instead stick to Windows > and nothing else for now to expedite the process. As long as it > is advertised as such, nobody would complain that it does not work > on Linux or macOS. That's quoted in the <211119.86ee7c4n8r.gmgdl@evledraar.gmail.com> you're replying to. No need to repeat it. > If that is not enough to tone down opposing opinions (the opinion of the > Git maintainer is more important, after all, it's his maintenance burden > so he gets to decide), you can also look at this statement from > https://lore.kernel.org/git/xmqq8sikblv2.fsf@gitster.c.googlers.com/: > > I already said that I feel that engineering burden to divert > resources for CMake support would be unacceptably high. There's some back & forth in that thread, I do think a fair summary of it is that the proponents of CMakeLists.txt, including yourself, assured reviewers that having the cmake component wouldn't place a maintenance burden on anyone not using it. Including yourself at: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2004251354390.18039@tvgsbejvaqbjf.bet/: When it comes to new Makefile knobs, I do agree that it would place an unacceptable burden on contributors if we expected them to add the same knob to CMakeLists.txt. But we already don't do that for our autoconf support, so why would we expect it for CMake? When it comes to adding new, and/or removing, files, I fail to see the problem. It is dead easy to keep the Makefile and CMakeLists.txt in sync when it comes to lists of files. The "that it" here refers to "slow our existing developers down by forcing them to become familiar with CMake", which you quote below. > The only reason we have CMake in addition to the Makefile (and the > autoconf-based) setup is that CMake makes it possible to build Git on > Windows in the development environment with which the majority of the > developers on Windows are familiar: Visual Studio. > > If it weren't for those developers, for who it would be a ridiculous > suggestion to "just go download GNU make", we would not have the CMake > based build at all. I'm happy it works for those developers, and don't mind at all that they're choosing to run a propriterary stack while developing a free software project, I'd just prefer not to be forced to do that because our CI has a hard dependency on it. We can argue about the trade-offs here, but I think it's clearly hyperbole to say that would be a ridiculous suggestion when it was the status quo until 2020. I'm specifically pointing out that the issue with the hard dependency of CI on this "contrib" component. I'd think of all people you'd be in vehement agreement with me on that point, after all our back & forth on the contrib/scalar topic, and that things "contrib" must be decoupled and "optional" in a way that this cmake integration clearly isn't. > And I am still agreeing with what Junio further said in the second mail I > linked above: > > [...] it is unclear why it would be beneficial to slow our > existing developers down by forcing them to become familiar with > CMake. > > So now we are discussing to extend the CMake build to allow Linux and > macOS developers to use it, to, for little to no benefit. We are very much > in the situation where we are slowed down by discussing something as > non-essential as extending our CMake support beyond Windows, while patches > that are provably much more beneficial to a lot more people are left > under-reviewed. > > Even worse: reviewers who _could_ provide high-quality reviews for those > patches (which takes a lot of time and diligence), but are as much pressed > for time as I am and therefore have to choose wisely how to spend their > time, are _actively_ distracted from spending their time more wisely. > > Can't we please focus on more relevant things again? Pretty please? We have out-of-tree patches to make this thing work outside of Windows authored by Phillip, and it sonuds like Đoàn would find it useful too. As for myself I really don't care to interact with this cmake component at all, but if we're not going to drop the CI hard dependency on it I'd find being able to test it on a free OS locally to be vastly preferrable. In any case, I'm not submitting patches in that direction. But I don't see a reason to discourage other people from collaborating on topics they may find useful, as you're doing here.
diff --git a/Makefile b/Makefile index 241dc322c0..413503b488 100644 --- a/Makefile +++ b/Makefile @@ -2222,6 +2222,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS) help.sp help.s help.o: command-list.h hook.sp hook.s hook.o: hook-list.h +builtin/bugreport.sp builtin/bugreport.s builtin/bugreport.o: hook-list.h builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
* builtin/bugreport.c includes hook-list.h, hence generated files from it must depend on hook-list.h Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> --- Makefile | 1 + 1 file changed, 1 insertion(+)