[XEN PATCH v2 0/5] build: reduce number of $(shell) execution on make 4.4

[XEN PATCH v2 0/5] build: reduce number of $(shell) execution on make 4.4

[Anthony PERARD]

Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.build-exported-shell-command-value-v2

v2:
- new patches removing TARGET_SUBARCH and TARGET_ARCH.
- style change in first patch

With GNU make 4.4, the number of execution of the command present in $(shell )
in exported variables increased greatly. This is probably because as of make
4.4, exported variable are also added to the environment of $(shell )
construct.

From the annoncement:

    https://lists.gnu.org/archive/html/info-gnu/2022-10/msg00008.html
    > * WARNING: Backward-incompatibility!
    >   Previously makefile variables marked as export were not exported to commands
    >   started by the $(shell ...) function.  Now, all exported variables are
    >   exported to $(shell ...).  If this leads to recursion during expansion, then
    >   for backward-compatibility the value from the original environment is used.
    >   To detect this change search for 'shell-export' in the .FEATURES variable.

Also, there's a new paragraph in the GNU make manual, but it's a warning about
exporting all variable, still it might be relevant to the new observed
behavior:

    https://www.gnu.org/software/make/manual/make.html#Variables_002fRecursion
    > Adding a variable’s value to the environment requires it to be expanded. If
    > expanding a variable has side-effects (such as the info or eval or similar
    > functions) then these side-effects will be seen every time a command is
    > invoked.

The issue was reported on IRC by jandryuk.

So, I've locate a few obvious candidate to fix, maybe there's more $(shell) to
change?

Anthony PERARD (5):
  build: define ARCH and SRCARCH later
  build: remove TARGET_SUBARCH, a duplicate of ARCH
  build: remove TARGET_ARCH, a duplicates of SRCARCH
  build: evaluate XEN_BUILD_* and XEN_DOMAIN on first use
  Config.mk: evaluate XEN_COMPILE_ARCH and XEN_OS on first use

 Config.mk              |  6 +++---
 xen/Makefile           | 40 ++++++++++++++++++----------------------
 xen/Rules.mk           |  2 +-
 xen/arch/riscv/arch.mk |  2 +-
 xen/build.mk           |  6 +++---
 5 files changed, 26 insertions(+), 30 deletions(-)

-- 
Anthony PERARD

[XEN PATCH v2 1/5] build: define ARCH and SRCARCH later

[Anthony PERARD]

Defining ARCH and SRCARCH later in xen/Makefile allows to switch to
immediate evaluation variable type.

ARCH and SRCARCH depends on value defined in Config.mk and aren't used
TARGET_SUBARCH or TARGET_ARCH, and not before it's needed in a
sub-make or a rule.

This will help reduce the number of times the shell rune is been
run.

With GNU make 4.4, the number of execution of the command present in
these $(shell ) increased greatly. This is probably because as of make
4.4, exported variable are also added to the environment of $(shell )
construct.

Also, `make -d` shows a lot of these:
    Makefile:39: not recursively expanding SRCARCH to export to shell function
    Makefile:38: not recursively expanding ARCH to export to shell function

Reported-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Tested-by: Jason Andryuk <jandryuk@gmail.com>
---

Notes:
    v2:
    - change quoting style
    - change identation

 xen/Makefile | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/Makefile b/xen/Makefile
index e89fc461fc..78d176f04e 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -35,12 +35,6 @@ MAKEFLAGS += -rR
 
 EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
 
-ARCH=$(XEN_TARGET_ARCH)
-SRCARCH=$(shell echo $(ARCH) | \
-          sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
-              -e s'/riscv.*/riscv/g')
-export ARCH SRCARCH
-
 # Allow someone to change their config file
 export KCONFIG_CONFIG ?= .config
 
@@ -241,6 +235,13 @@ include scripts/Kbuild.include
 include $(XEN_ROOT)/Config.mk
 
 # Set ARCH/SUBARCH appropriately.
