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

[Jan Beulich <jbeulich@xxxxxxxx>]

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