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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/01/2023 7:44 pm, Anthony PERARD wrote:
> No need for $(AUTOSRCS), GNU make can generate them as needed when
> trying to build them as needed when trying to build the object. Also,
> those two AUTOSRCS don't need to be a prerequisite of "all". As for
> the "clean" target, those two files are already removed via "_*.c".
>
> We don't need $(AUTOINCS) either:
> - As for both _libxl_save_msgs*.h headers, we are adding more
>   selective dependencies so the headers will still be generated as
>   needed.
> - "clean" rule already delete the _*.h files, so AUTOINCS aren't needed
>   there.
>
> "libxl_internal_json.h" doesn't seems to have ever existed, so the
> dependency is removed.
>
> Rework objects prerequisites, to have them dependents on either
> "libxl.h" or "libxl_internal.h". "libxl.h" is not normally included
> directly in the source code as "libxl_internal.h" is used instead, but
> we have "libxl.h" as prerequisite of "libxl_internal.h", so generated
> headers will still be generated as needed.
>
> Make doesn't need "libxl.h" to generate "testidl.c", "libxl.h" is only
> needed later when building "testidl.o". This avoid the need to
> regenerate "testidl.c" when only "libxl.h" changed. Also use automatic
> variables $< and $@.
>
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
>
> Notes:
>     v6:
>     - rebased, part of the patch commited as 4ff0811
>     - reword commit message
>     
>     v4:
>     - new patch
>
>  tools/libs/light/Makefile | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
>
> 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
> @@ -148,9 +148,6 @@ LIBXL_TEST_OBJS += $(foreach t, 
> $(LIBXL_TESTS_INSIDE),libxl_test_$t.opic)
>  TEST_PROG_OBJS += $(foreach t, $(LIBXL_TESTS_PROGS),test_$t.o) test_common.o
>  TEST_PROGS += $(foreach t, $(LIBXL_TESTS_PROGS),test_$t)
>  
> -AUTOINCS = _libxl_save_msgs_callout.h _libxl_save_msgs_helper.h
> -AUTOSRCS = _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
> -
>  CLIENTS = testidl libxl-save-helper
>  
>  SAVE_HELPER_OBJS = libxl_save_helper.o _libxl_save_msgs_helper.o
> @@ -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.

>  
> -all: $(CLIENTS) $(TEST_PROGS) $(AUTOSRCS) $(AUTOINCS)
> +all: $(CLIENTS) $(TEST_PROGS)
>  
> -$(OBJS-y) $(PIC_OBJS) $(SAVE_HELPER_OBJS) $(LIBXL_TEST_OBJS) 
> $(TEST_PROG_OBJS): $(AUTOINCS) libxl.api-ok
> +$(OBJS-y) $(PIC_OBJS) $(SAVE_HELPER_OBJS) $(LIBXL_TEST_OBJS) 
> $(TEST_PROG_OBJS): libxl.api-ok
>  
>  $(DSDT_FILES-y): acpi
>  
> @@ -196,7 +193,7 @@ libxl.api-ok: check-libxl-api-rules _libxl.api-for-check
>       $(PERL) $^
>       touch $@
>  
> -_libxl.api-for-check: $(XEN_INCLUDE)/libxl.h $(AUTOINCS)
> +_libxl.api-for-check: $(XEN_INCLUDE)/libxl.h
>       $(CC) $(CPPFLAGS) $(CFLAGS) -c -E $< $(APPEND_CFLAGS) \
>               -DLIBXL_EXTERNAL_CALLERS_ONLY=LIBXL_EXTERNAL_CALLERS_ONLY \
>               >$@.new
> @@ -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 ?

~Andrew