+
+ARCH := $(XEN_TARGET_ARCH)
+SRCARCH := $(shell echo $(ARCH) | \
+    sed -e 's/x86.*/x86/' -e 's/arm\(32\|64\)/arm/g' \
+        -e 's/riscv.*/riscv/g')
+export ARCH SRCARCH
+
 export TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
 export TARGET_ARCH     := $(shell echo $(XEN_TARGET_ARCH) | \
                             sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
-- 
Anthony PERARD

[XEN PATCH v2 2/5] build: remove TARGET_SUBARCH, a duplicate of ARCH

[Anthony PERARD]

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    v2:
    - new patch

 xen/Makefile | 3 +--
 xen/build.mk | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/Makefile b/xen/Makefile
index 78d176f04e..bc639a1f80 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -234,7 +234,7 @@ include scripts/Kbuild.include
 # we need XEN_TARGET_ARCH to generate the proper config
 include $(XEN_ROOT)/Config.mk
 
-# Set ARCH/SUBARCH appropriately.
+# Set ARCH/SRCARCH appropriately.
 
 ARCH := $(XEN_TARGET_ARCH)
 SRCARCH := $(shell echo $(ARCH) | \
@@ -242,7 +242,6 @@ SRCARCH := $(shell echo $(ARCH) | \
         -e 's/riscv.*/riscv/g')
 export ARCH SRCARCH
 
-export TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
 export TARGET_ARCH     := $(shell echo $(XEN_TARGET_ARCH) | \
                             sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
                                 -e s'/riscv.*/riscv/g')
diff --git a/xen/build.mk b/xen/build.mk
index 30d74d4772..7e33e710fd 100644
--- a/xen/build.mk
+++ b/xen/build.mk
@@ -41,7 +41,7 @@ include/xen/compile.h: include/xen/compile.h.in .banner FORCE
 targets += include/xen/compile.h
 
 -include $(wildcard .asm-offsets.s.d)
-asm-offsets.s: arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.c
+asm-offsets.s: arch/$(TARGET_ARCH)/$(ARCH)/asm-offsets.c
 	$(CC) $(call cpp_flags,$(c_flags)) -S -g0 -o $@.new -MQ $@ $<
 	$(call move-if-changed,$@.new,$@)
 
-- 
Anthony PERARD

[XEN PATCH v2 3/5] build: remove TARGET_ARCH, a duplicates of SRCARCH

[Anthony PERARD]

The same command is used to generate the value of both $(TARGET_ARCH)
and $(SRCARCH), as $(ARCH) is an alias for $(XEN_TARGET_ARCH).

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    v2:
    - new patch

 xen/Makefile           | 18 +++++++-----------
 xen/Rules.mk           |  2 +-
 xen/arch/riscv/arch.mk |  2 +-
 xen/build.mk           |  6 +++---
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/xen/Makefile b/xen/Makefile
index bc639a1f80..ac2765050b 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -242,10 +242,6 @@ SRCARCH := $(shell echo $(ARCH) | \
         -e 's/riscv.*/riscv/g')
 export ARCH SRCARCH
 
-export TARGET_ARCH     := $(shell echo $(XEN_TARGET_ARCH) | \
-                            sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
-                                -e s'/riscv.*/riscv/g')
-
 export CONFIG_SHELL := $(SHELL)
 export CC CXX LD NM OBJCOPY OBJDUMP ADDR2LINE
 export YACC = $(if $(BISON),$(BISON),bison)
@@ -262,7 +258,7 @@ export XEN_TREEWIDE_CFLAGS := $(CFLAGS)
 ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),)
 CLANG_FLAGS :=
 
-ifeq ($(TARGET_ARCH),x86)
+ifeq ($(SRCARCH),x86)
 # The tests to select whether the integrated assembler is usable need to happen
 # before testing any assembler features, or else the result of the tests would
 # be stale if the integrated assembler is not used.
@@ -430,22 +426,22 @@ endif
 
 ifdef building_out_of_srctree
     CFLAGS += -I$(objtree)/include
