On Sun, Oct 31, 2021 at 12:32:26AM +0200, Ævar Arnfjörð Bjarmason wrote: > Remove the hardcoded lists of objects in favor of using > $(wildcard). This means that every time a built-in, test tool etc. is > added we won't need to patch the top-level Makefile, except for the > few remaining cases where the asset in question would make it onto one > of our list of exceptions. > > Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/' > subdirectory, 2010-02-22) this has been relatively easy to do (and > even before that we could glob builtin-*.c). This pattern of > exhaustively enumerating files was then carried forward for > e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty > test-tool program, 2018-03-24). > > One reason not to do this is that now a new *.c file at the top-level > will be immediately picked up, so if a new *.c file is being worked on > "make" will error if it doesn't compile, whereas before that file > would need to be explicitly listed in the Makefile. I think than small > trade-off is worth it. A more general way of thinking about this is that we are switching from "ignore source files by default" to "stick source files into LIB_OBJS by default". So it's helpful if you were going to stick that file into LIB_OBJS, but harmful otherwise. Your "new *.c file" example is one case, because it wouldn't have been added _yet_. And I agree it's probably not that big a deal in practice. The other cases are ones similar to what you had to exclude from LIB_OBJS manually here: > +LIB_OBJS += $(filter-out \ > + $(ALL_COMPAT_OBJS) \ > + git.o common-main.o $(PROGRAM_OBJS) \ > + $(FUZZ_OBJS) $(CURL_OBJS),\ > + $(patsubst %.c,%.o,$(wildcard *.c))) So if I wanted to add a new external program source but forgot to put it into PROGRAM_OBJS, the default would now be to pick it up in LIB_OBJS. That's weird and definitely not what you'd want, but presumably you'd figure it out pretty quickly because we wouldn't have built the command you expected to exist. Likewise, there's an interesting tradeoff here for non-program object files. The current Makefile does not need to mention unix-socket.o outside of the NO_UNIX_SOCKETS ifdef block, because that's where we stick it in LIB_OBJS. After your patch, it gets mentioned twice: in that same spot, but also as an exception to the LIB_OBJS rule (via the ALL_COMPAT_OBJS variable above). So we're trading off having to remember to do one thing (add stuff to LIB_OBJS) for another (add stuff to the exception list). Now one of those happens a lot more than the other, which is why you get such a nice diffstat. So it might be worth the tradeoff. I don't have a very strong opinion either way on this. I felt like we'd discussed this direction before, and came up with this thread from the archive: https://lore.kernel.org/git/20110222155637.GC27178@xxxxxxxxxxxxxxxxxxxxx/ There it was coupled with suggestions to actually change the file layout. That could make some of those exceptions go away (say, if all of LIB_OBJS was in "lib/"), but it's a bigger change overall. So I offer it here mostly for historical context / interest. I didn't see anything obviously wrong in the patch, but two comments: > - De-indent an "ifndef" block, we don't usually indent their > contents. Quite a lot of existing conditional blocks are indented, but I think for conditional inclusions of one entry in a larger list (where the rest of the list isn't indented), this makes sense. And that's what you changed here. > +# LIB_OBJS: compat/* objects that live at the top-level > +ALL_COMPAT_OBJS += unix-socket.o > +ALL_COMPAT_OBJS += unix-stream-server.o > +ALL_COMPAT_OBJS += sha1dc_git.o I think "compat" is a misnomer here. For one thing, they're by definition not "compat/*" objects, because they're not in that directory. ;) But more importantly, the interesting thing about them is not that they're compatibility layers, but that they're part of a conditional compilation. I.e., we might or might not want them, which will be determined elsewhere in the Makefile, so they must not be part of the base LIB_OBJS set. Probably CONDITIONAL_OBJS or something might be more descriptive. That _could_ be used to include things like CURL_OBJS, but there's probably value in keeping those in their own list anyway. Likewise, they could go into a conditional-src/ directory (or some less-horrible name) to keep them distinct without needing an explicit list in the Makefile. That's sort of the flip-side of putting all the other LIB_OBJS ones into lib/. -Peff
