[PATCH RFC 0/2] use interpreters to invoke scripts

[Ujjwal Kumar]

This patch series aims at removing the dependency on execute
bit of the scripts in the kbuild system.

If not working with fresh clone of linux-next, clean the srctree:
make distclean
make tools/clean

To test the dependency on execute bits, I tried building the
kernel after removing x-bits for all files in the repository.
Removing execute bits:
for i in $(find -executable -type f); do chmod -x $i; done

Any attempts to configure (or build) the kernel fail because of
'Permission denied' on scripts with the following error:
$ make allmodconfig
sh: ./scripts/gcc-version.sh: Permission denied
init/Kconfig:34: syntax error
init/Kconfig:33: invalid statement
init/Kconfig:34: invalid statement
sh: ./scripts/ld-version.sh: Permission denied
init/Kconfig:39: syntax error
init/Kconfig:38: invalid statement
sh: ./scripts/clang-version.sh: Permission denied
init/Kconfig:49: syntax error
init/Kconfig:48: invalid statement
make[1]: *** [scripts/kconfig/Makefile:71: allmodconfig] Error 1
make: *** [Makefile:606: allmodconfig] Error 2

Changes:
1. Adds specific interpreters (in Kconfig) to invoke
scripts.

After this patch I could successfully do a kernel build
without any errors.

2. Again, adds specific interpreters to other parts of
kbuild system.

I could successfully perform the following make targets after
applying the PATCH 2/2:
make headerdep
make kselftest-merge
make rpm-pkg
make perf-tar-src-pkg
make ARCH=ia64 defconfig
ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make prepare

Following changes in PATCH 2/2 are not yet tested:
arch/arm64/kernel/vdso32/Makefile
arch/nds32/kernel/vdso/Makefile
scripts/Makefile.build

Ujjwal Kumar (2):
kconfig: use interpreters to invoke scripts
kbuild: use interpreters to invoke scripts

Makefile | 4 ++--
arch/arm64/kernel/vdso/Makefile | 2 +-
arch/arm64/kernel/vdso32/Makefile | 2 +-
arch/ia64/Makefile | 4 ++--
arch/nds32/kernel/vdso/Makefile | 2 +-
init/Kconfig | 16 ++++++++--------
scripts/Makefile.build | 2 +-
scripts/Makefile.package | 4 ++--
8 files changed, 18 insertions(+), 18 deletions(-)

--
2.26.2

[Lukas Bulwahn]

On Sat, 3 Oct 2020, Ujjwal Kumar wrote:

> This patch series aims at removing the dependency on execute
> bit of the scripts in the kbuild system.
>

Ujjwal, your setup to send out the patch series created three unrelated
emails rather than the default way, i.e., to have one cover letter
and the patches in reply to the cover letter.

You can see the difference here:

https://lore.kernel.org/linux-kbuild/

The presentation of your patch series looks different than the other
patch series on the list. Use the linux-kernel-mentees list for testing
your setup.

For this version of the patch series, I guess it is okay; but this set up
right for the next patch series.

> If not working with fresh clone of linux-next, clean the srctree:
> make distclean
> make tools/clean
>

I hit an unrelated issue on next-20201002 that make tools/clean fails.
Other than that, this is all good.

> To test the dependency on execute bits, I tried building the
> kernel after removing x-bits for all files in the repository.
> Removing execute bits:
> for i in $(find -executable -type f); do chmod -x $i; done
>

Okay, I did that.

> Any attempts to configure (or build) the kernel fail because of
> 'Permission denied' on scripts with the following error:
> $ make allmodconfig
> sh: ./scripts/gcc-version.sh: Permission denied
> init/Kconfig:34: syntax error
> init/Kconfig:33: invalid statement
> init/Kconfig:34: invalid statement
> sh: ./scripts/ld-version.sh: Permission denied
> init/Kconfig:39: syntax error
> init/Kconfig:38: invalid statement
> sh: ./scripts/clang-version.sh: Permission denied
> init/Kconfig:49: syntax error
> init/Kconfig:48: invalid statement
> make[1]: *** [scripts/kconfig/Makefile:71: allmodconfig] Error 1
> make: *** [Makefile:606: allmodconfig] Error 2
>

I can confirm that these errors are reported on next-20201002.

> Changes:
> 1. Adds specific interpreters (in Kconfig) to invoke
> scripts.
>
> After this patch I could successfully do a kernel build
> without any errors.
>

With this first patch, I could then successfully do:

make allmodconfig && make

So far, so good. I did check the first patch.

Lukas

