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

[Andrew Cooper <amc96@xxxxxxxx>]

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