Re: [XEN PATCH v2 4/5] build: evaluate XEN_BUILD_* and XEN_DOMAIN on first use

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.07.2023 11:15, Anthony PERARD wrote:
> On Fri, Jun 23, 2023 at 10:07:02AM +0200, Jan Beulich wrote:
>> On 22.06.2023 17:30, Anthony PERARD wrote:
>>> --- a/xen/Makefile
>>> +++ b/xen/Makefile
>>> @@ -11,10 +11,10 @@ export XEN_FULLVERSION   = 
>>> $(XEN_VERSION).$(XEN_SUBVERSION)$(XEN_EXTRAVERSION)
>>>  -include xen-version
>>>  
>>>  export XEN_WHOAMI  ?= $(USER)
>>> -export XEN_DOMAIN  ?= $(shell ([ -x /bin/dnsdomainname ] && 
>>> /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo 
>>> [unknown]))
>>> -export XEN_BUILD_DATE      ?= $(shell LC_ALL=C date)
>>> -export XEN_BUILD_TIME      ?= $(shell LC_ALL=C date +%T)
>>> -export XEN_BUILD_HOST      ?= $(shell hostname)
>>> +export XEN_DOMAIN  ?= $(eval XEN_DOMAIN := $(shell ([ -x 
>>> /bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && 
>>> /bin/domainname || echo [unknown])))$(XEN_DOMAIN)
>>> +export XEN_BUILD_DATE      ?= $(eval XEN_BUILD_DATE := $(shell LC_ALL=C 
>>> date))$(XEN_BUILD_DATE)
>>> +export XEN_BUILD_TIME      ?= $(eval XEN_BUILD_TIME := $(shell LC_ALL=C 
>>> date +%T))$(XEN_BUILD_TIME)
>>> +export XEN_BUILD_HOST      ?= $(eval XEN_BUILD_HOST := $(shell 
>>> hostname))$(XEN_BUILD_HOST)
>>
>> Interesting approach. It looks like it should be independent of make's
>> internal workings, but I still wonder: Is this documented somewhere,
>> so we won't be caught by surprise of it not working anymore because of
>> some change to make's internals?
> 
> So, this is a makefile trick that I've seen on someone's blog post.
> 
> But I tried to find evidence in the GNU make manual if variable expansion is
> expected to work like that, and I can't. So I can imagine one day make
> doing expansion of variable in parallel, or were the result of the eval
> would happen only on the next line. So I don't know if this approach is
> always going to work.
> 
> Maybe we could go for a safer alternative:
> 
> Simply replacing ?= by something actually documented in the manual, and
> then replacing = by := .
> 
>     ifeq ($(origin XEN_BUILD_DATE), undefined)
>     export XEN_BUILD_DATE := $(shell LC_ALL=C date)
>     endif
> 
> An alternative, with a macro could be:
> 
>     set-shell-if-undef = $(if $(filter undefined,$(origin $(1))),$(eval $(1) 
> := $(shell $(2))))
> 
>     $(call set-shell-if-undef,XEN_BUILD_DATE,LC_ALL=C date)
>     export XEN_BUILD_DATE
> 
> But this kind of hide that a shell command is been called. But having
> $(shell) as parameter of call $(call) mean the shell command is always
> called even when unneeded.
> 
> But then, with the macro, I'm not sure where to put it, to be able to
> use it here and in Config.mk for the next patch, another file in
> xen.git/config/*.mk ?
> 
> Should I replace all the eval with "ifeq (); endif" ?

I think that would be best (and I prefer that form anyway for being more
clear as to what it does).

Jan