[Lukas Bulwahn]

On Sat, 3 Oct 2020, Ujjwal Kumar wrote:

Ujjwal, I suggest that you continue to wait if you get any feedback from
Masahiro-san within the next two weeks (although the merge window) and if
not, try to rebase to the the next rc1 and resend this patchset with
Nathan's feedback tags added.

The merge window is busy time for maintainers; in the meantime, you might
look into if the build target 'make tools/clean' works on the current
release and if there are fixes getting merged that fix that.

Lukas

[Masahiro Yamada]

Andrew Morton suggested and applied the doc patch
(commit e9aae7af4601688386 in linux-next),
but did not pick up this series.

It is difficult to predict which patch he would
pick up, and which he would not.


I can apply this series
together with Lukas' base patch.


I pointed out possible mistakes in 2/2.
I can locally fix them up if you agree.


BTW, Kees Cook suggested dropping the x bit
from all scripts, but I did not agree with that part.


In the doc change, Lukas mentioned
"further clean-up patches", but I hope
it does not mean dropping the x bits.


--
Best Regards

Masahiro Yamada

[Ujjwal Kumar]

I agree with the changes you pointed out. I was in the process
of sending a V2 patch series (almost done). But if you prefer
on locally fixing them, that is completely fine.

>
>
> BTW, Kees Cook suggested dropping the x bit
> from all scripts, but I did not agree with that part.

IIRC, in the discussion Kees Cook suggestion was not to drop
x bit but rather he meant to use that as a trick to catch
any existing dependency on x bit.

>
>
> In the doc change, Lukas mentioned
> "further clean-up patches", but I hope
> it does not mean dropping the x bits.

IMO, he did not mean to drop the x bits.
But rather I have many more small changes similar to these.
He must be referring to these two patches and any future
patches around this issue.

>
>
> --
> Best Regards
>
> Masahiro Yamada
>

Thanks
Ujjwal Kumar

[Ujjwal Kumar]

- Adds specific interpreters (in Kconfig) to invoke

scripts.

After this patch I could successfully do a kernel build
without any errors.

- Again, adds specific interpreters to other parts of

kbuild system.

I could successfully perform the following make targets after
applying the PATCH 2/2:
make headerdep
make kselftest-merge
make rpm-pkg
make perf-tar-src-pkg
make ARCH=ia64 defconfig
ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make prepare

Following changes in PATCH 2/2 are not yet tested:
arch/arm64/kernel/vdso32/Makefile
arch/nds32/kernel/vdso/Makefile
scripts/Makefile.build

---
Changes in v2:

- Changes suggested by Masahiro Yamada
$($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)

Ujjwal Kumar (2):
kconfig: use interpreters to invoke scripts
kbuild: use interpreters to invoke scripts

Makefile | 4 ++--
arch/arm64/kernel/vdso/Makefile | 2 +-
arch/arm64/kernel/vdso32/Makefile | 2 +-
arch/ia64/Makefile | 4 ++--
arch/nds32/kernel/vdso/Makefile | 2 +-
init/Kconfig | 16 ++++++++--------
scripts/Makefile.build | 2 +-
scripts/Makefile.package | 4 ++--
8 files changed, 18 insertions(+), 18 deletions(-)

base-commit: 2cab4ac556258c14f6ec8d2157654e95556bbb31
--
2.25.1

[Ujjwal Kumar]

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.c24c...@linux-foundation.org/
Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/

Suggested-by: Andrew Morton <ak...@linux-foundation.org>
Suggested-by: Kees Cook <kees...@chromium.org>
Suggested-by: Lukas Bulwahn <lukas....@gmail.com>
Signed-off-by: Ujjwal Kumar <ujjwalk...@gmail.com>
---
init/Kconfig | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index c9446911cf41..8adf3194d26f 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,$(CONFIG_SHELL) $(srctree)/scripts/gcc-version.sh $(CC)) if CC_IS_GCC
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)

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,$(CONFIG_SHELL) $(srctree)/scripts/clang-version.sh $(CC))

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,$(CONFIG_SHELL) $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m64-flag)) if 64BIT
+ default $(success,$(CONFIG_SHELL) $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m32-flag))

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,$(CONFIG_SHELL) $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m64-flag) -static) if 64BIT
+ default $(success,$(CONFIG_SHELL) $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m32-flag) -static)

config CC_HAS_ASM_GOTO
- def_bool $(success,$(srctree)/scripts/gcc-goto.sh $(CC))
+ def_bool $(success,$(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC))

