Re: [PATCH] Makefile: fix use of -j without an argument

Re: [PATCH] Makefile: fix use of -j without an argument

[Matheus Tavares Bernardino]

On Fri, 12 Apr 2024 10:02:54 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Thu, Apr 11, 2024 at 5:46 PM Matheus Tavares Bernardino
> <quic_mathbern@quicinc.com> wrote:
> > +        $(if $(filter -j, $(MAKEFLAGS)) \
> > +            ,, \
> > +            $(or \
> > +                 $(filter -l% -j%, $(MAKEFLAGS)), \
> > +                 $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \
> > +        ) -d keepdepfile
> 
> This is more easily written as $(filter-out -j, $(or ...)).
> 
> I've sent a v2.

Thanks! 

[PATCH] Makefile: fix use of -j without an argument

[Matheus Tavares Bernardino]

Our Makefile massages the given make arguments to invoke ninja
accordingly. One key difference is that ninja will parallelize by
default, whereas make only does so with -j<n> or -j. The make man page
says that "if the -j option is given without an argument, make will not
limit the number of jobs that can run simultaneously". We use to support
that by replacing -j with "" (empty string) when calling ninja, so that
it would do its auto-parallelization based on the number of CPU cores.

This was accidentally broken at d1ce2cc95b (Makefile: preserve
--jobserver-auth argument when calling ninja, 2024-04-02),
causing `make -j` to fail:

$ make -j V=1
  /usr/bin/ninja -v   -j -d keepdepfile all | cat
  make  -C contrib/plugins/ V="1" TARGET_DIR="contrib/plugins/" all
  ninja: fatal: invalid -j parameter
  make: *** [Makefile:161: run-ninja] Error

Let's fix that and indent the touched code for better readability.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 Makefile | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 183756018f..d299c14dab 100644
--- a/Makefile
+++ b/Makefile
@@ -142,8 +142,12 @@ MAKE.k = $(findstring k,$(firstword $(filter-out --%,$(MAKEFLAGS))))
 MAKE.q = $(findstring q,$(firstword $(filter-out --%,$(MAKEFLAGS))))
 MAKE.nq = $(if $(word 2, $(MAKE.n) $(MAKE.q)),nq)
 NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if $(MAKE.k), -k0) \
-        $(or $(filter -l% -j%, $(MAKEFLAGS)), $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \
-        -d keepdepfile
+        $(if $(filter -j, $(MAKEFLAGS)) \
+	     ,, \
+	     $(or \
+	          $(filter -l% -j%, $(MAKEFLAGS)), \
+	          $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \
+        ) -d keepdepfile
 ninja-cmd-goals = $(or $(MAKECMDGOALS), all)
 ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g))
 
-- 
2.37.2

Re: [PATCH] Makefile: fix use of -j without an argument

[Philippe Mathieu-Daudé]

Hi Matheus,

On 11/4/24 17:09, Matheus Tavares Bernardino wrote:
> Our Makefile massages the given make arguments to invoke ninja
> accordingly. One key difference is that ninja will parallelize by
> default, whereas make only does so with -j<n> or -j. The make man page
> says that "if the -j option is given without an argument, make will not
> limit the number of jobs that can run simultaneously". We use to support
> that by replacing -j with "" (empty string) when calling ninja, so that
> it would do its auto-parallelization based on the number of CPU cores.
> 
> This was accidentally broken at d1ce2cc95b (Makefile: preserve
> --jobserver-auth argument when calling ninja, 2024-04-02),
> causing `make -j` to fail:
> 
> $ make -j V=1
>    /usr/bin/ninja -v   -j -d keepdepfile all | cat
>    make  -C contrib/plugins/ V="1" TARGET_DIR="contrib/plugins/" all
>    ninja: fatal: invalid -j parameter
>    make: *** [Makefile:161: run-ninja] Error
> 
> Let's fix that and indent the touched code for better readability.
> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>   Makefile | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 183756018f..d299c14dab 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -142,8 +142,12 @@ MAKE.k = $(findstring k,$(firstword $(filter-out --%,$(MAKEFLAGS))))
>   MAKE.q = $(findstring q,$(firstword $(filter-out --%,$(MAKEFLAGS))))
>   MAKE.nq = $(if $(word 2, $(MAKE.n) $(MAKE.q)),nq)
>   NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if $(MAKE.k), -k0) \
> -        $(or $(filter -l% -j%, $(MAKEFLAGS)), $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \
> -        -d keepdepfile
> +        $(if $(filter -j, $(MAKEFLAGS)) \
> +	     ,, \
> +	     $(or \
> +	          $(filter -l% -j%, $(MAKEFLAGS)), \
> +	          $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \
> +        ) -d keepdepfile
>   ninja-cmd-goals = $(or $(MAKECMDGOALS), all)
>   ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g))
>   

Apparently Martin sent the same patch (although not as nicely
indented) and Paolo queued it:
https://lore.kernel.org/qemu-devel/20240402081738.1051560-1-martin@geanix.com/

Re: [PATCH] Makefile: fix use of -j without an argument

[Matheus Tavares Bernardino]

Hi, Philippe

