Re: [Qemu-devel] [PATCH v6 04/15] Makefile: Rules for docker testing

[Fam Zheng]

On Tue, 05/31 14:02, Paolo Bonzini wrote:
> 
> 
> On 31/05/2016 13:00, Fam Zheng wrote:
> > On Tue, 05/31 10:51, Paolo Bonzini wrote:
> >>> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> >>> new file mode 100644
> >>> index 0000000..372733d
> >>> --- /dev/null
> >>> +++ b/tests/docker/Makefile.include
> >>> @@ -0,0 +1,121 @@
> >>> +# Makefile for Docker tests
> >>> +
> >>> +include $(SRC_PATH)/rules.mak
> >>
> >> Why include this _and_ include tests/docker/Makefile.include from the
> >> top Makefile?
> > 
> > This is for quiet-command, which is only conditionally included by top
> > Makefile.
> 
> Ah, it's for the case when configure has not been executed yet and the
> toplevel Makefile "assumes we are in the search tree".
> 
> >>
> >> I think you should do one of this:
> >>
> >> a) drop this inclusion; nice, but it pollutes the toplevel makefile a bit
> >>
> >> b) do the following:
> >>
> >> - link this file into the build tree in configure
> >>
> >> - include ../../config-host.mak
> > 
> > I prefer we support running from the src tree without running configure, but
> > $(MAKE) invocations doesn't propagate make variables such as SRC_PATH...
> > 
> >>
> >> - add to the toplevel Makefile a rule like
> >>
> >>    docker docker-%:
> >>            $(MAKE) -C tests/docker $@
> > 
> > ... and explicitly passing it (and $(V), etc.) here seems very ad-hocery.
> 
> V and others would be passed down to the recursive make.  Only SRC_PATH
> would not be passed down.

Yes, you are right. So it certainly will work, I just preferred recursive
include over resursive make for consistency (with tests/Makefile).

> 
> >>
> >> I prefer the latter.  Either would make patch 3 unnecessary.
> > 
> > Maybe I should make patch 3 a patch to make top Makefile include rules.mak
> > unconditionally?
> 
> Yeah, that would be good.
> 
> I'm still a bit undecided about the pollution introduced by
> tests/docker/Makefile.include, but I guess that's okay.

I think it's also okay to switch to "make -C tests/docker" for docker targets
(so "make docker" becomes "make -C tests/docker help"), this way the top
Makefile is not touched.

> 
> By the way, could you prepare a patch to rename tests/Makefile to
> tests/Makefile.include?  It's a good convention.

Sounds good, will do.

Fam