config CC_HAS_ASM_GOTO_OUTPUT
depends on CC_HAS_ASM_GOTO
--
2.25.1

[Ujjwal Kumar]

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.c24c...@linux-foundation.org/
Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/

Suggested-by: Andrew Morton <ak...@linux-foundation.org>
Suggested-by: Kees Cook <kees...@chromium.org>
Suggested-by: Lukas Bulwahn <lukas....@gmail.com>
Signed-off-by: Ujjwal Kumar <ujjwalk...@gmail.com>
---

Makefile | 4 ++--
arch/arm64/kernel/vdso/Makefile | 2 +-
arch/arm64/kernel/vdso32/Makefile | 2 +-
arch/ia64/Makefile | 4 ++--
arch/nds32/kernel/vdso/Makefile | 2 +-

scripts/Makefile.build | 2 +-
scripts/Makefile.package | 4 ++--

7 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Makefile b/Makefile
index 0af7945caa61..df20e71dd7c8 100644
--- a/Makefile
+++ b/Makefile
@@ -1256,7 +1256,7 @@ include/generated/utsrelease.h: include/config/kernel.release FORCE
PHONY += headerdep
headerdep:
$(Q)find $(srctree)/include/ -name '*.h' | xargs --max-args 1 \
- $(srctree)/scripts/headerdep.pl -I$(srctree)/include
+ $(PERL) $(srctree)/scripts/headerdep.pl -I$(srctree)/include

# ---------------------------------------------------------------------------
# Kernel headers
@@ -1312,7 +1312,7 @@ PHONY += kselftest-merge
kselftest-merge:
$(if $(wildcard $(objtree)/.config),, $(error No .config exists, config your kernel first!))
$(Q)find $(srctree)/tools/testing/selftests -name config | \
- xargs $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
+ xargs $(CONFIG_SHELL) $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
$(Q)$(MAKE) -f $(srctree)/Makefile olddefconfig

# ---------------------------------------------------------------------------
diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
index edccdb77c53e..fb07804b7fc1 100644
--- a/arch/arm64/kernel/vdso/Makefile
+++ b/arch/arm64/kernel/vdso/Makefile
@@ -65,7 +65,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 > $@

include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
$(call if_changed,vdsosym)
diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
index 7f96a1a9f68c..617c9ac58156 100644
--- a/arch/arm64/kernel/vdso32/Makefile
+++ b/arch/arm64/kernel/vdso32/Makefile
@@ -205,7 +205,7 @@ quiet_cmd_vdsomunge = MUNGE $@
gen-vdsosym := $(srctree)/$(src)/../vdso/gen_vdso_offsets.sh
quiet_cmd_vdsosym = VDSOSYM $@
# The AArch64 nm should be able to read an AArch32 binary
- cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
+ cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@

# Install commands for the unstripped file
quiet_cmd_vdso_install = INSTALL32 $@
diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
index 703b1c4f6d12..86d42a2d09cb 100644
--- a/arch/ia64/Makefile
+++ b/arch/ia64/Makefile
@@ -27,8 +27,8 @@ cflags-y := -pipe $(EXTRA) -ffixed-r13 -mfixed-range=f12-f15,f32-f127 \
-falign-functions=32 -frename-registers -fno-optimize-sibling-calls
KBUILD_CFLAGS_KERNEL := -mconstant-gp

-GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
-KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
+GAS_STATUS = $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
+KBUILD_CPPFLAGS += $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")