-    CFLAGS += -I$(objtree)/arch/$(TARGET_ARCH)/include
+    CFLAGS += -I$(objtree)/arch/$(SRCARCH)/include
 endif
 CFLAGS += -I$(srctree)/include
-CFLAGS += -I$(srctree)/arch/$(TARGET_ARCH)/include
+CFLAGS += -I$(srctree)/arch/$(SRCARCH)/include
 
 # Note that link order matters!
 ALL_OBJS-y                := common/built_in.o
 ALL_OBJS-y                += drivers/built_in.o
 ALL_OBJS-y                += lib/built_in.o
 ALL_OBJS-y                += xsm/built_in.o
-ALL_OBJS-y                += arch/$(TARGET_ARCH)/built_in.o
+ALL_OBJS-y                += arch/$(SRCARCH)/built_in.o
 ALL_OBJS-$(CONFIG_CRYPTO) += crypto/built_in.o
 
 ALL_LIBS-y                := lib/lib.a
 
-include $(srctree)/arch/$(TARGET_ARCH)/arch.mk
+include $(srctree)/arch/$(SRCARCH)/arch.mk
 
 # define new variables to avoid the ones defined in Config.mk
 export XEN_CFLAGS := $(CFLAGS)
@@ -588,8 +584,8 @@ $(TARGET): outputmakefile FORCE
 	$(Q)$(MAKE) $(build)=tools
 	$(Q)$(MAKE) $(build)=. include/xen/compile.h
 	$(Q)$(MAKE) $(build)=include all
