| Message ID | 20231104194207.3370542-3-sjg@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: Add a build target for Flat Image Tree | expand |
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.org/T/#me2fb68151d6f4f330808406f9a711fffee149529 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.
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.org/T/#me2fb68151d6f4f330808406f9a711fffee149529 > > > > 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? It just seems strange to use single quotes in a Makefile variable. I found it confusing. 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. Regards, Simon
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.org/T/#me2fb68151d6f4f330808406f9a711fffee149529 > > > > > > > > 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
Hi Masahiro, On Tue, 7 Nov 2023 at 07:13, Masahiro Yamada <masahiroy@kernel.org> wrote: > > 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.org/T/#me2fb68151d6f4f330808406f9a711fffee149529 > > > > > > > > > > > > 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. OK. > > > > > > > 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. Yes...I suppose I knew that Makefiles are completely literal, but thanks for the pointers. I tend to use double quotes by default and single quotes only when I have to...but it doesn't really matter so long as it is consistent. Anyway, moving the single quotes away from the var removes the confusion I had at the start of all of this. > > > > > > > > > > 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. OK, done in v4. Regards, Simon
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 \
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(-)