ifeq ($(GAS_STATUS),buggy)
$(error Sorry, you need a newer version of the assember, one that is built from \
diff --git a/arch/nds32/kernel/vdso/Makefile b/arch/nds32/kernel/vdso/Makefile
index 55df25ef0057..e77d4bcfa7c1 100644
--- 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 > $@

include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
$(call if_changed,vdsosym)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index a467b9323442..893217ee4a17 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -104,7 +104,7 @@ else ifeq ($(KBUILD_CHECKSRC),2)
endif

ifneq ($(KBUILD_EXTRA_WARN),)
- cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $<
+ cmd_checkdoc = $(PERL) $(srctree)/scripts/kernel-doc -none $<
endif

# Compile C sources (.c)
diff --git a/scripts/Makefile.package b/scripts/Makefile.package
index f952fb64789d..4fc16c4776cc 100644
--- a/scripts/Makefile.package
+++ b/scripts/Makefile.package
@@ -44,7 +44,7 @@ if test "$(objtree)" != "$(srctree)"; then \
echo >&2; \
false; \
fi ; \
-$(srctree)/scripts/setlocalversion --save-scmversion; \
+$(CONFIG_SHELL) $(srctree)/scripts/setlocalversion --save-scmversion; \
tar -I $(KGZIP) -c $(RCS_TAR_IGNORE) -f $(2).tar.gz \
--transform 's:^:$(2)/:S' $(TAR_CONTENT) $(3); \
rm -f $(objtree)/.scmversion
@@ -123,7 +123,7 @@ git --git-dir=$(srctree)/.git archive --prefix=$(perf-tar)/ \
mkdir -p $(perf-tar); \
git --git-dir=$(srctree)/.git rev-parse HEAD > $(perf-tar)/HEAD; \
(cd $(srctree)/tools/perf; \
-util/PERF-VERSION-GEN $(CURDIR)/$(perf-tar)/); \
+$(CONFIG_SHELL) util/PERF-VERSION-GEN $(CURDIR)/$(perf-tar)/); \
tar rf $(perf-tar).tar $(perf-tar)/HEAD $(perf-tar)/PERF-VERSION-FILE; \
rm -r $(perf-tar); \
$(if $(findstring tar-src,$@),, \
--
2.25.1

[Lukas Bulwahn]

Here is an instance of what Masahiro-san pointed out being wrong.

Ujjwal, will you send a v3?

[Ujjwal Kumar]

Following is the quoted text from the reply mail from Masahiro

>> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")

>> +GAS_STATUS = $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>> +KBUILD_CPPFLAGS += $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>
>
>
> These changes look wrong to me.
>
> $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)
>

From the above text, I understand as follows:

That my proposed change:
$(shell $(src...) -> $($(CONFIG_SHELL) $(src...)

is WRONG

and in the next line he suggested the required correction.
That being:
$($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)

Which is in v2 of the patch series.

Lukas, please correct me if I'm wrong so that I can work on v3
if required.

Also, Nathan reviewed both the patches in v1 of this series. So,
should I be the one who adds his tag in next iterations?


Thanks
Ujjwal Kumar

[Lukas Bulwahn]

Sorry, my memory tricked me; I got it confused. Your patch looks good.

> Also, Nathan reviewed both the patches in v1 of this series. So,
> should I be the one who adds his tag in next iterations?
>

Masahiro-san will probably just add them when he picks the patches.

Lukas

[Ujjwal Kumar]

On 13/10/20 12:24 am, Bernd Petrovitsch wrote:
> Hi all!

>
>>>> diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
>>>> index 703b1c4f6d12..86d42a2d09cb 100644
>>>> --- a/arch/ia64/Makefile
>>>> +++ b/arch/ia64/Makefile
>>>> @@ -27,8 +27,8 @@ cflags-y := -pipe $(EXTRA) -ffixed-r13 -mfixed-range=f12-f15,f32-f127 \
>>>> -falign-functions=32 -frename-registers -fno-optimize-sibling-calls
>>>> KBUILD_CFLAGS_KERNEL := -mconstant-gp
>>>>
>>>> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>>> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>>> +GAS_STATUS = $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>>> +KBUILD_CPPFLAGS += $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>>
>>> Here is an instance of what Masahiro-san pointed out being wrong.
>>>
>>> Ujjwal, will you send a v3?
>>
>> Following is the quoted text from the reply mail from Masahiro
>>
>>>> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>>> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>>> +GAS_STATUS = $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>>> +KBUILD_CPPFLAGS += $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>>
>>>
>>>
>>> These changes look wrong to me.
>>>
>>> $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)
>>>
>>
>> From the above text, I understand as follows:
>

> Did you actually *test* that (expecially) these lines work
> afterwards as good as before?

Yes, I did check my changes. TBH, I spent a considerable
amount of time in doing so (given that I'm new to the
community). And I explicitly mentioned the ones I couldn't
test in the cover letter.

But I'm afraid this particular change that Masahiro pointed
must have been overlooked by me (and possibly by others
involved in the process). Being the author of the patch I
accept my mistake.

Because this construct was new to me I read about it
thoroughly in the docs.
As soon as it was pointed out to me, I at once realised
that the change proposed by me was wrong (i didn't
have to look at the docs).

>
>> That my proposed change:
>> $(shell $(src...) -> $($(CONFIG_SHELL) $(src...)
>>
>> is WRONG
>

> Yup, as it's in a Makefile and that's a Makefile construct>

>> and in the next line he suggested the required correction.
>> That being:
>> $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)
>

> Such stuff should generally not be needed as the to-be-used
> shell can be set in Makefiles via a "SHELL = " assignment

It's not about setting shell but rather using it at required
place. The 'shell function' is meant to execute provided
commands in an environment outside of make; and executing
commands in that environment is somewhat similar to running
commands on a terminal.
Invoking a script file without setting the x bits will give
a permission denied error.
Similar thing happens when 'shell function' tries to invoke
the provided script. So the task was simply to prepend the
$CONFIG_SHELL (or $SHELL whichever is configured; simple sh
would also suffice) with the script file in 'shell function'.

> (defaulting to /bin/sh - what else;-).
> Flags for the shell can BTW set with ".SHELLFLAGS = ".

setting flags might not be the solution either.

>
> So please
> -) learn basic "Makefile" + "make" before brainlessly patching
> a Makefile.
> -) actually testy your changes to make sure the patch didn't
> broke anything
> -) and - last but not least - check if there isn't a shell
> already set (and which).

