tools/libs: Don't recursively expand MAJOR ?= $(shell ...)

[PATCH] tools/libs: Don't recursively expand MAJOR ?= $(shell ...)

Posted by Andrew Cooper 2 years, 4 months ago

?= is a deferred assignment.  Switch to an alternative form which lets us use
an immediate assignment.

Before, version.sh gets run anywhere between 46 and 88 times, with 50 on a
`clean`.  After, 6 times, invariant of main rune, and whether it is an
incremental build or not.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Juergen Gross <jgross@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>

The identity transform comes from the docs
https://www.gnu.org/software/make/manual/make.html#Flavors (final paragraph).

Something slightly weird is going on.  Before this, the exact number of hits
that verson.sh gets isn't stable, even when running repeat incremental builds.
I suspect this means we've got a lurking parallel build issue.
---
 tools/libs/libs.mk | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk
index dfbbef4080f6..b21e0bf083a0 100644
--- a/tools/libs/libs.mk
+++ b/tools/libs/libs.mk
@@ -6,7 +6,10 @@
 #   MINOR:   minor version of lib (0 if empty)
 
 LIBNAME := $(notdir $(CURDIR))
-MAJOR ?= $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile)
+
+ifeq ($(origin MAJOR), undefined)
+MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile)
+endif
 MINOR ?= 0
 
 SHLIB_LDFLAGS += -Wl,--version-script=libxen$(LIBNAME).map
-- 
2.11.0

Re: [PATCH] tools/libs: Don't recursively expand MAJOR ?= $(shell ...)

Posted by Jan Beulich 2 years, 4 months ago

On 13.12.2021 20:04, Andrew Cooper wrote:
> --- a/tools/libs/libs.mk
> +++ b/tools/libs/libs.mk
> @@ -6,7 +6,10 @@
>  #   MINOR:   minor version of lib (0 if empty)
>  
>  LIBNAME := $(notdir $(CURDIR))
> -MAJOR ?= $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile)
> +
> +ifeq ($(origin MAJOR), undefined)
> +MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile)
> +endif
>  MINOR ?= 0
>  
>  SHLIB_LDFLAGS += -Wl,--version-script=libxen$(LIBNAME).map

Wouldn't it be better to move the "endif" past the setting of MINOR
(which then could use := as well)? Libraries with their own versioning
would imo better specify both rather than relying on getting 0 from
here (which at present none of them does). Would require an
adjustment to the comment at the top of libs.mk, though.

And further, since you're switching to $(origin ...), wouldn't this
be an opportunity to avoid stray inheriting of values from the
environment, by switching to "ifneq ($(origin MAJOR), file)"? Or is
there an intention of allowing such control via the environment
(which would then override the versions for all libraries not
explicitly setting them)? In turn I would then further wonder
whether command line overrides are intended, but I guess people
doing so ought to indeed get what they have asked for (all libraries
versioned identically, assuming both MAJOR and MINOR get defined
that way).

Jan

Re: [PATCH] tools/libs: Don't recursively expand MAJOR ?= $(shell ...)

Posted by Andrew Cooper 2 years, 4 months ago

On 14/12/2021 08:17, Jan Beulich wrote:
> On 13.12.2021 20:04, Andrew Cooper wrote:
>> --- a/tools/libs/libs.mk
>> +++ b/tools/libs/libs.mk
>> @@ -6,7 +6,10 @@
>>  #   MINOR:   minor version of lib (0 if empty)
>>  
>>  LIBNAME := $(notdir $(CURDIR))
>> -MAJOR ?= $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile)
>> +
>> +ifeq ($(origin MAJOR), undefined)
>> +MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile)
>> +endif
>>  MINOR ?= 0
>>  
>>  SHLIB_LDFLAGS += -Wl,--version-script=libxen$(LIBNAME).map
> Wouldn't it be better to move the "endif" past the setting of MINOR
> (which then could use := as well)? Libraries with their own versioning
> would imo better specify both rather than relying on getting 0 from
> here (which at present none of them does). Would require an
> adjustment to the comment at the top of libs.mk, though.

