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
