2021-07-12 14:02:28

by Mikulas Patocka

[permalink] [raw]
Subject: [PATCH] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

The patch 042da426f8ebde012be9429ff705232af7ad7469
("scripts/setlocalversion: simplify the short version part") reduces the
indentation. Unfortunatelly, it also changes behavior in a subtle way - if
the user has empty "LOCALVERSION" variable, the plus sign is appended to
the kernel version. It wasn't appended before.

This patch reverts to the old behavior - we append the plus sign only if
the LOCALVERSION variable is not set.

Signed-off-by: Mikulas Patocka <[email protected]>
Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")

---
scripts/setlocalversion | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/scripts/setlocalversion
===================================================================
--- linux-2.6.orig/scripts/setlocalversion 2021-07-12 15:29:07.000000000 +0200
+++ linux-2.6/scripts/setlocalversion 2021-07-12 15:50:29.000000000 +0200
@@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
# full scm version string
res="$res$(scm_version)"
-elif [ -z "${LOCALVERSION}" ]; then
+elif [ "${LOCALVERSION+set}" != "set" ]; then
# append a plus sign if the repository is not in a clean
# annotated or signed tagged state (as git describe only
# looks at signed or annotated tags - git tag -a/-s) and


2021-07-12 18:18:05

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

On Mon, Jul 12, 2021 at 10:00:59AM -0400, Mikulas Patocka wrote:
> The patch 042da426f8ebde012be9429ff705232af7ad7469
> ("scripts/setlocalversion: simplify the short version part") reduces the
> indentation. Unfortunatelly, it also changes behavior in a subtle way - if
> the user has empty "LOCALVERSION" variable, the plus sign is appended to
> the kernel version. It wasn't appended before.
>
> This patch reverts to the old behavior - we append the plus sign only if
> the LOCALVERSION variable is not set.
>
> Signed-off-by: Mikulas Patocka <[email protected]>
> Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
>
> ---
> scripts/setlocalversion | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: linux-2.6/scripts/setlocalversion
> ===================================================================
> --- linux-2.6.orig/scripts/setlocalversion 2021-07-12 15:29:07.000000000 +0200
> +++ linux-2.6/scripts/setlocalversion 2021-07-12 15:50:29.000000000 +0200
> @@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
> if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
> # full scm version string
> res="$res$(scm_version)"
> -elif [ -z "${LOCALVERSION}" ]; then
> +elif [ "${LOCALVERSION+set}" != "set" ]; then

That's really subtle, can you add a comment here that this handles an
empty file?

thanks,

greg k-h

2021-07-12 19:08:15

by Mikulas Patocka

[permalink] [raw]
Subject: [PATCH v2] scripts/setlocalversion: fix a bug when LOCALVERSION is empty



On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:

> On Mon, Jul 12, 2021 at 10:00:59AM -0400, Mikulas Patocka wrote:
> > The patch 042da426f8ebde012be9429ff705232af7ad7469
> > ("scripts/setlocalversion: simplify the short version part") reduces the
> > indentation. Unfortunatelly, it also changes behavior in a subtle way - if
> > the user has empty "LOCALVERSION" variable, the plus sign is appended to
> > the kernel version. It wasn't appended before.
> >
> > This patch reverts to the old behavior - we append the plus sign only if
> > the LOCALVERSION variable is not set.
> >
> > Signed-off-by: Mikulas Patocka <[email protected]>
> > Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
> >
> > ---
> > scripts/setlocalversion | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Index: linux-2.6/scripts/setlocalversion
> > ===================================================================
> > --- linux-2.6.orig/scripts/setlocalversion 2021-07-12 15:29:07.000000000 +0200
> > +++ linux-2.6/scripts/setlocalversion 2021-07-12 15:50:29.000000000 +0200
> > @@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
> > if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
> > # full scm version string
> > res="$res$(scm_version)"
> > -elif [ -z "${LOCALVERSION}" ]; then
> > +elif [ "${LOCALVERSION+set}" != "set" ]; then
>
> That's really subtle, can you add a comment here that this handles an
> empty file?
>
> thanks,
>
> greg k-h

