Re: [XEN PATCH for-4.17 v6 2/5] libs/light: Rework targets prerequisites

From: Anthony PERARD <anthony.perard@citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: <xen-devel@lists.xenproject.org>, Wei Liu <wl@xen.org>,
	Juergen Gross <jgross@suse.com>
Subject: Re: [XEN PATCH for-4.17 v6 2/5] libs/light: Rework targets prerequisites
Date: Tue, 7 Feb 2023 11:37:12 +0000	[thread overview]
Message-ID: <Y+I36LIDzNwiwI83@perard.uk.xensource.com> (raw)
In-Reply-To: <fff0eb32-cc4d-a3b5-c637-58f643cb644a@citrix.com>

On Mon, Feb 06, 2023 at 06:02:51PM +0000, Andrew Cooper wrote:
> On 20/01/2023 7:44 pm, Anthony PERARD wrote:
> > diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
> > index cd3fa855e1..b28447a2ae 100644
> > --- a/tools/libs/light/Makefile
> > +++ b/tools/libs/light/Makefile
> > @@ -178,13 +175,13 @@ libxl_x86_acpi.o libxl_x86_acpi.opic: CFLAGS += -I$(XEN_ROOT)/tools
> >  $(SAVE_HELPER_OBJS): CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenevtchn) $(CFLAGS_libxenguest)
> >  
> >  testidl.o: CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenlight)
> > -testidl.c: libxl_types.idl gentest.py $(XEN_INCLUDE)/libxl.h $(AUTOINCS)
> > -	$(PYTHON) gentest.py libxl_types.idl testidl.c.new
> > -	mv testidl.c.new testidl.c
> > +testidl.c: libxl_types.idl gentest.py
> > +	$(PYTHON) gentest.py $< $@.new
> > +	mv -f $@.new $@
> 
> Doesn't this want to be a mov-if-changed?
> 
> We don't need to force a rebuild if testidl.c hasn't changed, I don't think.

I don't like move-if-changed, as when the prerequisites happens to be
newer than the target, the rules keeps been executed on incremental
builds.

Also in this case, only two targets depends on this one, "testidl.o" and
"testidl", so move-if-changed would not be very useful.

> > @@ -208,14 +205,22 @@ _libxl_save_msgs_helper.h _libxl_save_msgs_callout.h: \
> >  	$(PERL) -w $< $@ >$@.new
> >  	$(call move-if-changed,$@.new,$@)
> >  
> > +#
> > +# headers dependencies on generated headers
> > +#
> >  $(XEN_INCLUDE)/libxl.h: $(XEN_INCLUDE)/_libxl_types.h
> >  $(XEN_INCLUDE)/libxl_json.h: $(XEN_INCLUDE)/_libxl_types_json.h
> >  libxl_internal.h: $(XEN_INCLUDE)/libxl.h $(XEN_INCLUDE)/libxl_json.h
> >  libxl_internal.h: _libxl_types_internal.h _libxl_types_private.h _libxl_types_internal_private.h
> > -libxl_internal_json.h: _libxl_types_internal_json.h
> > +libxl_internal.h: _libxl_save_msgs_callout.h
> >  
> > -$(OBJS-y) $(PIC_OBJS) $(LIBXL_TEST_OBJS) $(TEST_PROG_OBJS) $(SAVE_HELPER_OBJS): $(XEN_INCLUDE)/libxl.h
> > +#
> > +# objects dependencies on headers that depends on generated headers
> > +#
> > +$(TEST_PROG_OBJS): $(XEN_INCLUDE)/libxl.h
> >  $(OBJS-y) $(PIC_OBJS) $(LIBXL_TEST_OBJS): libxl_internal.h
> > +$(SAVE_HELPER_OBJS): $(XEN_INCLUDE)/libxl.h _libxl_save_msgs_helper.h
> > +testidl.o: $(XEN_INCLUDE)/libxl.h
> 
> I know you're just rearranging existing the existing logic, but why
> doesn't `#include <libxl.h>` not generate suitable dependences ?

Make doesn't know how to read *.c files ;-). But more importantly, we
don't have any logic to generate dependencies ahead of building the
object, so make doesn't know about it.

Thanks,

-- 
Anthony PERARD