On Thu, 11 Apr 2024 17:29:58 +0200 =?UTF-8?Q?Philippe_Mathieu-Daud=C3=A9?= <philmd@linaro.org> wrote:
>
> Hi Matheus,
> 
> On 11/4/24 17:09, Matheus Tavares Bernardino wrote:
> > Our Makefile massages the given make arguments to invoke ninja
> > accordingly. One key difference is that ninja will parallelize by
> > default, whereas make only does so with -j<n> or -j. The make man page
> > says that "if the -j option is given without an argument, make will not
> > limit the number of jobs that can run simultaneously". We use to support
> > that by replacing -j with "" (empty string) when calling ninja, so that
> > it would do its auto-parallelization based on the number of CPU cores.
> > 
> > This was accidentally broken at d1ce2cc95b (Makefile: preserve
> > --jobserver-auth argument when calling ninja, 2024-04-02),
> > causing `make -j` to fail:
> > 
> > $ make -j V=1
> >    /usr/bin/ninja -v   -j -d keepdepfile all | cat
> >    make  -C contrib/plugins/ V="1" TARGET_DIR="contrib/plugins/" all
> >    ninja: fatal: invalid -j parameter
> >    make: *** [Makefile:161: run-ninja] Error
> > 
> > Let's fix that and indent the touched code for better readability.
> > 
> > Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> > ---
> >   Makefile | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 183756018f..d299c14dab 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -142,8 +142,12 @@ MAKE.k = $(findstring k,$(firstword $(filter-out --%,$(MAKEFLAGS))))
> >   MAKE.q = $(findstring q,$(firstword $(filter-out --%,$(MAKEFLAGS))))
> >   MAKE.nq = $(if $(word 2, $(MAKE.n) $(MAKE.q)),nq)
> >   NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if $(MAKE.k), -k0) \
> > -        $(or $(filter -l% -j%, $(MAKEFLAGS)), $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \
> > -        -d keepdepfile
> > +        $(if $(filter -j, $(MAKEFLAGS)) \
> > +	     ,, \
> > +	     $(or \
> > +	          $(filter -l% -j%, $(MAKEFLAGS)), \
> > +	          $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \
> > +        ) -d keepdepfile
> >   ninja-cmd-goals = $(or $(MAKECMDGOALS), all)
> >   ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g))
> >   
> 
> Apparently Martin sent the same patch (although not as nicely
> indented) and Paolo queued it:
> https://lore.kernel.org/qemu-devel/20240402081738.1051560-1-martin@geanix.com/

Actually, this patch is a follow-up to that one, fixing a feature that was
accidentally broken.

Re: [PATCH] Makefile: fix use of -j without an argument

[Philippe Mathieu-Daudé]

On 11/4/24 17:38, Matheus Tavares Bernardino wrote:
> Hi, Philippe
> 
> On Thu, 11 Apr 2024 17:29:58 +0200 =?UTF-8?Q?Philippe_Mathieu-Daud=C3=A9?= <philmd@linaro.org> wrote:
>>
>> Hi Matheus,
>>
>> On 11/4/24 17:09, Matheus Tavares Bernardino wrote:
>>> Our Makefile massages the given make arguments to invoke ninja
>>> accordingly. One key difference is that ninja will parallelize by
>>> default, whereas make only does so with -j<n> or -j. The make man page
>>> says that "if the -j option is given without an argument, make will not
>>> limit the number of jobs that can run simultaneously". We use to support
>>> that by replacing -j with "" (empty string) when calling ninja, so that
>>> it would do its auto-parallelization based on the number of CPU cores.
>>>
>>> This was accidentally broken at d1ce2cc95b (Makefile: preserve
>>> --jobserver-auth argument when calling ninja, 2024-04-02),
>>> causing `make -j` to fail:
>>>
>>> $ make -j V=1
>>>     /usr/bin/ninja -v   -j -d keepdepfile all | cat
>>>     make  -C contrib/plugins/ V="1" TARGET_DIR="contrib/plugins/" all
>>>     ninja: fatal: invalid -j parameter
>>>     make: *** [Makefile:161: run-ninja] Error
>>>
>>> Let's fix that and indent the touched code for better readability.
>>>
>>> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
>>> ---
>>>    Makefile | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 183756018f..d299c14dab 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -142,8 +142,12 @@ MAKE.k = $(findstring k,$(firstword $(filter-out --%,$(MAKEFLAGS))))
>>>    MAKE.q = $(findstring q,$(firstword $(filter-out --%,$(MAKEFLAGS))))
>>>    MAKE.nq = $(if $(word 2, $(MAKE.n) $(MAKE.q)),nq)
>>>    NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if $(MAKE.k), -k0) \
>>> -        $(or $(filter -l% -j%, $(MAKEFLAGS)), $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \
>>> -        -d keepdepfile
>>> +        $(if $(filter -j, $(MAKEFLAGS)) \
>>> +	     ,, \
>>> +	     $(or \
>>> +	          $(filter -l% -j%, $(MAKEFLAGS)), \
>>> +	          $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \
>>> +        ) -d keepdepfile
>>>    ninja-cmd-goals = $(or $(MAKECMDGOALS), all)
>>>    ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g))
>>>    
>>
>> Apparently Martin sent the same patch (although not as nicely
>> indented) and Paolo queued it:
>> https://lore.kernel.org/qemu-devel/20240402081738.1051560-1-martin@geanix.com/
> 
> Actually, this patch is a follow-up to that one, fixing a feature that was
> accidentally broken.

Oops sorry I missed that, I was not expecting this patch to be merged
in 9.0 :/

Re: [PATCH] Makefile: fix use of -j without an argument

[Paolo Bonzini]

On Thu, Apr 11, 2024 at 5:46 PM Matheus Tavares Bernardino
<quic_mathbern@quicinc.com> wrote:
> +        $(if $(filter -j, $(MAKEFLAGS)) \
> +            ,, \
> +            $(or \
> +                 $(filter -l% -j%, $(MAKEFLAGS)), \
> +                 $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \
> +        ) -d keepdepfile

This is more easily written as $(filter-out -j, $(or ...)).

I've sent a v2.

Paolo

>  ninja-cmd-goals = $(or $(MAKECMDGOALS), all)
>  ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g))
>
> --
> 2.37.2
>