* [PATCH] scripts/setlocalversion: fix a bug when LOCALVERSION is empty
@ 2021-07-12 14:00 Mikulas Patocka
2021-07-12 18:15 ` Greg Kroah-Hartman
0 siblings, 1 reply; 11+ messages in thread
From: Mikulas Patocka @ 2021-07-12 14:00 UTC (permalink / raw)
To: Masahiro Yamada, Greg Kroah-Hartman, Nico Schottelius; +Cc: linux-kernel
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 <mpatocka@redhat.com>
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] scripts/setlocalversion: fix a bug when LOCALVERSION is empty
2021-07-12 14:00 [PATCH] scripts/setlocalversion: fix a bug when LOCALVERSION is empty Mikulas Patocka
@ 2021-07-12 18:15 ` Greg Kroah-Hartman
2021-07-12 19:06 ` [PATCH v2] " Mikulas Patocka
0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2021-07-12 18:15 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: Masahiro Yamada, Nico Schottelius, linux-kernel
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 <mpatocka@redhat.com>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] scripts/setlocalversion: fix a bug when LOCALVERSION is empty
2021-07-12 18:15 ` Greg Kroah-Hartman
@ 2021-07-12 19:06 ` Mikulas Patocka
2021-07-12 19:16 ` Greg Kroah-Hartman
0 siblings, 1 reply; 11+ messages in thread
From: Mikulas Patocka @ 2021-07-12 19:06 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Masahiro Yamada, Nico Schottelius, linux-kernel
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 <mpatocka@redhat.com>
> > 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 <mpatocka@redhat.com>
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 <mpatocka@redhat.com>
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] scripts/setlocalversion: fix a bug when LOCALVERSION is empty
2021-07-12 19:06 ` [PATCH v2] " Mikulas Patocka
@ 2021-07-12 19:16 ` Greg Kroah-Hartman
2021-07-12 19:35 ` [PATCH v3] " Mikulas Patocka
0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2021-07-12 19:16 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: Masahiro Yamada, Nico Schottelius, linux-kernel
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 <mpatocka@redhat.com>
> > > 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 <mpatocka@redhat.com>
>
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 <mpatocka@redhat.com>
> Fixes: 042da426f8eb ("scripts/setlocalversion: simplify the short version part")
See, use that way to quote a commit.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty
2021-07-12 19:16 ` Greg Kroah-Hartman
@ 2021-07-12 19:35 ` Mikulas Patocka
2021-07-12 21:17 ` Nico Schottelius
0 siblings, 1 reply; 11+ messages in thread
From: Mikulas Patocka @ 2021-07-12 19:35 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Masahiro Yamada, Nico Schottelius, linux-kernel
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 <mpatocka@redhat.com>
> > > > 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 <mpatocka@redhat.com>
> >
>
> 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 <mpatocka@redhat.com>
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 <mpatocka@redhat.com>
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty
2021-07-12 19:35 ` [PATCH v3] " Mikulas Patocka
@ 2021-07-12 21:17 ` Nico Schottelius
2021-07-13 8:59 ` Mikulas Patocka
0 siblings, 1 reply; 11+ messages in thread
From: Nico Schottelius @ 2021-07-12 21:17 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Greg Kroah-Hartman, Masahiro Yamada, Nico Schottelius, linux-kernel
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 <mpatocka@redhat.com> 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 <mpatocka@redhat.com>
>> > > > 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 <mpatocka@redhat.com>
>> >
>>
>> 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 <mpatocka@redhat.com>
>
> 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 <mpatocka@redhat.com>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty
2021-07-12 21:17 ` Nico Schottelius
@ 2021-07-13 8:59 ` Mikulas Patocka
2021-07-13 9:11 ` Nico Schottelius
2021-07-14 11:54 ` Masahiro Yamada
0 siblings, 2 replies; 11+ messages in thread
From: Mikulas Patocka @ 2021-07-13 8:59 UTC (permalink / raw)
To: Nico Schottelius; +Cc: Greg Kroah-Hartman, Masahiro Yamada, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty
2021-07-13 8:59 ` Mikulas Patocka
@ 2021-07-13 9:11 ` Nico Schottelius
2021-07-13 11:43 ` Mikulas Patocka
2021-07-14 11:54 ` Masahiro Yamada
1 sibling, 1 reply; 11+ messages in thread
From: Nico Schottelius @ 2021-07-13 9:11 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Nico Schottelius, Greg Kroah-Hartman, Masahiro Yamada, linux-kernel
Mikulas Patocka <mpatocka@redhat.com> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty
2021-07-13 9:11 ` Nico Schottelius
@ 2021-07-13 11:43 ` Mikulas Patocka
0 siblings, 0 replies; 11+ messages in thread
From: Mikulas Patocka @ 2021-07-13 11:43 UTC (permalink / raw)
To: Nico Schottelius
Cc: Nico Schottelius, Greg Kroah-Hartman, Masahiro Yamada, linux-kernel
On Tue, 13 Jul 2021, Nico Schottelius wrote:
>
>
> Mikulas Patocka <mpatocka@redhat.com> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty
2021-07-13 8:59 ` Mikulas Patocka
2021-07-13 9:11 ` Nico Schottelius
@ 2021-07-14 11:54 ` Masahiro Yamada
2021-07-18 13:27 ` Masahiro Yamada
1 sibling, 1 reply; 11+ messages in thread
From: Masahiro Yamada @ 2021-07-14 11:54 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Nico Schottelius, Greg Kroah-Hartman, Linux Kernel Mailing List
On Tue, Jul 13, 2021 at 5:59 PM Mikulas Patocka <mpatocka@redhat.com> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] scripts/setlocalversion: fix a bug when LOCALVERSION is empty
2021-07-14 11:54 ` Masahiro Yamada
@ 2021-07-18 13:27 ` Masahiro Yamada
0 siblings, 0 replies; 11+ messages in thread
From: Masahiro Yamada @ 2021-07-18 13:27 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Nico Schottelius, Greg Kroah-Hartman, Linux Kernel Mailing List
On Wed, Jul 14, 2021 at 8:54 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, Jul 13, 2021 at 5:59 PM Mikulas Patocka <mpatocka@redhat.com> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-07-18 13:28 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-12 14:00 [PATCH] scripts/setlocalversion: fix a bug when LOCALVERSION is empty Mikulas Patocka
2021-07-12 18:15 ` Greg Kroah-Hartman
2021-07-12 19:06 ` [PATCH v2] " Mikulas Patocka
2021-07-12 19:16 ` Greg Kroah-Hartman
2021-07-12 19:35 ` [PATCH v3] " Mikulas Patocka
2021-07-12 21:17 ` Nico Schottelius
2021-07-13 8:59 ` Mikulas Patocka
2021-07-13 9:11 ` Nico Schottelius
2021-07-13 11:43 ` Mikulas Patocka
2021-07-14 11:54 ` Masahiro Yamada
2021-07-18 13:27 ` Masahiro Yamada
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).