Re: [PATCH] build: adjust include/xen/compile.h generation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.01.2022 12:22, Anthony PERARD wrote:
> On Tue, Jan 11, 2022 at 03:16:17PM +0100, Jan Beulich wrote:
>> Prior to 19427e439e01 ("build: generate "include/xen/compile.h" with
>> if_changed") running "make install-xen" as root would not have printed
>> the banner under normal circumstances. Its printing would instead have
>> indicated that something was wrong (or during a normal build the lack
>> of printing would do so).
> 
> So, having several line of logs with one generating "compile.h", and
> several object rebuild plus the re-linking of xen isn't enough has to
> indicate that something is wrong?

Well, for warnings and errors to be easy to spot (and until your rework
to make our build more Linux-like is in place) passing -s to make is a
must, imo.

> Also, with this patch, the banner would no longer be printed on rebuild,
> unless one doesn't try to prevent regeneration of "compile.h" by
> exporting two variables.
> 
> I kind of like having the banner been nearly always printed, but I'm not
> opposed to the change.

I'd be happy to use a solution where the banner is always printed for
"normal" builds, but not when compile.h re-generation is skipped
during "install-xen" as root. Assuming of course that was the behavior
prior to your changes - I never tried suppressing compile.h updating
via setting XEN_BUILD_{DATE,TIME}. My goal merely is that it not be
printed during "install-xen" as root, as that's how things had been
for many years.

>> Further aforementioned change had another undesirable effect, which I
>> didn't notice during review: Originally compile.h would have been
>> re-generated (and final binaries re-linked) when its dependencies were
>> updated after an earlier build. This is no longer the case now, which
>> means that if some other file also was updated, then the re-build done
>> during "make install-xen" would happen with a stale compile.h (as its
>> updating is suppressed in this case).
> 
> So, the comment:
>     Don't refresh this files during e.g., 'sudo make install'
> wasn't correct?

In a way, yes. The file would have got refreshed for two reasons: By
deleting it (i.e. unconditionally) during a normal build, but via
dependencies only during "install-xen" as root.

> On the other hand, it's probably not good to not regen the file when the
> prerequisite changes. It's kind of weird to "rm" the target, but I don't
> have a better solution at the moment.

I agree it's weird, but I've outlined the only alternative to this
that I see, ...

>> Restore the earlier behavior for both aspects.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> An alternative to removing the target would be to replace "! -r $@" by
>> "-n '$?'" in the shell "if", but that would cause unhelpful alteration
>> of what gets recorded in include/xen/.compile.h.cmd. (The command
>> normally changes every time anyway, due to the inclusion of
>> $(XEN_BUILD_TIME), but I consider that different from varying the
>> conditions of the "if".)

... here.

>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -424,6 +424,7 @@ targets += .banner
>>  quiet_cmd_compile.h = UPD     $@
>>  define cmd_compile.h
>>      if [ ! -r $@ -o -O $@ ]; then \
>> +    cat .banner; \
>>      sed -e 's/@@date@@/$(XEN_BUILD_DATE)/g' \
>>          -e 's/@@time@@/$(XEN_BUILD_TIME)/g' \
>>          -e 's/@@whoami@@/$(XEN_WHOAMI)/g' \
>> @@ -441,7 +442,7 @@ define cmd_compile.h
>>  endef
>>  
>>  include/xen/compile.h: include/xen/compile.h.in .banner FORCE
>> -    @cat .banner
>> +    $(if $(filter-out FORCE,$?),rm -fv $@)
> 
> Is there a reason for -v? Do we care if the file existed?

That's meant to be an indication of the file getting updated during
"install-xen" as root. I thought it might be nice to have this extra
indicator, but I wouldn't mind dropping it if that helps acceptance
of the change. Can you let me know how important this aspect is to
you?

> Do we want to log "rm -f compile.h" ? Or could you just prefix the line
> with $(Q)?

I'll add $(Q). As said, I always build with "make -s" (except when
debugging weird build issues), so this is nothing I would have noticed.

Jan