Re: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
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.
I think adding some of the above reasoning to the commit message would
be helpful IMO.
> 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é
Thanks, Roger.
Re: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
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?
The switch to if_changed LGTM.
Thanks, Roger.
Re: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
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é
Thanks.
Jan
Re: [PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o
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.
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).
Jan