-	$(Q)$(MAKE) $(build)=arch/$(TARGET_ARCH) include
-	$(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
+	$(Q)$(MAKE) $(build)=arch/$(SRCARCH) include
+	$(Q)$(MAKE) $(build)=. arch/$(SRCARCH)/include/asm/asm-offsets.h
 	$(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@
 
 SUBDIRS = xsm arch common crypto drivers lib test
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 59072ae8df..8af3dd7277 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -180,7 +180,7 @@ cpp_flags = $(filter-out -Wa$(comma)% -flto,$(1))
 c_flags = -MMD -MP -MF $(depfile) $(XEN_CFLAGS)
 a_flags = -MMD -MP -MF $(depfile) $(XEN_AFLAGS)
 
-include $(srctree)/arch/$(TARGET_ARCH)/Rules.mk
+include $(srctree)/arch/$(SRCARCH)/Rules.mk
 
 c_flags += $(_c_flags)
 a_flags += $(_c_flags)
diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk
index 7448f759b4..8403f96b6f 100644
--- a/xen/arch/riscv/arch.mk
+++ b/xen/arch/riscv/arch.mk
@@ -15,5 +15,5 @@ riscv-march-$(CONFIG_RISCV_ISA_C)       := $(riscv-march-y)c
 CFLAGS += -march=$(riscv-march-y) -mstrict-align -mcmodel=medany
 
 # TODO: Drop override when more of the build is working
-override ALL_OBJS-y = arch/$(TARGET_ARCH)/built_in.o
+override ALL_OBJS-y = arch/$(SRCARCH)/built_in.o
 override ALL_LIBS-y =
diff --git a/xen/build.mk b/xen/build.mk
index 7e33e710fd..26dd5a8e87 100644
--- a/xen/build.mk
+++ b/xen/build.mk
@@ -41,11 +41,11 @@ include/xen/compile.h: include/xen/compile.h.in .banner FORCE
 targets += include/xen/compile.h
 
 -include $(wildcard .asm-offsets.s.d)
-asm-offsets.s: arch/$(TARGET_ARCH)/$(ARCH)/asm-offsets.c
+asm-offsets.s: arch/$(SRCARCH)/$(ARCH)/asm-offsets.c
 	$(CC) $(call cpp_flags,$(c_flags)) -S -g0 -o $@.new -MQ $@ $<
 	$(call move-if-changed,$@.new,$@)
 
-arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: asm-offsets.s
+arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s
 	@(set -e; \
 	  echo "/*"; \
 	  echo " * DO NOT MODIFY."; \
@@ -87,4 +87,4 @@ endif
 targets += prelink.o
 
 $(TARGET): prelink.o FORCE
-	$(Q)$(MAKE) $(build)=arch/$(TARGET_ARCH) $@
+	$(Q)$(MAKE) $(build)=arch/$(SRCARCH) $@
-- 
Anthony PERARD

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

[Anthony PERARD]

With GNU make 4.4, the number of execution of the command present in
these $(shell ) increased greatly. This is probably because as of make
4.4, exported variable are also added to the environment of $(shell )
construct.

Also, `make -d` shows a lot of these:
    Makefile:15: not recursively expanding XEN_BUILD_DATE to export to shell function
    Makefile:16: not recursively expanding XEN_BUILD_TIME to export to shell function
    Makefile:17: not recursively expanding XEN_BUILD_HOST to export to shell function
    Makefile:14: not recursively expanding XEN_DOMAIN to export to shell function

So, to avoid having these command been run more than necessery, we
will use a construct to evaluate on first use.

Reported-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Tested-by: Jason Andryuk <jandryuk@gmail.com>
---
 xen/Makefile | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/Makefile b/xen/Makefile
index ac2765050b..e3b1468f83 100644
--- 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)
 
 # Best effort attempt to find a python interpreter, defaulting to Python 3 if
 # available.  Fall back to just `python` if `which` is nowhere to be found.
-- 
Anthony PERARD

[XEN PATCH v2 5/5] Config.mk: evaluate XEN_COMPILE_ARCH and XEN_OS on first use

[Anthony PERARD]

With GNU make 4.4, the number of execution of the command present in
these $(shell ) increased greatly. This is probably because as of make
4.4, exported variable are also added to the environment of $(shell )
construct.

So, to avoid having these command been run more than necessery, we
will use a construct to evaluate on first use.

Reported-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Tested-by: Jason Andryuk <jandryuk@gmail.com>
---
 Config.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Config.mk b/Config.mk
index c529b1ba19..5fbdbc4500 100644
--- a/Config.mk
+++ b/Config.mk
@@ -19,13 +19,13 @@ or       = $(if $(strip $(1)),$(1),$(if $(strip $(2)),$(2),$(if $(strip $(3)),$(
 
 -include $(XEN_ROOT)/.config
 
-XEN_COMPILE_ARCH    ?= $(shell uname -m | sed -e s/i.86/x86_32/ \
+XEN_COMPILE_ARCH    ?= $(eval XEN_COMPILE_ARCH := $(shell uname -m | sed -e s/i.86/x86_32/ \
                          -e s/i86pc/x86_32/ -e s/amd64/x86_64/ \
                          -e s/armv7.*/arm32/ -e s/armv8.*/arm64/ \
-                         -e s/aarch64/arm64/)
+                         -e s/aarch64/arm64/))$(XEN_COMPILE_ARCH)
 
 XEN_TARGET_ARCH     ?= $(XEN_COMPILE_ARCH)
-XEN_OS              ?= $(shell uname -s)
+XEN_OS              ?= $(eval XEN_OS := $(shell uname -s))$(XEN_OS)
 
 CONFIG_$(XEN_OS) := y
 
-- 
Anthony PERARD

Re: [XEN PATCH v2 0/5] build: reduce number of $(shell) execution on make 4.4

[Jason Andryuk]

On Thu, Jun 22, 2023 at 11:31 AM Anthony PERARD
<anthony.perard@citrix.com> wrote:
>
> Patch series available in this git branch:
> https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.build-exported-shell-command-value-v2
>
> v2:
> - new patches removing TARGET_SUBARCH and TARGET_ARCH.
> - style change in first patch

For the v2 series:
Tested-by: Jason Andryuk <jandryuk@gmail.com>

Thanks,
Jason

Re: [XEN PATCH v2 1/5] build: define ARCH and SRCARCH later

[Jan Beulich]

On 22.06.2023 17:30, Anthony PERARD wrote:
> Defining ARCH and SRCARCH later in xen/Makefile allows to switch to
> immediate evaluation variable type.
> 
> ARCH and SRCARCH depends on value defined in Config.mk and aren't used
> TARGET_SUBARCH or TARGET_ARCH, and not before it's needed in a
> sub-make or a rule.

Something looks to be missing in this sentence. I first thought it
was merely a "by", but I don't see how TARGET_{,SUB}ARCH matter
here.

> This will help reduce the number of times the shell rune is been
> run.
> 
> With GNU make 4.4, the number of execution of the command present in
> these $(shell ) increased greatly. This is probably because as of make
> 4.4, exported variable are also added to the environment of $(shell )
> construct.
> 
> Also, `make -d` shows a lot of these:
>     Makefile:39: not recursively expanding SRCARCH to export to shell function
>     Makefile:38: not recursively expanding ARCH to export to shell function
> 
> Reported-by: Jason Andryuk <jandryuk@gmail.com>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> Tested-by: Jason Andryuk <jandryuk@gmail.com>

With some suitable adjustment
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [XEN PATCH v2 2/5] build: remove TARGET_SUBARCH, a duplicate of ARCH

[Jan Beulich]

On 22.06.2023 17:30, Anthony PERARD wrote:
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [XEN PATCH v2 3/5] build: remove TARGET_ARCH, a duplicates of SRCARCH

[Jan Beulich]

On 22.06.2023 17:30, Anthony PERARD wrote:
> The same command is used to generate the value of both $(TARGET_ARCH)
> and $(SRCARCH), as $(ARCH) is an alias for $(XEN_TARGET_ARCH).
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

[Jan Beulich]

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?

Jan

Re: [XEN PATCH v2 5/5] Config.mk: evaluate XEN_COMPILE_ARCH and XEN_OS on first use

[Jan Beulich]

On 22.06.2023 17:30, Anthony PERARD wrote:
> --- a/Config.mk
> +++ b/Config.mk
> @@ -19,13 +19,13 @@ or       = $(if $(strip $(1)),$(1),$(if $(strip $(2)),$(2),$(if $(strip $(3)),$(
>  
>  -include $(XEN_ROOT)/.config
>  
> -XEN_COMPILE_ARCH    ?= $(shell uname -m | sed -e s/i.86/x86_32/ \
> +XEN_COMPILE_ARCH    ?= $(eval XEN_COMPILE_ARCH := $(shell uname -m | sed -e s/i.86/x86_32/ \
>                           -e s/i86pc/x86_32/ -e s/amd64/x86_64/ \
>                           -e s/armv7.*/arm32/ -e s/armv8.*/arm64/ \
> -                         -e s/aarch64/arm64/)
> +                         -e s/aarch64/arm64/))$(XEN_COMPILE_ARCH)

I'd like to suggest to wrap this differently, e.g.

XEN_COMPILE_ARCH    ?= $(eval XEN_COMPILE_ARCH := \
                         $(shell uname -m | \
                                 sed -e s/i.86/x86_32/ \
                                     -e s/i86pc/x86_32/ -e s/amd64/x86_64/ \
                                     -e s/armv7.*/arm32/ -e s/armv8.*/arm64/ \
                                     -e s/aarch64/arm64/) \
                         )$(XEN_COMPILE_ARCH)

>  XEN_TARGET_ARCH     ?= $(XEN_COMPILE_ARCH)
> -XEN_OS              ?= $(shell uname -s)
> +XEN_OS              ?= $(eval XEN_OS := $(shell uname -s))$(XEN_OS)

With these further uses of this same construct as in patch 4, is there
any chance of abstracting (part of, e.g. at least the rhs of the ?=)
the construct into a macro?

Jan

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

[Anthony PERARD]

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

Thanks,

-- 
Anthony PERARD

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

[Jan Beulich]

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