Re: [XEN PATCH v7 25/51] build: rework test/livepatch/Makefile

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.08.2021 12:50, Anthony PERARD wrote:
> This rework the livepatch/Makefile to make it less repetitive and make
> use of the facilities. All the targets to be built are now listed in
> $(extra-y) which will allow Rules.mk to build them without the need of
> a local target in a future patch.
> 
> There are some changes/fixes in this patch:
> - when "xen-syms" is used for a target, it is added to the dependency
>   list of the target, which allow to rebuild the target when xen-syms
>   changes. But if "xen-syms" is missing, make simply fails.
> - modinfo.o wasn't removing it's $@.bin file like the other targets,
>   this is now done.
> - The command to build *.livepatch targets as been fixed to use
>   $(XEN_LDFLAGS) rather than just $(LDFLAGS) which is a fallout from
>   2740d96efdd3 ("xen/build: have the root Makefile generates the
>   CFLAGS")
> 
> make will findout the dependencies of the *.livepatch files and thus
> what to built by "looking" at the objects listed in the *-objs
> variables. The actual dependencies is generated by the new
> "multi_depend" macro.
> 
> "$(targets)" needs to be updated with the objects listed in the
> different *-objs variables to allow make to load the .*.cmd dependency
> files.
> 
> This patch copies the macro "multi_depend" from Linux 5.12.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Just two and a half remarks; I'd really like the livepatch maintainers
to properly review this change.

> --- a/xen/scripts/Kbuild.include
> +++ b/xen/scripts/Kbuild.include
> @@ -151,3 +151,12 @@ why =                                                    
>                     \
>  
>  echo-why = $(call escsq, $(strip $(why)))

Not the least seeing this in context, ...

>  endif
> +
> +# Useful for describing the dependency of composite objects
> +# Usage:
> +#   $(call multi_depend, multi_used_targets, suffix_to_remove, suffix_to_add)
> +define multi_depend

... I would wish we wouldn't introduce further names with underscores
in them when dashes are valid to be used.

> +$(foreach m, $(notdir $1), \
> +     $(eval $(obj)/$m: \
> +     $(addprefix $(obj)/, $(foreach s, $3, $($(m:%$(strip $2)=%$(s)))))))

I'd like to suggest to be consistent here: Either $(s) and then also
$(m) in both places, or $m and then also $s.

> --- a/xen/test/livepatch/Makefile
> +++ b/xen/test/livepatch/Makefile
>[...]
> +$(obj)/%.livepatch: FORCE
> +     $(call if_changed,livepatch)
> +
> +$(call multi_depend, $(filter %.livepatch,$(extra-y)), .livepatch, -objs)
> +targets += $(sort $(foreach m,$(basename $(notdir $(filter 
> %.livepatch,$(extra-y)))), \
> +    $($(m)-objs)))

I think it would help readability if the 2nd line was properly indented to
reflect the pending open parentheses:

targets += $(sort $(foreach m,$(basename $(notdir $(filter 
%.livepatch,$(extra-y)))), \
                    $($(m)-objs)))

or (less desirable imo)

targets += $(sort \
             $(foreach m,$(basename $(notdir $(filter 
%.livepatch,$(extra-y)))), \
               $($(m)-objs)))

Jan