| Message ID | 20230920015559.1877441-1-aik@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kernel] kbuild: get rid of unwanted "+" when CONFIG_LOCALVERSION is set | expand |
On Wed, Sep 20, 2023 at 10:56 AM Alexey Kardashevskiy <aik@amd.com> wrote: > > The scripts/setlocalversion script correctly tries not adding "+" when > CONFIG_LOCALVERSION is defined. I am afraid you are misunderstanding the script. CONFIG_LOCALVERSION and the "+" sign is unrelated. > However, instead of grepping for it > (as it is done for CONFIG_LOCALVERSION_AUTO=y), it relies on LOCALVERSION > set in the shell which is not. > > Export LOCALVERSION so scripts/setlocalversion could see it and not add > unwanted "+" at the end of the kernelrelease. > > Signed-off-by: Alexey Kardashevskiy <aik@amd.com> So, scripts/setlocalversion will always see defined LOCALVERSION. With your patch, LOCALVERSION would be set to an empty value, which would make the following condition always false. elif [ "${LOCALVERSION+set}" != "set" ]; then Your patch is equivalent to deleting line 175-183 of scripts/setlocalversion. Of course, that is wrong and unacceptable. > --- > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 57698d048e2c..fc45bed69790 100644 > --- a/Makefile > +++ b/Makefile > @@ -368,7 +368,7 @@ include $(srctree)/scripts/Kbuild.include > # Read KERNELRELEASE from include/config/kernel.release (if it exists) > KERNELRELEASE = $(call read-file, include/config/kernel.release) > KERNELVERSION = $(VERSION)$(if $(PATCHLEVEL),.$(PATCHLEVEL)$(if $(SUBLEVEL),.$(SUBLEVEL)))$(EXTRAVERSION) > -export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION > +export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION LOCALVERSION > > include $(srctree)/scripts/subarch.include > > -- > 2.41.0 >
On 20/9/23 21:59, Masahiro Yamada wrote: > On Wed, Sep 20, 2023 at 10:56 AM Alexey Kardashevskiy <aik@amd.com> wrote: >> >> The scripts/setlocalversion script correctly tries not adding "+" when >> CONFIG_LOCALVERSION is defined. > > I am afraid you are misunderstanding the script. Possibly :) I should have read of "+set"... sorry :-/ Thanks for spending time looking at this. > CONFIG_LOCALVERSION and the "+" sign is unrelated. How come? scripts/setlocalversion -> if [ "${LOCALVERSION+set}" != "set" ] -> scm_version --short -> echo "+". Where is that LOCALVERSION supposed to come from, and when? Is not LOCALVERSION related to CONFIG_LOCALVERSION? >> However, instead of grepping for it >> (as it is done for CONFIG_LOCALVERSION_AUTO=y), it relies on LOCALVERSION >> set in the shell which is not. >> >> Export LOCALVERSION so scripts/setlocalversion could see it and not add >> unwanted "+" at the end of the kernelrelease. >> >> Signed-off-by: Alexey Kardashevskiy <aik@amd.com> > > > So, scripts/setlocalversion will always see > defined LOCALVERSION. > > With your patch, LOCALVERSION would be set to an empty value, > which would make the following condition always false. > > elif [ "${LOCALVERSION+set}" != "set" ]; then > > > Your patch is equivalent to deleting > line 175-183 of scripts/setlocalversion. > > Of course, that is wrong and unacceptable. Ok. What is the right way of getting rid of the "+"? Thanks, > > > > > > > >> --- >> Makefile | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/Makefile b/Makefile >> index 57698d048e2c..fc45bed69790 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -368,7 +368,7 @@ include $(srctree)/scripts/Kbuild.include >> # Read KERNELRELEASE from include/config/kernel.release (if it exists) >> KERNELRELEASE = $(call read-file, include/config/kernel.release) >> KERNELVERSION = $(VERSION)$(if $(PATCHLEVEL),.$(PATCHLEVEL)$(if $(SUBLEVEL),.$(SUBLEVEL)))$(EXTRAVERSION) >> -export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION >> +export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION LOCALVERSION >> >> include $(srctree)/scripts/subarch.include >> >> -- >> 2.41.0 >> > >
On 20/9/23 23:30, Alexey Kardashevskiy wrote: > > On 20/9/23 21:59, Masahiro Yamada wrote: >> On Wed, Sep 20, 2023 at 10:56 AM Alexey Kardashevskiy <aik@amd.com> >> wrote: >>> >>> The scripts/setlocalversion script correctly tries not adding "+" when >>> CONFIG_LOCALVERSION is defined. >> >> I am afraid you are misunderstanding the script. > > Possibly :) I should have read of "+set"... sorry :-/ Thanks for > spending time looking at this. > >> CONFIG_LOCALVERSION and the "+" sign is unrelated. > > How come? > > scripts/setlocalversion -> if [ "${LOCALVERSION+set}" != "set" ] -> > scm_version --short -> echo "+". > > Where is that LOCALVERSION supposed to come from, and when? Is not > LOCALVERSION related to CONFIG_LOCALVERSION? > >>> However, instead of grepping for it >>> (as it is done for CONFIG_LOCALVERSION_AUTO=y), it relies on >>> LOCALVERSION >>> set in the shell which is not. >>> >>> Export LOCALVERSION so scripts/setlocalversion could see it and not add >>> unwanted "+" at the end of the kernelrelease. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com> >> >> >> So, scripts/setlocalversion will always see >> defined LOCALVERSION. >> >> With your patch, LOCALVERSION would be set to an empty value, >> which would make the following condition always false. >> >> elif [ "${LOCALVERSION+set}" != "set" ]; then >> >> >> Your patch is equivalent to deleting >> line 175-183 of scripts/setlocalversion. >> >> Of course, that is wrong and unacceptable. > > Ok. What is the right way of getting rid of the "+"? Thanks, make LOCALVERSION="" ... seems to be the only way, is that right? I thought CONFIG_LOCALVERSION is good enough for not having "+", hmm.
On Wed, Sep 20, 2023 at 10:30 PM Alexey Kardashevskiy <aik@amd.com> wrote: > > > On 20/9/23 21:59, Masahiro Yamada wrote: > > On Wed, Sep 20, 2023 at 10:56 AM Alexey Kardashevskiy <aik@amd.com> wrote: > >> > >> The scripts/setlocalversion script correctly tries not adding "+" when > >> CONFIG_LOCALVERSION is defined. > > > > I am afraid you are misunderstanding the script. > > Possibly :) I should have read of "+set"... sorry :-/ Thanks for > spending time looking at this. > > > CONFIG_LOCALVERSION and the "+" sign is unrelated. > > How come? > > scripts/setlocalversion -> if [ "${LOCALVERSION+set}" != "set" ] -> > scm_version --short -> echo "+". > > Where is that LOCALVERSION supposed to come from, and when? Is not > LOCALVERSION related to CONFIG_LOCALVERSION? 1) LOCALVERSION is an environment variable. $ LOCALVERSION=-foo make or $ make LOCALVERSION=-foo 2) CONFIG_LOCALVERSION is a CONFIG option $ make menuconfig and set the value. 1) and 2) are orthogonal. If you set both, you can get both of them. > >> However, instead of grepping for it > >> (as it is done for CONFIG_LOCALVERSION_AUTO=y), it relies on LOCALVERSION > >> set in the shell which is not. > >> > >> Export LOCALVERSION so scripts/setlocalversion could see it and not add > >> unwanted "+" at the end of the kernelrelease. > >> > >> Signed-off-by: Alexey Kardashevskiy <aik@amd.com> > > > > > > So, scripts/setlocalversion will always see > > defined LOCALVERSION. > > > > With your patch, LOCALVERSION would be set to an empty value, > > which would make the following condition always false. > > > > elif [ "${LOCALVERSION+set}" != "set" ]; then > > > > > > Your patch is equivalent to deleting > > line 175-183 of scripts/setlocalversion. > > > > Of course, that is wrong and unacceptable. > > Ok. What is the right way of getting rid of the "+"? Thanks, -- Best Regards Masahiro Yamada
On Fri, Sep 22, 2023 at 9:59 AM Alexey Kardashevskiy <aik@amd.com> wrote: > > > On 20/9/23 23:30, Alexey Kardashevskiy wrote: > > > > On 20/9/23 21:59, Masahiro Yamada wrote: > >> On Wed, Sep 20, 2023 at 10:56 AM Alexey Kardashevskiy <aik@amd.com> > >> wrote: > >>> > >>> The scripts/setlocalversion script correctly tries not adding "+" when > >>> CONFIG_LOCALVERSION is defined. > >> > >> I am afraid you are misunderstanding the script. > > > > Possibly :) I should have read of "+set"... sorry :-/ Thanks for > > spending time looking at this. > > > >> CONFIG_LOCALVERSION and the "+" sign is unrelated. > > > > How come? > > > > scripts/setlocalversion -> if [ "${LOCALVERSION+set}" != "set" ] -> > > scm_version --short -> echo "+". > > > > Where is that LOCALVERSION supposed to come from, and when? Is not > > LOCALVERSION related to CONFIG_LOCALVERSION? > > > >>> However, instead of grepping for it > >>> (as it is done for CONFIG_LOCALVERSION_AUTO=y), it relies on > >>> LOCALVERSION > >>> set in the shell which is not. > >>> > >>> Export LOCALVERSION so scripts/setlocalversion could see it and not add > >>> unwanted "+" at the end of the kernelrelease. > >>> > >>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com> > >> > >> > >> So, scripts/setlocalversion will always see > >> defined LOCALVERSION. > >> > >> With your patch, LOCALVERSION would be set to an empty value, > >> which would make the following condition always false. > >> > >> elif [ "${LOCALVERSION+set}" != "set" ]; then > >> > >> > >> Your patch is equivalent to deleting > >> line 175-183 of scripts/setlocalversion. > >> > >> Of course, that is wrong and unacceptable. > > > > Ok. What is the right way of getting rid of the "+"? Thanks, > > > make LOCALVERSION="" ... > seems to be the only way, is that right? I thought CONFIG_LOCALVERSION > is good enough for not having "+", hmm. Correct. Reading the commit description of 5df99bec210a2cf89dd91e52f0d0a714bf4cd96a makes it clearer. Best Regards Masahiro Yamada
diff --git a/Makefile b/Makefile index 57698d048e2c..fc45bed69790 100644 --- a/Makefile +++ b/Makefile @@ -368,7 +368,7 @@ include $(srctree)/scripts/Kbuild.include # Read KERNELRELEASE from include/config/kernel.release (if it exists) KERNELRELEASE = $(call read-file, include/config/kernel.release) KERNELVERSION = $(VERSION)$(if $(PATCHLEVEL),.$(PATCHLEVEL)$(if $(SUBLEVEL),.$(SUBLEVEL)))$(EXTRAVERSION) -export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION +export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION LOCALVERSION include $(srctree)/scripts/subarch.include
The scripts/setlocalversion script correctly tries not adding "+" when CONFIG_LOCALVERSION is defined. However, instead of grepping for it (as it is done for CONFIG_LOCALVERSION_AUTO=y), it relies on LOCALVERSION set in the shell which is not. Export LOCALVERSION so scripts/setlocalversion could see it and not add unwanted "+" at the end of the kernelrelease. Signed-off-by: Alexey Kardashevskiy <aik@amd.com> --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)