I considered that, but decided against it.

Absolutely nothing good can come of having a mix/match of whether MAJOR
and MINOR are set, and the whole point of this logic is to provide a
safe default when things are unspecified.

>
> And further, since you're switching to $(origin ...), wouldn't this
> be an opportunity to avoid stray inheriting of values from the
> environment, by switching to "ifneq ($(origin MAJOR), file)"?

No.  Not because I think setting MAJOR on the command line is sensible,
but because it fails the principle of lease surprise.

Basically all variables are editable on the command line and the
environment.  Prohibiting this one alone is bizarre, unnecessary, and
fragile in the case where if it is encountered, it's probably someone
who knows exactly what they're doing, trying to debug the build system.

~Andrew

Re: [PATCH] tools/libs: Don't recursively expand MAJOR ?= $(shell ...)

Posted by Jan Beulich 2 years, 4 months ago

On 14.12.2021 13:52, Andrew Cooper wrote:
> On 14/12/2021 08:17, Jan Beulich wrote:
>> On 13.12.2021 20:04, Andrew Cooper wrote:
>>> --- a/tools/libs/libs.mk
>>> +++ b/tools/libs/libs.mk
>>> @@ -6,7 +6,10 @@
>>>  #   MINOR:   minor version of lib (0 if empty)
>>>  
>>>  LIBNAME := $(notdir $(CURDIR))
>>> -MAJOR ?= $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile)
>>> +
>>> +ifeq ($(origin MAJOR), undefined)
>>> +MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile)
>>> +endif
>>>  MINOR ?= 0
>>>  
>>>  SHLIB_LDFLAGS += -Wl,--version-script=libxen$(LIBNAME).map
>> Wouldn't it be better to move the "endif" past the setting of MINOR
>> (which then could use := as well)? Libraries with their own versioning
>> would imo better specify both rather than relying on getting 0 from
>> here (which at present none of them does). Would require an
>> adjustment to the comment at the top of libs.mk, though.
> 
> I considered that, but decided against it.
> 
> Absolutely nothing good can come of having a mix/match of whether MAJOR
> and MINOR are set, and the whole point of this logic is to provide a
> safe default when things are unspecified.
> 
>>
>> And further, since you're switching to $(origin ...), wouldn't this
>> be an opportunity to avoid stray inheriting of values from the
>> environment, by switching to "ifneq ($(origin MAJOR), file)"?
> 
> No.  Not because I think setting MAJOR on the command line is sensible,
> but because it fails the principle of lease surprise.
> 
> Basically all variables are editable on the command line and the
> environment.  Prohibiting this one alone is bizarre, unnecessary, and
> fragile in the case where if it is encountered, it's probably someone
> who knows exactly what they're doing, trying to debug the build system.

And then there's that someone else who ends up having MAJOR or MINOR
set in the environment from whatever was done previously in a shell.
The two variables are simply of too generic name to sensibly be
communicated via the environment (and I specifically separate the
variant where they're specified on the make command line).

Anyway - none of what I've said is an objection. I was merely hoping
we could get the whole thing a little less fragile at this occasion.

Jan

Re: [PATCH] tools/libs: Don't recursively expand MAJOR ?= $(shell ...)

Posted by Anthony PERARD 2 years, 4 months ago

On Mon, Dec 13, 2021 at 07:04:49PM +0000, Andrew Cooper wrote:
> ?= is a deferred assignment.  Switch to an alternative form which lets us use
> an immediate assignment.
> 
> Before, version.sh gets run anywhere between 46 and 88 times, with 50 on a
> `clean`.  After, 6 times, invariant of main rune, and whether it is an

Instead of just 6, you probably mean between 6 and 12 times,
"make clean; make; make", the last make would run version.sh 12 times.

> incremental build or not.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Thanks,

-- 
Anthony PERARD