* [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).