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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>

Thanks.

Jan