OK.


From: Mikulas Patocka <[email protected]>

The patch 042da426f8ebde012be9429ff705232af7ad7469
("scripts/setlocalversion: simplify the short version part") reduces
indentation. Unfortunatelly, it also changes behavior in a subtle way - if
the user has empty "LOCALVERSION" variable, the plus sign is appended to
the kernel version. It wasn't appended before.

This patch reverts to the old behavior - we append the plus sign only if
the LOCALVERSION variable is not set.

Signed-off-by: Mikulas Patocka <[email protected]>
Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")

---
scripts/setlocalversion | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

Index: linux-2.6/scripts/setlocalversion
===================================================================
--- linux-2.6.orig/scripts/setlocalversion 2021-07-12 15:29:07.000000000 +0200
+++ linux-2.6/scripts/setlocalversion 2021-07-12 21:00:59.000000000 +0200
@@ -131,11 +131,14 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
# full scm version string
res="$res$(scm_version)"
-elif [ -z "${LOCALVERSION}" ]; then
- # append a plus sign if the repository is not in a clean
- # annotated or signed tagged state (as git describe only
- # looks at signed or annotated tags - git tag -a/-s) and
- # LOCALVERSION= is not specified
+elif [ "${LOCALVERSION+set}" != "set" ]; then
+ # If the variable LOCALVERSION is not set, append a plus
+ # sign if the repository is not in a clean annotated or
+ # signed tagged state (as git describe only looks at signed
+ # or annotated tags - git tag -a/-s).
+ #
+ # If the variable LOCALVERSION is set (including being set
+ # to an empty string), we don't want to append a plus sign.
scm=$(scm_version --short)
res="$res${scm:++}"
fi

2021-07-12 19:18:09

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v2] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

On Mon, Jul 12, 2021 at 03:06:48PM -0400, Mikulas Patocka wrote:
>
>
> On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:
>
> > On Mon, Jul 12, 2021 at 10:00:59AM -0400, Mikulas Patocka wrote:
> > > The patch 042da426f8ebde012be9429ff705232af7ad7469
> > > ("scripts/setlocalversion: simplify the short version part") reduces the
> > > indentation. Unfortunatelly, it also changes behavior in a subtle way - if
> > > the user has empty "LOCALVERSION" variable, the plus sign is appended to
> > > the kernel version. It wasn't appended before.
> > >
> > > This patch reverts to the old behavior - we append the plus sign only if
> > > the LOCALVERSION variable is not set.
> > >
> > > Signed-off-by: Mikulas Patocka <[email protected]>
> > > Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
> > >
> > > ---
> > > scripts/setlocalversion | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > Index: linux-2.6/scripts/setlocalversion
> > > ===================================================================
> > > --- linux-2.6.orig/scripts/setlocalversion 2021-07-12 15:29:07.000000000 +0200
> > > +++ linux-2.6/scripts/setlocalversion 2021-07-12 15:50:29.000000000 +0200
> > > @@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
> > > if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
> > > # full scm version string
> > > res="$res$(scm_version)"
> > > -elif [ -z "${LOCALVERSION}" ]; then
> > > +elif [ "${LOCALVERSION+set}" != "set" ]; then
> >
> > That's really subtle, can you add a comment here that this handles an
> > empty file?
> >
> > thanks,
> >
> > greg k-h
>
> OK.
>
>
> From: Mikulas Patocka <[email protected]>
>

I can't take a patch like this :(

> The patch 042da426f8ebde012be9429ff705232af7ad7469
> ("scripts/setlocalversion: simplify the short version part") reduces

Properly quote commits, the documentation says you do not need to use
the full sha1.


> indentation. Unfortunatelly, it also changes behavior in a subtle way - if
> the user has empty "LOCALVERSION" variable, the plus sign is appended to
> the kernel version. It wasn't appended before.
>
> This patch reverts to the old behavior - we append the plus sign only if
> the LOCALVERSION variable is not set.
>
> Signed-off-by: Mikulas Patocka <[email protected]>
> Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")

