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

[Lukas Bulwahn <lukas.bulwahn@gmail.com>]

On Sat, 26 Sep 2020, Ujjwal Kumar wrote:

> 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
>

kconfig: use interpreters to invoke scripts

> Having the execute bits set wasn't something that we could depend on.

We cannot rely on execute bits to be set on files in the repository.

> Any portion of the kernel that invokes such files provides the interpreter 
> explicitly.
>
The build script should use the explicit interpreter when invoking any 
script from the repository.
 
> 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.
>

Well, I think it is better to prepare an "official guidance" as commit in 
the kernel documentation and then simply refer to that. That will be a 
separate patch in this series.

>   - 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?
>

I think there is nothing wrong with having everyone that suggested the 
work mentioned, so Andrew, Kees and me.

Also add a Link: to the discussion on lore.kernel.org as reference.

> > 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.
>

Again, another good catch. Make that yet another patch.
 
> > 
> > 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.

Yes, another good catch and patch.

Collect all those patches and then send them out as patch series.

So something like:

- Documentation patch: describe in the kbuild documentation not to rely on 
the x-bit.
- patch on kconfig (this one)
- patch on Makefile.lib
- patch on s390
- ... and more if we found some more issues

The kernel community can handle many small patches, don't worry. Also, 
there is no need to rush with some first patch now, we can collect all 
patches and then send them out as one series. These are not showstopper 
problems, but some real nice small cleanups nobody noticed so far.

If I find some time, I can write a quick patch for the Documentation and 
we can review that together.

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