linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
From: Lukas Bulwahn <lukas.bulwahn@gmail.com>
To: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
Cc: linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] [PATCH RFC] kbuild: use interpreters to invoke scripts
Date: Thu, 1 Oct 2020 16:09:54 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.21.2010011554160.24571@felia> (raw)
In-Reply-To: <80f188d8-b0c2-ecf1-d0b9-faad146d2cec@gmail.com>



On Thu, 1 Oct 2020, Ujjwal Kumar wrote:

> On 01/10/20 4:38 pm, Lukas Bulwahn wrote:
> > 
> > 
> > On Thu, 1 Oct 2020, Ujjwal Kumar wrote:
> > 
> >> We cannot rely on execute bits to be set on files in the repository.
> >> The build script should use the explicit interpreter when invoking any
> >> script from the repository.
> >>
> >> Link: https://lore.kernel.org/lkml/20200830174409.c24c3f67addcce0cea9a9d4c@linux-foundation.org/
> >> Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/
> >>
> >> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> >> Suggested-by: Kees Cook <keescook@chromium.org>
> >> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> >> Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
> >> ---
> >>  Documentation/Makefile                           | 16 ++++++++--------
> >>  Documentation/userspace-api/media/Makefile       |  6 +++---
> >>  Makefile                                         |  4 ++--
> >>  arch/arm64/kernel/vdso/Makefile                  |  2 +-
> >>  arch/arm64/kernel/vdso32/Makefile                |  2 +-
> >>  arch/ia64/Makefile                               |  4 ++--
> >>  arch/nds32/kernel/vdso/Makefile                  |  2 +-
> >>  .../comedi/drivers/ni_routing/tools/Makefile     |  6 +++---
> >>  scripts/Makefile.build                           |  2 +-
> >>  scripts/Makefile.package                         |  4 ++--
> >>  tools/bootconfig/Makefile                        |  2 +-
> >>  tools/bpf/Makefile.helpers                       |  2 +-
> >>  tools/lib/bpf/Makefile                           |  2 +-
> >>  tools/perf/Makefile.perf                         |  2 +-
> >>  tools/power/cpupower/Makefile                    |  2 +-
> >>  tools/testing/selftests/rcutorture/Makefile      |  2 +-
> >>  tools/testing/selftests/rseq/Makefile            |  2 +-
> >>  tools/testing/selftests/vm/Makefile              |  6 +++---
> >>  tools/testing/selftests/wireguard/qemu/Makefile  |  4 ++--
> >>  tools/testing/selftests/x86/Makefile             |  6 +++---
> >>  20 files changed, 39 insertions(+), 39 deletions(-)
> >>
> > 
> > You will probably need to split this patch into multiple patches, but the 
> > discussion will show how to split it best.
> > 
> > Probably:
> > 
> >  - one for Documentation
> >  - one for general kbuild, maybe including arch
> >  - one for selftests
> >  - one for the other tools
> >  - one for the comedi driver
> > 
> > So, what did you do for testing your change?
> 
> First, I had to unset the execute bits on all files.
> for i in $(find -executable -type f); do chmod -x $i; done
> 
> To test the changes, I invoked the make rule under which my current 
> changes lie. And I could see a permission-denied whenever a script 
> was invoked. After the patch, the make rule progressed without 
> any permissions error.
> I couldn't test each and every change, because some rules failed 
> before invoking the script and others could not be invoked by me.
> 

Which could you not test? Please share that.

> I successfully tested the changes under Documentation/ (and some others).
> 
> $ make htmldocs
> make[1]: execvp: ./scripts/sphinx-pre-install: Permission denied
> make[1]: *** [Documentation/Makefile:81: htmldocs] Error 127
> make: *** [Makefile:1661: htmldocs] Error 2
> 
> Files that I tested my changes on:
> Documentation/Makefile                           | 16 ++++++++--------
> Documentation/userspace-api/media/Makefile       |  6 +++---
> Makefile                                         |  4 ++--
> tools/perf/Makefile.perf                         |  2 +-
> tools/power/cpupower/Makefile                    |  2 +-
> 
> 
> > 
> > Did you check if CONFIG_SHELL is actually available in tools?
> 
> Yes, I did check.
> To verify, I echo(ed) the variables under a make task. And invoked 
> the task from srctree.
>

That sounds good.
 
> >> --- a/arch/nds32/kernel/vdso/Makefile
> >> +++ b/arch/nds32/kernel/vdso/Makefile
> >> @@ -39,7 +39,7 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
> >>  # Generate VDSO offsets using helper script
> >>  gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
> >>  quiet_cmd_vdsosym = VDSOSYM $@
> >> -      cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
> >> +      cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
> >>
> > 
> > I guess it is better to modify gen-vdsosym.
> 
> Do you mean something as follows:
> gen-vdsosym := $(CONFIG_SHELL) $(srctree)/$(src)/gen_vdso_offsets.sh
> 
> If so, I have never seen that style in the build files. Infact, the current
> style is followed at many places.
> For instance,
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/scripts/Makefile.lib?h=next-20201001#n398
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/tools/Makefile?h=next-20201001#n50
>

I searched around as well. I think your proposal is fine.

How about splitting out a first patch on the generic kbuild files and send 
that out as RFC, first send the RFC patch to linux-kernel-mentees list, 
then if that looks good, we go for Masahiro and the linux-kbuild list?

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

  reply	other threads:[~2020-10-01 14:10 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-01 10:37 [Linux-kernel-mentees] [PATCH RFC] kbuild: use interpreters to invoke scripts Ujjwal Kumar
2020-10-01 11:08 ` Lukas Bulwahn
2020-10-01 13:47   ` Ujjwal Kumar
2020-10-01 14:09     ` Lukas Bulwahn [this message]
2020-10-01 19:21       ` Ujjwal Kumar
2020-10-01 19:31 Ujjwal Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.21.2010011554160.24571@felia \
    --to=lukas.bulwahn@gmail.com \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=ujjwalkumar0501@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).