diff mbox series

[v2,2/3] arm: boot: Use double quotes for image name

Message ID	20231104194207.3370542-3-sjg@chromium.org
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7; From: Simon Glass <sjg@chromium.org> To: linux-arm-kernel@lists.infradead.org Cc: U-Boot Mailing List <u-boot@lists.denx.de>, Tom Rini <trini@konsulko.com>, Masahiro Yamada <masahiroy@kernel.org>, Simon Glass <sjg@chromium.org>, Nathan Chancellor <nathan@kernel.org>, Nick Desaulniers <ndesaulniers@google.com>, Nicolas Schier <nicolas@fjasle.eu>, linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v2 2/3] arm: boot: Use double quotes for image name Date: Sat, 4 Nov 2023 13:42:03 -0600 Message-ID: <20231104194207.3370542-3-sjg@chromium.org> In-Reply-To: <20231104194207.3370542-1-sjg@chromium.org> References: <20231104194207.3370542-1-sjg@chromium.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	arm64: Add a build target for Flat Image Tree \| [v2,0/3] arm64: Add a build target for Flat Image Tree [v2,1/3] kbuild: arm64: Add BOOT_TARGETS variable [v2,2/3] arm: boot: Use double quotes for image name [v2,3/3] arm64: boot: Support Flat Image Tree

Commit Message

Simon Glass Nov. 4, 2023, 7:42 p.m. UTC

  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(-)

Comments

Masahiro Yamada Nov. 7, 2023, 10:12 a.m. UTC | #1

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.

Simon Glass Nov. 7, 2023, 1:11 p.m. UTC | #2

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

Masahiro Yamada Nov. 7, 2023, 2:12 p.m. UTC | #3

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

Simon Glass Nov. 7, 2023, 2:47 p.m. UTC | #4

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 mbox series

Patch

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 \