[Qemu-devel] [PATCH] Makefile: Properly order build targets 'all' and 'check'

[Qemu-devel] [PATCH] Makefile: Properly order build targets 'all' and 'check'

[Michal Privoznik]

I'm used to run 'make -j5 all check'. However, this is not possible in
qemu because of the missing dependency in the Makefile. If I do that,
tests are usually started with build and since not everything is built
yet, they often fail too. Moreover, we should run test suite only
after every binary we want to test has been built.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index e7c5c3a..67eeb87 100644
--- a/Makefile
+++ b/Makefile
@@ -151,6 +151,7 @@ dummy := $(call unnest-vars,, \
 
 ifneq ($(wildcard config-host.mak),)
 include $(SRC_PATH)/tests/Makefile
+check: all
 endif
 ifeq ($(CONFIG_SMARTCARD_NSS),y)
 include $(SRC_PATH)/libcacard/Makefile
-- 
2.3.6

Re: [Qemu-devel] [PATCH] Makefile: Properly order build targets 'all' and 'check'

[Peter Maydell]

On 23 June 2015 at 13:30, Michal Privoznik <mprivozn@redhat.com> wrote:
> I'm used to run 'make -j5 all check'. However, this is not possible in
> qemu because of the missing dependency in the Makefile. If I do that,
> tests are usually started with build and since not everything is built
> yet, they often fail too. Moreover, we should run test suite only
> after every binary we want to test has been built.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Makefile b/Makefile
> index e7c5c3a..67eeb87 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -151,6 +151,7 @@ dummy := $(call unnest-vars,, \
>
>  ifneq ($(wildcard config-host.mak),)
>  include $(SRC_PATH)/tests/Makefile
> +check: all
>  endif
>  ifeq ($(CONFIG_SMARTCARD_NSS),y)
>  include $(SRC_PATH)/libcacard/Makefile

I'm having difficulty understanding the use of conditionals
in our makefile -- can you explain why inside this ifneq
rather than outside is the right place for this dependency?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Makefile: Properly order build targets 'all' and 'check'

[Michal Privoznik]

On 23.06.2015 14:37, Peter Maydell wrote:
> On 23 June 2015 at 13:30, Michal Privoznik <mprivozn@redhat.com> wrote:
>> I'm used to run 'make -j5 all check'. However, this is not possible in
>> qemu because of the missing dependency in the Makefile. If I do that,
>> tests are usually started with build and since not everything is built
>> yet, they often fail too. Moreover, we should run test suite only
>> after every binary we want to test has been built.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  Makefile | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Makefile b/Makefile
>> index e7c5c3a..67eeb87 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -151,6 +151,7 @@ dummy := $(call unnest-vars,, \
>>
>>  ifneq ($(wildcard config-host.mak),)
>>  include $(SRC_PATH)/tests/Makefile
>> +check: all
>>  endif
>>  ifeq ($(CONFIG_SMARTCARD_NSS),y)
>>  include $(SRC_PATH)/libcacard/Makefile
> 
> I'm having difficulty understanding the use of conditionals
> in our makefile -- can you explain why inside this ifneq
> rather than outside is the right place for this dependency?

Well, I guess it could be outside too. My thinking was, that we include
the tests/Makefile - which defines the 'check' target - only
conditionally, therefore the dependency should be defined only sometimes
too. I guess it won't hurt if it was outside too.

Michal

Re: [Qemu-devel] [PATCH] Makefile: Properly order build targets 'all' and 'check'

[Thomas Huth]

On Tue, 23 Jun 2015 14:42:35 +0200
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 23.06.2015 14:37, Peter Maydell wrote:
> > On 23 June 2015 at 13:30, Michal Privoznik <mprivozn@redhat.com> wrote:
> >> I'm used to run 'make -j5 all check'. However, this is not possible in
> >> qemu because of the missing dependency in the Makefile. If I do that,
> >> tests are usually started with build and since not everything is built
> >> yet, they often fail too. Moreover, we should run test suite only
> >> after every binary we want to test has been built.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  Makefile | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/Makefile b/Makefile
> >> index e7c5c3a..67eeb87 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -151,6 +151,7 @@ dummy := $(call unnest-vars,, \
> >>
> >>  ifneq ($(wildcard config-host.mak),)
> >>  include $(SRC_PATH)/tests/Makefile
> >> +check: all
> >>  endif
> >>  ifeq ($(CONFIG_SMARTCARD_NSS),y)
> >>  include $(SRC_PATH)/libcacard/Makefile
> > 
> > I'm having difficulty understanding the use of conditionals
> > in our makefile -- can you explain why inside this ifneq
> > rather than outside is the right place for this dependency?
> 
> Well, I guess it could be outside too. My thinking was, that we include
> the tests/Makefile - which defines the 'check' target - only
> conditionally, therefore the dependency should be defined only sometimes
> too. I guess it won't hurt if it was outside too.

Maybe it's simpler to add the dependency into tests/Makefile instead?

 Thomas

Re: [Qemu-devel] [PATCH] Makefile: Properly order build targets 'all' and 'check'

[Michal Privoznik]

On 23.06.2015 14:49, Thomas Huth wrote:
> On Tue, 23 Jun 2015 14:42:35 +0200
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
>> On 23.06.2015 14:37, Peter Maydell wrote:
>>> On 23 June 2015 at 13:30, Michal Privoznik <mprivozn@redhat.com> wrote:
>>>> I'm used to run 'make -j5 all check'. However, this is not possible in
>>>> qemu because of the missing dependency in the Makefile. If I do that,
>>>> tests are usually started with build and since not everything is built
>>>> yet, they often fail too. Moreover, we should run test suite only
>>>> after every binary we want to test has been built.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>  Makefile | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/Makefile b/Makefile
>>>> index e7c5c3a..67eeb87 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -151,6 +151,7 @@ dummy := $(call unnest-vars,, \
>>>>
>>>>  ifneq ($(wildcard config-host.mak),)
>>>>  include $(SRC_PATH)/tests/Makefile
>>>> +check: all
>>>>  endif
>>>>  ifeq ($(CONFIG_SMARTCARD_NSS),y)
>>>>  include $(SRC_PATH)/libcacard/Makefile
>>>
>>> I'm having difficulty understanding the use of conditionals
>>> in our makefile -- can you explain why inside this ifneq
>>> rather than outside is the right place for this dependency?
>>
>> Well, I guess it could be outside too. My thinking was, that we include
>> the tests/Makefile - which defines the 'check' target - only
>> conditionally, therefore the dependency should be defined only sometimes
>> too. I guess it won't hurt if it was outside too.
> 
> Maybe it's simpler to add the dependency into tests/Makefile instead?
> 

Yeah, that could work too. For some reason I thought that having it
there would result in making 'all' just under tests/. But Now that I
tried it out it works just nicely. Do you want me to post v2 or is that
something that can be fixed just before pushing?

Michal

Re: [Qemu-devel] [Qemu-trivial] [PATCH] Makefile: Properly order build targets 'all' and 'check'

[Michael Tokarev]

23.06.2015 16:35, Michal Privoznik wrote:
> On 23.06.2015 14:49, Thomas Huth wrote:

>> Maybe it's simpler to add the dependency into tests/Makefile instead?
>>
> 
> Yeah, that could work too. For some reason I thought that having it
> there would result in making 'all' just under tests/. But Now that I
> tried it out it works just nicely. Do you want me to post v2 or is that
> something that can be fixed just before pushing?

"Fixing" is not moving the new code to another file.  Please post a v2.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] Makefile: Properly order build targets 'all' and 'check'

[Peter Maydell]

On 23 June 2015 at 14:35, Michal Privoznik <mprivozn@redhat.com> wrote:
> Yeah, that could work too. For some reason I thought that having it
> there would result in making 'all' just under tests/. But Now that I
> tried it out it works just nicely.

Have you tested both "build in the source tree" and "build in
a separate directory from the source tree", by the way?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Makefile: Properly order build targets 'all' and 'check'

[Stefan Weil]

Am 23.06.2015 um 19:31 schrieb Peter Maydell:
> On 23 June 2015 at 14:35, Michal Privoznik <mprivozn@redhat.com> wrote:
>> Yeah, that could work too. For some reason I thought that having it
>> there would result in making 'all' just under tests/. But Now that I
>> tried it out it works just nicely.
> Have you tested both "build in the source tree" and "build in
> a separate directory from the source tree", by the way?
>
> thanks
> -- PMM

Both will work, as the modification only adds a dependency.

Do we care that running "make check" will take longer with this
patch? Make needs some time to check all dependencies for
"all", even if nothing has to be done.

I feel a little bit uneasy with something depending on all.
Maybe some day we'll want to include check in the default
build. Then all would depend on check which depends on
all which depends on check and so on. An intermediate
make target could solve that:

all: full-build
check: full-build
full-build: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all modules

Stefan

Re: [Qemu-devel] [PATCH] Makefile: Properly order build targets 'all' and 'check'

[Markus Armbruster]

Stefan Weil <sw@weilnetz.de> writes:

> Am 23.06.2015 um 19:31 schrieb Peter Maydell:
>> On 23 June 2015 at 14:35, Michal Privoznik <mprivozn@redhat.com> wrote:
>>> Yeah, that could work too. For some reason I thought that having it
>>> there would result in making 'all' just under tests/. But Now that I
>>> tried it out it works just nicely.
>> Have you tested both "build in the source tree" and "build in
>> a separate directory from the source tree", by the way?
>>
>> thanks
>> -- PMM
>
> Both will work, as the modification only adds a dependency.
>
> Do we care that running "make check" will take longer with this
> patch? Make needs some time to check all dependencies for
> "all", even if nothing has to be done.

If this bothers us, we could try making it an order-only prerequisite:

check: | all

https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html

> I feel a little bit uneasy with something depending on all.
> Maybe some day we'll want to include check in the default
> build. Then all would depend on check which depends on

I agree that depending on the default goal (here: all) isn't nice.

> all which depends on check and so on. An intermediate
> make target could solve that:
>
> all: full-build
> check: full-build
> full-build: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all modules

Re: [Qemu-devel] [PATCH] Makefile: Properly order build targets 'all' and 'check'

[Michal Privoznik]

On 25.06.2015 09:08, Markus Armbruster wrote:
> Stefan Weil <sw@weilnetz.de> writes:
> 
>> Am 23.06.2015 um 19:31 schrieb Peter Maydell:
>>> On 23 June 2015 at 14:35, Michal Privoznik <mprivozn@redhat.com> wrote:
>>>> Yeah, that could work too. For some reason I thought that having it
>>>> there would result in making 'all' just under tests/. But Now that I
>>>> tried it out it works just nicely.
>>> Have you tested both "build in the source tree" and "build in
>>> a separate directory from the source tree", by the way?
>>>
>>> thanks
>>> -- PMM
>>
>> Both will work, as the modification only adds a dependency.
>>
>> Do we care that running "make check" will take longer with this
>> patch? Make needs some time to check all dependencies for
>> "all", even if nothing has to be done.
> 
> If this bothers us, we could try making it an order-only prerequisite:
> 
> check: | all

I'm not sure this is the right approach. What is there to check if
nothing has been built? I think this dependency is not order-only. It
should be a real dependency.

> 
> https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
> 
>> I feel a little bit uneasy with something depending on all.
>> Maybe some day we'll want to include check in the default
>> build. Then all would depend on check which depends on
> 
> I agree that depending on the default goal (here: all) isn't nice.
> 
>> all which depends on check and so on. An intermediate
>> make target could solve that:
>>
>> all: full-build
>> check: full-build
>> full-build: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all modules

Well, if we ever do that, it can be done this way. Or any other way that
would be applicable to the code in the future.

Michal