Re: [PATCH] Makefile: dedup git-ls-files output to prevent duplicate targets

From: Jiang Xin <worldhello.net@gmail.com>
Date: May 26, 2022, 06:06 UTC
Message-ID: <CANYiYbEcNJ7+7XW-8-v+p8q=aiOP9RJYvST8ethVjxVdNugR5Q@mail.gmail.com>
In-Reply-To: <xmqqo7zl2b66.fsf@gitster.g>

On Thu, May 26, 2022 at 12:03 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 66 quoted lines

>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > If there are unresolved conflicts left in the working tree, "make" may
> > report warnings as follows:
> >
> >     Makefile:xxxx: target '.build/pot/po/FOO.c.po' given more than once
> >                    in the same rule
> >
> > The duplicate targets are introduced by the following pattern rule we
> > added in the preceding commit for incremental build of "po/git.pot",
> >
> >     $(LOCALIZED_C_GEN_PO): .build/pot/po/%.po: %
> >
> > and the duplicate entries in $(LOCALIZED_C_GEN_PO) come from the
> > "git ls-files" command in SOURCES_CMD.
> >
> > We can pass the option "--deduplicate" to git-ls-files to suppress
> > duplicate entries for unresolved conflicts.
>
> Thanks for a quick response.
>
> We certainly can say "your SOURCES_CMD MUST NOT produce duplicates"
> and passing the --deduplicate option is one valid way to fix this
> specific case.
>
> But I wonder if a more future-proof solution is to dedup the output
> of the SOURCES_CMD ourselves on the Makefile side.  That way, even
> if we update SOURCES_CMD in a way that could contain duplicates, we
> won't have to worry about duplicates.
>
> ---
>
> It feels way overkill to "sort" the list just to dedup its elements,
> but that is what GNU Make documentation info page recommends us to
> do, and we already do use it for deduplication in our Makefile
> twice.
>
> '$(sort LIST)'
>      Sorts the words of LIST in lexical order, removing duplicate words.
>      The output is a list of words separated by single spaces.  Thus,
>
>           $(sort foo bar lose)
>
>      returns the value 'bar foo lose'.
>
>      Incidentally, since 'sort' removes duplicate words, you can use it
>      for this purpose even if you don't care about the sort order.
>
>
> diff --git i/Makefile w/Makefile
> index 2b61f66259..1d3d3deba1 100644
> --- i/Makefile
> +++ w/Makefile
> @@ -860,7 +860,7 @@ SOURCES_CMD = ( \
>                 -o \( -name '*.sh' -type f -print \) \
>                 | sed -e 's|^\./||' \
>         )
> -FOUND_SOURCE_FILES := $(shell $(SOURCES_CMD))
> +FOUND_SOURCE_FILES := $(sort $(shell $(SOURCES_CMD)))
>
>  FOUND_C_SOURCES = $(filter %.c,$(FOUND_SOURCE_FILES))
>  FOUND_H_SOURCES = $(filter %.h,$(FOUND_SOURCE_FILES))
>

If I disabled the git-ls-files command like below,

    @@ -846,7 +846,7 @@ generated-hdrs: $(GENERATED_H)
     ## Exhaustive lists of our source files, either dynamically generated,
     ## or hardcoded.
     SOURCES_CMD = ( \
    -       git ls-files --deduplicate \
    +       git bad-ls-files --deduplicate \
                    '*.[hcS]' \
                    '*.sh' \
                    ':!*[tp][0-9][0-9][0-9][0-9]*' \

, and run "make", will display the following warnings:

    Makefile:2751: target `.build/pot/po/command-list.h.po' given more
than once in the same rule.
    Makefile:2751: target `.build/pot/po/config-list.h.po' given more
than once in the same rule.
    Makefile:2751: target `.build/pot/po/hook-list.h.po' given more
than once in the same rule.

This is because the three generated header files (defined in $(GENERATED_H)) are also included in the result of "SOURCES_CMD". We can fix this by sorting LOCALIZED_C:

    -LOCALIZED_C = $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) $(SCALAR_SOURCES) \
    -             $(GENERATED_H)
    +LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES)
$(SCALAR_SOURCES) \
    +               $(GENERATED_H))

Will send v2 patch.