See, use that way to quote a commit.

thanks,

greg k-h

2021-07-12 19:37:46

by Mikulas Patocka

[permalink] [raw]
Subject: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty



On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:

> On Mon, Jul 12, 2021 at 03:06:48PM -0400, Mikulas Patocka wrote:
> >
> >
> > On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:
> >
> > > On Mon, Jul 12, 2021 at 10:00:59AM -0400, Mikulas Patocka wrote:
> > > > The patch 042da426f8ebde012be9429ff705232af7ad7469
> > > > ("scripts/setlocalversion: simplify the short version part") reduces the
> > > > indentation. Unfortunatelly, it also changes behavior in a subtle way - if
> > > > the user has empty "LOCALVERSION" variable, the plus sign is appended to
> > > > the kernel version. It wasn't appended before.
> > > >
> > > > This patch reverts to the old behavior - we append the plus sign only if
> > > > the LOCALVERSION variable is not set.
> > > >
> > > > Signed-off-by: Mikulas Patocka <[email protected]>
> > > > Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
> > > >
> > > > ---
> > > > scripts/setlocalversion | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > Index: linux-2.6/scripts/setlocalversion
> > > > ===================================================================
> > > > --- linux-2.6.orig/scripts/setlocalversion 2021-07-12 15:29:07.000000000 +0200
> > > > +++ linux-2.6/scripts/setlocalversion 2021-07-12 15:50:29.000000000 +0200
> > > > @@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
> > > > if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
> > > > # full scm version string
> > > > res="$res$(scm_version)"
> > > > -elif [ -z "${LOCALVERSION}" ]; then
> > > > +elif [ "${LOCALVERSION+set}" != "set" ]; then
> > >
> > > That's really subtle, can you add a comment here that this handles an
> > > empty file?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > OK.
> >
> >
> > From: Mikulas Patocka <[email protected]>
> >
>
> I can't take a patch like this :(
>
> > The patch 042da426f8ebde012be9429ff705232af7ad7469
> > ("scripts/setlocalversion: simplify the short version part") reduces
>
> Properly quote commits, the documentation says you do not need to use
> the full sha1.



From: Mikulas Patocka <[email protected]>

