| Message ID | 20230622153005.31604-5-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | build: reduce number of $(shell) execution on make 4.4 | expand |
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
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,
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
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.
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(-)