Re: [Linux-kernel-mentees] [PATCH] kconfig: Prepend interpeters when calling scripts

[Ujjwal Kumar <ujjwalkumar0501@gmail.com>]

On 26/09/20 11:42 am, Lukas Bulwahn wrote:
> 
> Basically the diff is okay; already start writing the commit message.
> 
> On Sat, 26 Sep 2020, Ujjwal Kumar wrote:
> 
>> Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
>> ---
>>  init/Kconfig | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 91456ac0ef20..9f6ec575e3d1 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -30,12 +30,12 @@ config CC_IS_GCC
>>  
>>  config GCC_VERSION
>>  	int
>> -	default $(shell,$(srctree)/scripts/gcc-version.sh $(CC)) if CC_IS_GCC
>> +	default $(shell,sh $(srctree)/scripts/gcc-version.sh $(CC)) if CC_IS_GCC
> 
> in the top-level Makefile, (./Makefile), you will find a section:
> 
> # SHELL used by kbuild
> CONFIG_SHELL := sh
> 
> I think that is what is supposed to be used here.
> 
>>  	default 0
>>  
>>  config LD_VERSION
>>  	int
>> -	default $(shell,$(LD) --version | $(srctree)/scripts/ld-version.sh)
>> +	default $(shell,$(LD) --version | awk -f $(srctree)/scripts/ld-version.sh)
>>
> 
> Further you find:
> 
> # Make variables (CC, etc...)
> 
> This section defines variables for each interpreter, in case someone wants
> to modify/override the default interpreters.
> 
> You can find there:
> 
> AWK             = awk
> 
> Use that.
> 
>>  config CC_IS_CLANG
>>  	def_bool $(success,echo "$(CC_VERSION_TEXT)" | grep -q clang)
>> @@ -45,20 +45,20 @@ config LD_IS_LLD
>>  
>>  config CLANG_VERSION
>>  	int
>> -	default $(shell,$(srctree)/scripts/clang-version.sh $(CC))
>> +	default $(shell,sh $(srctree)/scripts/clang-version.sh $(CC))
>>
> 
> I think CONFIG_SHELL is right here.
>   
>>  config CC_CAN_LINK
>>  	bool
>> -	default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m64-flag)) if 64BIT
>> -	default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m32-flag))
>> +	default $(success,sh $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m64-flag)) if 64BIT
>> +	default $(success,sh $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m32-flag))
>>
> 
> Same here.
>   
>>  config CC_CAN_LINK_STATIC
>>  	bool
>> -	default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m64-flag) -static) if 64BIT
>> -	default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m32-flag) -static)
>> +	default $(success,sh $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m64-flag) -static) if 64BIT
>> +	default $(success,sh $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m32-flag) -static)
>>
> 
> Same here.
>   
>>  config CC_HAS_ASM_GOTO
>> -	def_bool $(success,$(srctree)/scripts/gcc-goto.sh $(CC))
>> +	def_bool $(success,sh $(srctree)/scripts/gcc-goto.sh $(CC))
>>
> 
> Same here.
> 
> I think this is already a good size for the patch, it solves one problem 
> to have the basic Configuration targets in kbuild work without the need 
> for any executable flag on repository files.
> 
> Fix it up, write an appropriate commit message, we will review here on the 

How is the following for a commit message?
kconfig: use interpreters and not rely on x-bit to invoke scripts

Having the execute bits set wasn't something that we could depend on.
Any portion of the kernel that invokes such files provides the interpreter 
explicitly.

Additionally, relying on x-bit is not recommended for following reasons:

  - An official guidance is currently unavailable to decide whether 
    it is mandatory to have the x-bit set.

  - It is possible that the x-bit will be erased/ignored when handling 
    patches (for example, using patch(1)).

I am wondering who should I include for 'Suggested-by:' tag?

> linux-kernel-mentees list and then it can go out into the wild wild west 
> of the kernel mailing list.
> 

Yes I will make the changes.

Also, some Makefile(s) do not use '$(CONFIG_SHELL)' and instead use 'sh'.
For instance,
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/scripts/Makefile.lib?h=next-20200925#n422
AND
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/s390/boot/Makefile?h=next-20200925#n74

I wonder if they should be improved to maintain the consistency or left 
at their current state.

> 
> Lukas
> 
>>  config CC_HAS_ASM_GOTO_OUTPUT
>>  	depends on CC_HAS_ASM_GOTO
>> -- 
>> 2.26.2
>>
>>

On looking at Makefile(s), I found some places where we miss specifying the 
interpreter and so we are relying on executable bit again.
here:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/nds32/kernel/vdso/Makefile?h=next-20200925#n42

I wonder if it should go as a separate patch, simply because its Makefile now.

Thanks
Ujjwal Kumar
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees