Hi Simon,
On Tue, Nov 7, 2023 at 3:11 PM Simon Glass sjg@chromium.org wrote:
Hi Masahiro,
On Tue, 7 Nov 2023 at 03:13, Masahiro Yamada masahiroy@kernel.org wrote:
On Sat, Nov 4, 2023 at 9:42 PM Simon Glass sjg@chromium.org wrote:
The use of single quotes in the image name causes them to appear in the image description when the uImage is created. Use double quotes, to avoid this.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Split double-quote change out into its own patch
scripts/Makefile.lib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 68d0134bdbf9..03e79e319293 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -487,7 +487,7 @@ UIMAGE_OPTS-y ?= UIMAGE_TYPE ?= kernel UIMAGE_LOADADDR ?= arch_must_set_this UIMAGE_ENTRYADDR ?= $(UIMAGE_LOADADDR) -UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)' +UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
quiet_cmd_uimage = UIMAGE $@ cmd_uimage = $(BASH) $(MKIMAGE) -A $(UIMAGE_ARCH) -O linux \ -- 2.42.0.869.gea05f2083d-goog
NACK.
This is because you are doing *WRONG* in 3/3.
Look at your code closely.
https://lore.kernel.org/linux-kbuild/20231104194207.3370542-4-sjg@chromium.o...
In the mainline kernel, the quotation appears only in the definition of UIMAGE_NAME.
masahiro@zoe:~/ref/linux(master)$ git grep UIMAGE_NAME scripts/Makefile.lib:UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)' scripts/Makefile.lib: -n $(UIMAGE_NAME) -d $< $@
The single quotes are consumed by shell.
This is mainline + your patch set.
masahiro@zoe:~/ref/linux(simon-v2)$ git grep UIMAGE_NAME scripts/Makefile.lib:UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)" scripts/Makefile.lib: -n "$(UIMAGE_NAME)" -d $< $@ scripts/Makefile.lib: --name "$(UIMAGE_NAME)" \
You quoted the definition of UIMAGE_NAME, and also variable references.
See how it is expanded.
--name "$(UIMAGE_NAME)"
==>
--name ""Linux-$(KERNELRELEASE)""
==>
--name Linux-$(KERNELRELEASE)
You added double quotes in a row, just to cancel it.
Yes, I understand that. But without the quotes in -n "$(UIMAGE_NAME)" then the name cannot contain spaces. So we do need some sort of quoting, right?
Yes.
If you move the quoting to the variable reference, it is acceptable because there is a good reason to do so.
UIMAGE_NAME ?= Linux-$(KERNELRELEASE)
... -n '$(UIMAGE_NAME)' -d $< $@
This is the correct change.
It just seems strange to use single quotes in a Makefile variable. I found it confusing.
Right. Why don't you remove it, then?
For clarification, there is no concept of quoting in GNU Make.
The single quote character ' and the double quote character " are just normal characters for Make.
GNU Make handles them just like alphabets and numbers.
GNU Make just replaces $(UIMAGE_NAME) with 'Linux-$(KERNELRELEASE)' verbatim.
It is the _shell_ that understands the quoting.
Just in case here is the spec for "2.2.2 Single-Quotes" vs "2.2.3 Double-Quotes"
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
Shell supports both single-quoting and double-quoting for good reasons.
There is no good or bad because both of them are meaningful.
I think you are saying you want to keep the single quotes in the var declaration and drop the quotes from the cmd_fit rule. I am OK with that, but I do think it is unusual not to quote something which might have spaces. It may cause confusion for others, as it did for me?
Anyway, I'll send a new version with the quoting reverted.
Please move the single quotes as I suggested above.
The reason is because UIMAGE_NAME can be passed-in by a user and it can contain whitespaces.
Regards, Simon