btw, I do agree with your points.

>
> MfG,
> Bernd
>

If I said anything incorrect please correct me.


Thanks
Ujjwal Kumar

[Bernd Petrovitsch]

Hi all!

Did you actually *test* that (expecially) these lines work
afterwards as good as before?

> That my proposed change:
> $(shell $(src...) -> $($(CONFIG_SHELL) $(src...)
>
> is WRONG

Yup, as it's in a Makefile and that's a Makefile construct.

> and in the next line he suggested the required correction.
> That being:
> $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)

Such stuff should generally not be needed as the to-be-used
shell can be set in Makefiles via a "SHELL = " assignment

(defaulting to /bin/sh - what else;-).
Flags for the shell can BTW set with ".SHELLFLAGS = ".

So please
-) learn basic "Makefile" + "make" before brainlessly patching
a Makefile.
-) actually testy your changes to make sure the patch didn't
broke anything
-) and - last but not least - check if there isn't a shell
already set (and which).

MfG,
Bernd
--
There is no cloud, just other people computers.
-- https://static.fsf.org/nosvn/stickers/thereisnocloud.svg

[Masahiro Yamada]

You are talking about a different thing.



Take the current code as an example:

$(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")

Here are two shell invocations.


[1]
The command
$(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)"
is run in /bin/sh because the default value of SHELL is /bin/sh.


[2]
The script, arch/ia64/scripts/check-gas, is run in /bin/sh
because the hash-bang (the first line of check-gas)
specifies #!/bin/sh




Bernd is talking about [1].

In contrast, this patch is addressing [2] because
Andrew Morton suggested to run scripts without relying
on the executable bit.
(and, after this patch, we run scripts without relying
on the hash-bang because we now specify the interpreter.)


Of course, [1] and [2] can be different.


I always want to use /bin/sh for [1],
so please do not use bash-extension inside $(shell ...)


You have more choices for [2].

If arch/ia64/scripts/check-gas had been written with bash-extension,
the code would have been changed into:

$(shell $(BASH) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")


I hope this will be clearer.

> So please
> -) learn basic "Makefile" + "make" before brainlessly patching
> a Makefile.
> -) actually testy your changes to make sure the patch didn't
> broke anything
> -) and - last but not least - check if there isn't a shell
> already set (and which).
>
> MfG,
> Bernd
> --
> There is no cloud, just other people computers.
> -- https://static.fsf.org/nosvn/stickers/thereisnocloud.svg

[Masahiro Yamada]

On Tue, Oct 13, 2020 at 2:08 AM Ujjwal Kumar <ujjwalk...@gmail.com> 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.c24c...@linux-foundation.org/
> Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/
>
> Suggested-by: Andrew Morton <ak...@linux-foundation.org>
> Suggested-by: Kees Cook <kees...@chromium.org>
> Suggested-by: Lukas Bulwahn <lukas....@gmail.com>
> Signed-off-by: Ujjwal Kumar <ujjwalk...@gmail.com>
> ---

The patch prefix 'kconfig:' should be used for changes
under scripts/kconfig/.


I want to see both prefixed with "kbuild:".

1/2: kbuild: use interpreters in Kconfig files to invoke scripts
2/2: kbuild: use interpreters in Makefiles to invoke scripts


More preferably, those two patches should be squashed into a
single patch titled "kbuild: use interpreters to invoke scripts"



BTW, I notice some code left unconverted.

For example,
https://github.com/torvalds/linux/blob/master/init/Kconfig#L68
https://github.com/torvalds/linux/blob/v5.9/certs/Makefile#L25

Maybe more...



I know it is difficult to cover everything, but please
re-check the remaining code.

> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-li...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20201012170631.1241502-2-ujjwalkumar0501%40gmail.com.