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).

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).

Restore the earlier behavior for both aspects.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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".)

--- 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 $@)
 	$(call if_changed,compile.h)
 
 targets += include/xen/compile.h

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?

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.

> 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?

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.

> Restore the earlier behavior for both aspects.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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".)
> 
> --- 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?

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

Thanks,

-- 
Anthony PERARD

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@suse.com>
>> ---
>> 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

On Thu, Jan 13, 2022 at 01:11:41PM +0100, Jan Beulich wrote:
> 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.

I see, I guess keeping that behavior is kind of useful (banner been
printed when compile.h is regenerated).

> >> +	$(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?

I guess it is fine to keep the '-v'. It isn't like it is something that
will happen very often.

> > 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.

With $(Q) added:
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD