Re: [PATCH v2 3/3] Makefile: replace most hardcoded object lists with $(wildcard)

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Ævar

On 21/01/2022 12:01, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Nov 09 2021, Junio C Hamano wrote:
> 
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>>>> We can also have some DEVOPTS knob so we'll prune out files found if
>>>> a
>>>> $(shell)-out to "git status" tells us they're untracked. I wouldn't mind
>>>> that (and could implement it) if it was optional.
>>>> Also note that you've got some of this already, e.g. we'll pick up
>>>> *.h
>>>> files via a glob for "make TAGS", the dependency graph etc.
>>>
>>> I'd be happier using 'git ls-files' with a glob if we need to move
>>> away from listing the files explicitly rather than having to pass some
>>> exclude list when running make. Having seen your comments below about
>>> ls-files/find I had a look at the Makefile and they always seem to be
>>> used together as "git ls-files ... || find ...". Doing that would mean
>>> we wouldn't try to build any untracked files but still find everything
>>> in a tarball.
>>
>> I've been quiet on this topic because honestly I do not find the
>> pros-and-cons favourable for more use of wildcards [*].  Tools like
>> git (especially .gitignore) and Makefile are to help users to be
>> safely sloppy by ensuring that random crufts the users may create in
>> the working tree for their convenience are not picked up by default
>> unless the project to consciously expresses the desire to use them.
>>
>> Allowing to be sloppy while maintaining Makefile feels like a false
>> economy, and having to paper it over by adding exceptions and
>> forcing developers to learn such ad-hoc rules even more so.
>>
>>      Side note: TAGS generation and some other minor things may use
>>      $(wildcard) and can throw tokens in cruft files in the output,
>>      which is not ideal, but the damage is local.  We cannot treat
>>      that the same as building binaries and tarballs.
>>
>> If we could use "git ls-files" consistently, that may make it
>> somewhat safer; you'd at least need to "git add" a new file before
>> it gets into the picture.  But it would be impossible, because we
>> need to be able to bootstrap Git from a tarball extract.
>>
>>>>>> We could make this simpler still for the Makefile by moving
>>>>>> "unix-socket.c" etc. to e.g. a "conditional-src/" directory, likewise
>>>>>> for $(PROGRAM_OBJS) to e.g. "programs/". If we did that we would not
>>>>>> need the "$(filter-out)" for LIB_OBJS. I don't think that's worth it,
>>>>>> e.g. due to "git log -- <path>" on the files now needing a "--follow".
>>
>> And it is not quite clear to me why we want to even more pile
>> workaround like this on top.  This also is to paper over the mistake
>> of being sloppy and using $(wildcard), which makes it unable to
>> distinguish, among the ones that match a pattern, between FOO_OBJS
>> and BAR_OBJS, no?  Moving files around in the working tree to group
>> related things together is a good thing, and it has been a good move
>> to separate built-ins and library-ish parts into different
>> directories.  But the above does not sound like it.
>>
>> Other than "these source files may or may not be compiled
>> depending", what trait do files in conditional-src/ share, other
>> than "dividing them into a separate category makes it simpler to
>> write Makefile using $(wildcard)"?  I do not think of a good one.
>>
>> The only time I found that the large list of files in Makefile was
>> problematic was *NOT* when multiple topics added, renamed or removed
>> the files (it is pretty much bog standard merge conflicts that do
>> not happen very often to begin with).  It is when this kind of
>> "large scale refactoring" for the sake of refactoring happens.
>>
>>> I'm not so worried about those other targets, but being able to
>>> reliably build and test git with some cruft lying around is useful
>>> though. I'm still not entirely sure what the motivation for this
>>> change is (adding new files is not that common) but I think using the
>>> established "git ls-files || find" pattern would be a good way of
>>> globbing without picking up rubbish if there is a compelling reason to
>>> drop the lists.
>>
>> Yes.
> 
> Reviewing the reftable coverity topic I was reminded of this
> patch. I.e. in it we have this fix:
> https://lore.kernel.org/git/xmqqtugl102l.fsf@gitster.g/
> 
> Which shows another advantage of using this sort of $(wildcard) pattern,
> i.e. if we had this:
> 	
> 	diff --git a/Makefile b/Makefile
> 	index 5580859afdb..48ea18afa53 100644
> 	--- a/Makefile
> 	+++ b/Makefile
> 	@@ -2443,33 +2443,9 @@ XDIFF_OBJS += xdiff/xutils.o
> 	 .PHONY: xdiff-objs
> 	 xdiff-objs: $(XDIFF_OBJS)
> 	
> 	-REFTABLE_OBJS += reftable/basics.o
> 	-REFTABLE_OBJS += reftable/error.o
> 	-REFTABLE_OBJS += reftable/block.o
> 	-REFTABLE_OBJS += reftable/blocksource.o
> 	-REFTABLE_OBJS += reftable/iter.o
> 	-REFTABLE_OBJS += reftable/publicbasics.o
> 	-REFTABLE_OBJS += reftable/merged.o
> 	-REFTABLE_OBJS += reftable/pq.o
> 	-REFTABLE_OBJS += reftable/reader.o
> 	-REFTABLE_OBJS += reftable/record.o
> 	-REFTABLE_OBJS += reftable/refname.o
> 	-REFTABLE_OBJS += reftable/generic.o
> 	-REFTABLE_OBJS += reftable/stack.o
> 	-REFTABLE_OBJS += reftable/tree.o
> 	-REFTABLE_OBJS += reftable/writer.o
> 	-
> 	-REFTABLE_TEST_OBJS += reftable/basics_test.o
> 	-REFTABLE_TEST_OBJS += reftable/block_test.o
> 	-REFTABLE_TEST_OBJS += reftable/dump.o
> 	-REFTABLE_TEST_OBJS += reftable/merged_test.o
> 	-REFTABLE_TEST_OBJS += reftable/pq_test.o
> 	-REFTABLE_TEST_OBJS += reftable/record_test.o
> 	-REFTABLE_TEST_OBJS += reftable/readwrite_test.o
> 	-REFTABLE_TEST_OBJS += reftable/refname_test.o
> 	-REFTABLE_TEST_OBJS += reftable/stack_test.o
> 	-REFTABLE_TEST_OBJS += reftable/test_framework.o
> 	-REFTABLE_TEST_OBJS += reftable/tree_test.o
> 	+REFTABLE_SOURCES = $(wildcard reftable/*.c)
> 	+REFTABLE_OBJS += $(filter-out test,$(REFTABLE_SOURCES:%.c=%.o))
> 	+REFTABLE_TEST_OBJS += $(filter test,$(REFTABLE_SOURCES:%.c=%.o))
> 	
> 	 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
> 
> We'd have a shorter Makefile, not need to manually maintain the list,
> and we'd have been getting linker errors all along on the dead code
> (just showing one of many here):
> 
> 	$ make
> 	[...]
> 	/usr/bin/ld: reftable/libreftable.a(generic.o): in function `reftable_table_seek_ref':
> 	/home/avar/g/git/reftable/generic.c:17: multiple definition of `reftable_table_seek_ref'; reftable/libreftable.a(reftable.o):/home/avar/g/git/reftable/reftable.c:17: first defined here
> 	[...]
> 	clang: error: linker command failed with exit code 1 (use -v to see invocation)
> 	make: *** [Makefile:2925: t/helper/test-tool] Error 1
> 	make: Target 'all' not remade because of errors.

Random cruft breaking the build was the reason I objected to this 
change, just because the cruft was being tracked by git in this case 
does not change that.

Best Wishes

Phillip