Re: [Buildroot] [PATCH 1/1] Makefile: fix rule order for legal-info

["Yann E. MORIN" <yann.morin.1998@free.fr>]

James, All,

On 2023-02-11 11:43 -0700, James Hilliard spake thusly:
> This command relies on the clean/prepare operations being in a
> specific order.
> 
> Currently the execution ordering of the clean/prepare operations may
> change, for example if --shuffle=reversed is set.
> 
> To ensure the evaluation order is always correct use double colon
> rules to make the evaluation order explicit as per make docs:
> 
> The double-colon rules for a target are executed in the order they
> appear in the makefile.

Thanks for this patch!

What bothers me a bit about using double-colon, is that the make manual
explicitly state that "[d]ouble-colon rules are somewhat obscure and not
often very useful; they provide a mechanism for cases in which the
method used to update a target differs depending on which prerequisite
files caused the update, and such cases are rare."

So, I understand that it does fix the issue, but I'm afraid that it is
going to be so easy to forget about that issue, and what I get from the
make manual is that we should not use them unless as a last resort in
very special corner cases, and the case at hand is not identified by the
manual, which somewhat implies that double-colon rules were written
specifically to handle the case where "the method used to update a
target differs depending on which prerequisite files caused the update",
not specifically to ensure that the order is guarantedd. It even goes as
far as suggesting that this should not really be relied upon:

    The double-colon rules for a target are executed in the order they
    appear in the makefile. However, the cases where double-colon rules
    really make sense are those where the order of executing the recipes
    would not matter.

So, to me, the ordering is only a side-effect of the original goal,
which is to have different commands to generate a target, based on what
prerequisite caused the the update...

I am also afraid that there are so many other places around where we
implicitly rely on the order the prerequisites are listed, and that we
can't easily spot (that I spotted the legal-info issue and asked you
about it, is pure chance, as I was looking at something completely
different).

However, in the legal-info case, the rule is definitely not
parallel-safe, and we do really want it and its dependencies *not* to
execute in parallel in fact.

Indeed, all PKG-legal-info rules will emit a little blurb that is
appended to the manisfest.csv, with commands like:
    echo 'PKG,VERSION,blablsablab' >> manifest.csv

So, if we run that in parallel, this is going to explode [1].

So, I believe in this case, we do not want to use :: rules, but really
ensure that legal-info and all PKG-legal-info rules are never run in
parallel, *and* that they are executed in the order they are listed in
the prerequisites.

We probably want to just add in the top-level Makefile:

    .NOTPARALLEL: legal-info

and in package/pkg-generic.mk, something along the lines of:

    .NOTPARALLEL: $(2)-legal-info #(2)-all-legal-info

Thoughts?

[0] https://www.gnu.org/software/make/manual/make.html#Double_002dColon
[1] with very nice and bright colors, but a bit too loud still

Regards,
Yann E. MORIN.

> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  Makefile | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 4880b426b5..09575a5e3c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -839,7 +839,9 @@ legal-info-prepare: $(LEGAL_INFO_DIR)
>  	@cp $(BR2_CONFIG) $(LEGAL_INFO_DIR)/buildroot.config
>  
>  .PHONY: legal-info
> -legal-info: legal-info-clean legal-info-prepare $(foreach p,$(PACKAGES),$(p)-all-legal-info) \
> +legal-info:: legal-info-clean
> +legal-info:: legal-info-prepare
> +legal-info:: $(foreach p,$(PACKAGES),$(p)-all-legal-info) \
>  		$(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST)
>  	@cat support/legal-info/README.header >>$(LEGAL_REPORT)
>  	@if [ -r $(LEGAL_WARNINGS) ]; then \
> -- 
> 2.34.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot