[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Anthony PERARD <anthony.perard@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 11 Oct 2021 15:28:26 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=W9kzb4yBJ9OGJMK0JLjIkh6eFopaFEA/AGpXNmsx6zw=; b=VynkAk2uIjjHMp9BqD4hQkgxNSAvlxqOKkw9zOOoeoXSm5xB4vQJDpNY1L7VBfLsBKVRuEEK4xChZ/v3D78MKQGAkSnbIzpbYv4hZUrFX+PdNh7+YTlbZSFcpFe/d158HgpYvpOR1YvR2ioINCHsT0g3F4jbjKWJ67QCWaFYro3vNsQ5nanDmKB9ZzQYbDHOvo72aL8V8fXA1hTJ4K8qzzKi3jb1PL3hoyUFucvKnYXk5aJFIQPPRjnokzyLyJt8ZieBXOldPo0BO1uTTQV2kZ/p/2c6dZva7MdJaMIZ1VHEq7of4O7GKt5TXu2XHJ05NymeG+YB0Dw03YHa2PiDSQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=G33pFvyomwleGXyV1O+MvVLvAZ1xvccCscBAlpB5e3eX5gYQD/z9cVaC5qlFviYzUSAfCs3SnCI5io+VhNPWbD/hYTJWx07MH3Ic4O7jLC80WAxa2Na9TkCJUAiLCpC8ukEJoD+b2m6E6vTtf48iYmj/YPjoMrVv5HTiqwsbS5r04C3aXbdeH+/7Et1x1U6bbv6weh3gACC0Kpfi5P48KzC8bFhBT3avp1ok5/DvJtPYK84TF45pDxH2F9toeotL56a92ot1bebe+b3ljpIR68uZJz6TpIt8JmN4R55TMUF+Wec+OmO5YcVj1FhUNDBRezPDj6WJ7MrhMmUaX0nsmw==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 11 Oct 2021 13:28:42 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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)))), \

or (less desirable imo)

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




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.