Re: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	George Dunlap <George.Dunlap@eu.citrix.com>
Subject: Re: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
Date: Tue, 15 Sep 2020 17:14:33 +0200	[thread overview]
Message-ID: <4c1baacd-efc0-37e1-037f-64d3914a02c9@suse.com> (raw)
In-Reply-To: <20200915134630.GM753@Air-de-Roger>

On 15.09.2020 15:46, Roger Pau Monné wrote:
> On Tue, Sep 15, 2020 at 02:26:34PM +0200, Jan Beulich wrote:
>> On 15.09.2020 13:56, Roger Pau Monné wrote:
>>> On Mon, Sep 14, 2020 at 12:15:39PM +0200, Jan Beulich wrote:
>>>> Switch to $(call if_changed,ld) where possible; presumably not doing so
>>>> in e321576f4047 ("xen/build: start using if_changed") right away was an
>>>> oversight, as it did for Arm in (just) one case. It failed to add
>>>> prelink.o to $(targets), though, causing - judging from the observed
>>>> behavior on x86 - undue rebuilds of the final binary (because of
>>>> prelink.o getting rebuild for $(cmd_prelink.o) being empty, in turn
>>>> because of .prelink.o.cmd not getting read) during "make install-xen".
>>>
>>> I'm not sure I follow why prelink.o needs to be added to targets, does
>>> this offer some kind of protection against rebuilds when doing make
>>> install?
>>
>> In a way, but (as I view it) not really. It is the use of ...
>>
>>> The switch to if_changed LGTM.
>>
>> ... if_changed which requires this. .*.cmd files will only be loaded
>> for anything explicitly or implicitly listed as a target. While .o
>> coming from $(obj-y) get added there automatically, prelink.o is not
>> something that could be recognized as needing adding, hence the
>> "manual" insertion.
> 
> This seems very prone to mistakes, as you have to remember that
> whatever object that uses if_changed should also be part of the
> targets set, or else it will get rebuild unconditionally.

The use of $(if_changed ...) has further similar pitfalls: One may
not forget to add FORCE to the dependencies, and there may not be
blanks after the comma (where one would usually put one). But I think
the benefits of this construct are so significant that this extra
care that's needed is well justified. Plus this is actually mentioned
(at least in passing) in docs/misc/xen-makefiles/makefile.rst.

> I think adding some of the above reasoning to the commit message would
> be helpful IMO.

Well, I've merely re-explained what I think the commit message
already says.

>> Without .prelink.o.cmd loaded, $(if_changed ) will always arrange
>> for it to get re-built, because it then will consider the command
>> used to build the file to have changed (as the stored one appears to
>> be empty).
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Jan