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 [email protected] 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 <[email protected]>
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
