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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 10, 2021, 13:21 UTC
Message-ID: <211110.86h7cki0uo.gmgdl@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2111101332130.21127@tvgsbejvaqbjf.bet>

On Wed, Nov 10 2021, Johannes Schindelin wrote:

Show 9 quoted lines

> Hi Junio,
>
> On Tue, 9 Nov 2021, Junio C Hamano wrote:
>
>> 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.
>
> If you ever needed another opinion to back you up on this: I fully agree.

I could go either way on that, but in terms of Makefile maintenance it does suck a lot less to pick one or the other.

A (I realize, unstated) eventual goal I had was to move these wildcard declarations to some common list you can include from various Makefiles, currently we've got dependency bugs in e.g. Makefile & Documentation/Makefile interaction.

If we're not OK with $(wildcard) as a pattern that would mean changing
all of these to hardcoded (in some cases quite big) lists somewhere:
    
    $ git -P grep -E '^[^~]+\$\(wildcard.+\*' ':!git-gui' ':!gitk-git' ':!contrib'
    Documentation/Makefile:         $(wildcard git-*.txt))
    Documentation/Makefile:HOWTO_TXT += $(wildcard howto/*.txt)
    Documentation/Makefile:DOC_DEP_TXT += $(wildcard *.txt)
    Documentation/Makefile:DOC_DEP_TXT += $(wildcard config/*.txt)
    Documentation/Makefile:API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt)))
    Documentation/Makefile:mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*)
    Documentation/Makefile:%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
    Makefile:command-list.h: $(wildcard Documentation/git*.txt)
    Makefile:POFILES := $(wildcard po/*.po)
    Makefile:LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm perl/Git/*/*/*.pm)
    Makefile:LIB_CPAN := $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
    Makefile:coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))
    Makefile:coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))
    t/Makefile:T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
    t/Makefile:TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
    t/Makefile:THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))
    t/Makefile:TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
    t/Makefile:CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
    t/interop/Makefile:T = $(sort $(wildcard i[0-9][0-9][0-9][0-9]-*.sh))

What do you & Junio think about that?

I don't really mind either way, as long as I stop running into occasional bugs where I need to run "git clean -dxf" because the Makeefile was too stupid to properly manage its dependencies.

Show 9 quoted lines

>> 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.
>
> Indeed, the ability to build from a `.tar` extract is important. That's
> why we were careful to use `ls-files` in `LIB_H` and in
> `FIND_SOURCE_FILE`, falling back on using `find` if the `ls-files` call
> failed.

Why would you need any of that to *build* from a .tar extract? I think we should remove that LIB_H thing entirely.

Its only purpose is to support someone who:

 1. Wants to do an *incremental* build, not a "build from tar". I.e. you
    build already, changed a header, and now you want to not over-build
    again.

    Your compiler is perfectly capable of locating headers in an -I dir
    for you.

 2. Doesn't have gcc or clang installed. Note "installed", not to
    build.

    Well, currently we require you to build with those to use .depends &
    COMPUTE_HEADER_DEPENDENCIES, but that's an easily fixable
    implementation detail.

    We can easily make the .depend files with gcc/clang and build with
    another compiler. I had a 5-10 line local change at some point to do
    that.

 3. Doesn't find it acceptable to have a fallback of just a glob like
    "**.h" for that "depends" target.

    I.e. we'd over-rebuild if you dropped in a new *.h we're not
    actually using into your extracted tarball, but really, who cares?

 4. Wants to run "make hdr-check" or "make pot", both of which I think
    are OK to say "you need to run this on a box that has .depends (or
    in the case of *.pot, we can use a greedier glob).

> And to be honest, even `LIB_H` and `FIND_SOURCE_FILE` would quite
> potentially better be hard-coded (with a CI check to ensure that they're
> up to date).

That would be a bug, just because I don't build on Windows doesn't mean that I wouldn't like "make TAGS coccicheck" to find compat/win32/ at all.

It doesn't do that now for a different reason, but that's a bug that should be fixed.