The commit 042da426f8eb ("scripts/setlocalversion: simplify the short
version part") reduces indentation. Unfortunately, it also changes behavior
in a subtle way - if the user has empty "LOCALVERSION" variable, the plus
sign is appended to the kernel version. It wasn't appended before.

This patch reverts to the old behavior - we append the plus sign only if
the LOCALVERSION variable is not set.

Signed-off-by: Mikulas Patocka <[email protected]>
Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")

---
scripts/setlocalversion | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

Index: linux-2.6/scripts/setlocalversion
===================================================================
--- linux-2.6.orig/scripts/setlocalversion 2021-07-12 15:29:07.000000000 +0200
+++ linux-2.6/scripts/setlocalversion 2021-07-12 21:00:59.000000000 +0200
@@ -131,11 +131,14 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
# full scm version string
res="$res$(scm_version)"
-elif [ -z "${LOCALVERSION}" ]; then
- # append a plus sign if the repository is not in a clean
- # annotated or signed tagged state (as git describe only
- # looks at signed or annotated tags - git tag -a/-s) and
- # LOCALVERSION= is not specified
+elif [ "${LOCALVERSION+set}" != "set" ]; then
+ # If the variable LOCALVERSION is not set, append a plus
+ # sign if the repository is not in a clean annotated or
+ # signed tagged state (as git describe only looks at signed
+ # or annotated tags - git tag -a/-s).
+ #
+ # If the variable LOCALVERSION is set (including being set
+ # to an empty string), we don't want to append a plus sign.
scm=$(scm_version --short)
res="$res${scm:++}"
fi

2021-07-12 21:25:51

by Nico Schottelius

[permalink] [raw]
Subject: Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty


Sorry to say, but I am not convinced by the patch.

While yes, we might have changed the behaviour slightly, reading
something on the line of

if [ -z ... ]

is significantly more simple, elegant and easier to maintain, than
a rather atypical special case for setting a variable to empty,
using

if [ "${LOCALVERSION+set}" != "set" ] ..

*and* because it is so atypical, adding a long comment for it, too.

Additonally, -z should be correct if the variable *is* truly empty. I
assume it actually isn't and contains whitespace, which is not the same
as being set and empty.

Instead of re-adding complexity, could you consider changing the build
flow so that LOCALVERSION is either unset or empty?

Nico

Executed in bash:

% touch empty
% a=$(cat empty)
% [ -z "$a" ] && echo "really empty"
really empty


Mikulas Patocka <[email protected]> writes:

> On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:
>
>> On Mon, Jul 12, 2021 at 03:06:48PM -0400, Mikulas Patocka wrote:
>> >
>> >
>> > On Mon, 12 Jul 2021, Greg Kroah-Hartman wrote:
>> >
>> > > On Mon, Jul 12, 2021 at 10:00:59AM -0400, Mikulas Patocka wrote:
>> > > > The patch 042da426f8ebde012be9429ff705232af7ad7469
>> > > > ("scripts/setlocalversion: simplify the short version part") reduces the
>> > > > indentation. Unfortunatelly, it also changes behavior in a subtle way - if
>> > > > the user has empty "LOCALVERSION" variable, the plus sign is appended to
>> > > > the kernel version. It wasn't appended before.
>> > > >
>> > > > This patch reverts to the old behavior - we append the plus sign only if
>> > > > the LOCALVERSION variable is not set.
>> > > >
>> > > > Signed-off-by: Mikulas Patocka <[email protected]>
>> > > > Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
>> > > >
>> > > > ---
>> > > > scripts/setlocalversion | 2 +-
>> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > > >
>> > > > Index: linux-2.6/scripts/setlocalversion
>> > > > ===================================================================
>> > > > --- linux-2.6.orig/scripts/setlocalversion 2021-07-12 15:29:07.000000000 +0200
>> > > > +++ linux-2.6/scripts/setlocalversion 2021-07-12 15:50:29.000000000 +0200
>> > > > @@ -131,7 +131,7 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
>> > > > if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
>> > > > # full scm version string
>> > > > res="$res$(scm_version)"
>> > > > -elif [ -z "${LOCALVERSION}" ]; then
>> > > > +elif [ "${LOCALVERSION+set}" != "set" ]; then
>> > >
>> > > That's really subtle, can you add a comment here that this handles an
>> > > empty file?
>> > >
>> > > thanks,
>> > >
>> > > greg k-h
>> >
>> > OK.
>> >
>> >
>> > From: Mikulas Patocka <[email protected]>
>> >
>>
>> I can't take a patch like this :(
>>
>> > The patch 042da426f8ebde012be9429ff705232af7ad7469
>> > ("scripts/setlocalversion: simplify the short version part") reduces
>>
>> Properly quote commits, the documentation says you do not need to use
>> the full sha1.
>
>
>
> From: Mikulas Patocka <[email protected]>
>
> The commit 042da426f8eb ("scripts/setlocalversion: simplify the short
> version part") reduces indentation. Unfortunately, it also changes behavior
> in a subtle way - if the user has empty "LOCALVERSION" variable, the plus
> sign is appended to the kernel version. It wasn't appended before.
>
> This patch reverts to the old behavior - we append the plus sign only if
> the LOCALVERSION variable is not set.
>
> Signed-off-by: Mikulas Patocka <[email protected]>
> Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
>
> ---
> scripts/setlocalversion | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> Index: linux-2.6/scripts/setlocalversion
> ===================================================================
> --- linux-2.6.orig/scripts/setlocalversion 2021-07-12 15:29:07.000000000 +0200
> +++ linux-2.6/scripts/setlocalversion 2021-07-12 21:00:59.000000000 +0200
> @@ -131,11 +131,14 @@ res="${res}${CONFIG_LOCALVERSION}${LOCAL
> if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
> # full scm version string
> res="$res$(scm_version)"
> -elif [ -z "${LOCALVERSION}" ]; then
> - # append a plus sign if the repository is not in a clean
> - # annotated or signed tagged state (as git describe only
> - # looks at signed or annotated tags - git tag -a/-s) and
> - # LOCALVERSION= is not specified
> +elif [ "${LOCALVERSION+set}" != "set" ]; then
> + # If the variable LOCALVERSION is not set, append a plus
> + # sign if the repository is not in a clean annotated or
> + # signed tagged state (as git describe only looks at signed
> + # or annotated tags - git tag -a/-s).
> + #
> + # If the variable LOCALVERSION is set (including being set
> + # to an empty string), we don't want to append a plus sign.
> scm=$(scm_version --short)
> res="$res${scm:++}"
> fi


--
Sustainable and modern Infrastructures by ungleich.ch

2021-07-13 09:00:42

by Mikulas Patocka

[permalink] [raw]
Subject: Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty



On Mon, 12 Jul 2021, Nico Schottelius wrote:

>
> Sorry to say, but I am not convinced by the patch.
>
> While yes, we might have changed the behaviour slightly, reading
> something on the line of
>
> if [ -z ... ]
>
> is significantly more simple, elegant and easier to maintain, than
> a rather atypical special case for setting a variable to empty,
> using
>
> if [ "${LOCALVERSION+set}" != "set" ] ..
>
> *and* because it is so atypical, adding a long comment for it, too.
>
> Additonally, -z should be correct if the variable *is* truly empty. I
> assume it actually isn't and contains whitespace, which is not the same
> as being set and empty.
>
> Instead of re-adding complexity, could you consider changing the build
> flow so that LOCALVERSION is either unset or empty?
>
> Nico

I set LOCALVERSION to an empty string (with "export LOCALVERSION="). This
prevented the kernel from adding a "+" sign to the kernel version. Since
the commit 042da426f8eb, it no longer works and the kernel adds a "+" sign
if LOCALVERSION is set and empty.

If you don't like "if [ "${LOCALVERSION+set}" != "set" ]", then please
provide some other way how to test if the variable is set.

Mikulas

2021-07-13 09:12:04

by Nico Schottelius

[permalink] [raw]
Subject: Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty



Mikulas Patocka <[email protected]> writes:
> I set LOCALVERSION to an empty string (with "export LOCALVERSION="). This
> prevented the kernel from adding a "+" sign to the kernel version. Since
> the commit 042da426f8eb, it no longer works and the kernel adds a "+" sign
> if LOCALVERSION is set and empty.
>
> If you don't like "if [ "${LOCALVERSION+set}" != "set" ]", then please
> provide some other way how to test if the variable is set.

I fail to see the problem you are solving, as that case works exactly
like I wrote in my last mail:

[11:09:03] nb3:~$ export LOCALVERSION=; [ -z "${LOCALVERSION}" ] && echo unset
unset
[11:09:27] nb3:~$ echo $BASH_VERSION
5.1.8(1)-release

Did you try that in your environment?


--
Sustainable and modern Infrastructures by ungleich.ch

2021-07-13 11:46:13

by Mikulas Patocka

[permalink] [raw]
Subject: Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty



On Tue, 13 Jul 2021, Nico Schottelius wrote:

>
>
> Mikulas Patocka <[email protected]> writes:
> > I set LOCALVERSION to an empty string (with "export LOCALVERSION="). This
> > prevented the kernel from adding a "+" sign to the kernel version. Since
> > the commit 042da426f8eb, it no longer works and the kernel adds a "+" sign
> > if LOCALVERSION is set and empty.
> >
> > If you don't like "if [ "${LOCALVERSION+set}" != "set" ]", then please
> > provide some other way how to test if the variable is set.
>
> I fail to see the problem you are solving, as that case works exactly
> like I wrote in my last mail:

The problem is this - I have a few patches applied to the kernel and I
don't want the plus sign in the kernel version. So, I set
"export LOCALVERSION=". In the kernel 5.13 and previous versions, there is
no plus sign at the end of version string. With the version 5.14-rc1,
there is a plus sign.because of the patch 042da426f8eb.

> [11:09:03] nb3:~$ export LOCALVERSION=; [ -z "${LOCALVERSION}" ] && echo unset
> unset
> [11:09:27] nb3:~$ echo $BASH_VERSION
> 5.1.8(1)-release
>
> Did you try that in your environment?

I tried and it and it doesn't work - the plus sign is present in the
kernel version because [ -z "${LOCALVERSION}" ] return true.

Mikulas

2021-07-14 11:57:16

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

On Tue, Jul 13, 2021 at 5:59 PM Mikulas Patocka <[email protected]> wrote:
>
>
>
> On Mon, 12 Jul 2021, Nico Schottelius wrote:
>
> >
> > Sorry to say, but I am not convinced by the patch.
> >
> > While yes, we might have changed the behaviour slightly, reading
> > something on the line of
> >
> > if [ -z ... ]
> >
> > is significantly more simple, elegant and easier to maintain, than
> > a rather atypical special case for setting a variable to empty,
> > using
> >
> > if [ "${LOCALVERSION+set}" != "set" ] ..
> >
> > *and* because it is so atypical, adding a long comment for it, too.
> >
> > Additonally, -z should be correct if the variable *is* truly empty. I
> > assume it actually isn't and contains whitespace, which is not the same
> > as being set and empty.
> >
> > Instead of re-adding complexity, could you consider changing the build
> > flow so that LOCALVERSION is either unset or empty?
> >
> > Nico
>
> I set LOCALVERSION to an empty string (with "export LOCALVERSION="). This
> prevented the kernel from adding a "+" sign to the kernel version. Since
> the commit 042da426f8eb, it no longer works and the kernel adds a "+" sign
> if LOCALVERSION is set and empty.
>
> If you don't like "if [ "${LOCALVERSION+set}" != "set" ]", then please
> provide some other way how to test if the variable is set.

if [ -z "${LOCALVERSION+set}" ]

is the alternative way I came up with, but
I am OK with your v3.


I will pick it up for -rc2.




--
Best Regards
Masahiro Yamada

2021-07-18 13:29:20

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty

On Wed, Jul 14, 2021 at 8:54 PM Masahiro Yamada <[email protected]> wrote:
>
> On Tue, Jul 13, 2021 at 5:59 PM Mikulas Patocka <[email protected]> wrote:
> >
> >
> >
> > On Mon, 12 Jul 2021, Nico Schottelius wrote:
> >
> > >
> > > Sorry to say, but I am not convinced by the patch.
> > >
> > > While yes, we might have changed the behaviour slightly, reading
> > > something on the line of
> > >
> > > if [ -z ... ]
> > >
> > > is significantly more simple, elegant and easier to maintain, than
> > > a rather atypical special case for setting a variable to empty,
> > > using
> > >
> > > if [ "${LOCALVERSION+set}" != "set" ] ..
> > >
> > > *and* because it is so atypical, adding a long comment for it, too.
> > >
> > > Additonally, -z should be correct if the variable *is* truly empty. I
> > > assume it actually isn't and contains whitespace, which is not the same
> > > as being set and empty.
> > >
> > > Instead of re-adding complexity, could you consider changing the build
> > > flow so that LOCALVERSION is either unset or empty?
> > >
> > > Nico
> >
> > I set LOCALVERSION to an empty string (with "export LOCALVERSION="). This
> > prevented the kernel from adding a "+" sign to the kernel version. Since
> > the commit 042da426f8eb, it no longer works and the kernel adds a "+" sign
> > if LOCALVERSION is set and empty.
> >
> > If you don't like "if [ "${LOCALVERSION+set}" != "set" ]", then please
> > provide some other way how to test if the variable is set.
>
> if [ -z "${LOCALVERSION+set}" ]
>
> is the alternative way I came up with, but
> I am OK with your v3.
>
>
> I will pick it up for -rc2.
>
>

Applied to linux-kbuild.
Thanks.




--
Best Regards
Masahiro Yamada