Re: [Xen-devel] [XEN PATCH v3 2/6] xen: Have Kconfig check $(CC)'s version

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.01.2020 13:29, Anthony PERARD wrote:
> On Thu, Jan 16, 2020 at 12:30:49PM +0100, Jan Beulich wrote:
>> On 15.01.2020 18:00, Anthony PERARD wrote:
>>> --- a/xen/Kconfig
>>> +++ b/xen/Kconfig
>>> @@ -4,9 +4,25 @@
>>>  #
>>>  mainmenu "Xen/$(SRCARCH) $(XEN_FULLVERSION) Configuration"
>>>  
>>> +source "scripts/Kconfig.include"
>>> +
>>>  config BROKEN
>>>     bool
>>>  
>>> +config CC_IS_GCC
>>> +   def_bool $(success,$(CC) --version | head -n 1 | grep -q gcc)
>>> +
>>> +config GCC_VERSION
>>> +   int
>>> +   default $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC))
>>> +
>>> +config CC_IS_CLANG
>>> +   def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
>>> +
>>> +config CLANG_VERSION
>>> +   int
>>> +   default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))
>>
>> I continue to be unhappy about the redundancy, but I will accept
>> it if others indeed think this is helpful. However, I don't see
>> then why the setting of CC_IS_* need another shell invocation
>> each - this could just be *_VERSION > 0 then, seeing that the
>> scripts already to a respective grep of the --version output.
> 
> From functionality point of view, replacing the macro by
> "def_bool %_VERSION > 0" in "config CC_IS_%" would be fine, even so it
> would be weird to read. I think that would need a comment saying:
>   # %-version.sh is expected to return "0" when $(CC) isn't %
> 
> That could be done on commit.

Sure.

>> Even better would imo be, as suggested before, a "depends on
>> CC_IS_*" on each *_VERSION.
> 
> Haven't we discussed this before?

Indeed, hence also my "as suggested before". I remain unconvinced
that is it useful to have e.g.

CONFIG_GCC_VERSION=80300
CONFIG_CLANG_VERSION=0

in xen/.config. This is at best confusing, because it may not
represent what the system actually has installed (which I realize
is also not the intention, but the variable naming suggests that
this is what was found on the system; I have no better naming
suggestion, to preempt a possible question to this effect).

>> As a nit - common style elsewhere would suggest that there ought
>> to be a blank after the commas in $(macro, ...) invocations.
>> This would then extend to Kconfig.include as well, unless that's
>> a largely verbatim inherited file.
> 
> That's not a good idea, it may not matter in this case but adding a
> space after commas in some other cases will not do what one wants. make
> and Kconfig keeps the spaces when expanding the macro.

Where blanks matter they should of course be omitted. When they
don't matter, personally I think common style guidelines should be
honored. But this may be just me ...

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel