2023-09-20 03:57:40

by Alexey Kardashevskiy

[permalink] [raw]
Subject: [PATCH kernel] kbuild: get rid of unwanted "+" when CONFIG_LOCALVERSION is set

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 <[email protected]>
---
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


2023-09-20 12:13:58

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH kernel] kbuild: get rid of unwanted "+" when CONFIG_LOCALVERSION is set

On Wed, Sep 20, 2023 at 10:56 AM Alexey Kardashevskiy <[email protected]> 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 <[email protected]>


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
>


--
Best Regards
Masahiro Yamada

2023-09-20 14:10:32

by Alexey Kardashevskiy

[permalink] [raw]
Subject: Re: [PATCH kernel] kbuild: get rid of unwanted "+" when CONFIG_LOCALVERSION is set


On 20/9/23 21:59, Masahiro Yamada wrote:
> On Wed, Sep 20, 2023 at 10:56 AM Alexey Kardashevskiy <[email protected]> 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 <[email protected]>
>
>
> 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
>>
>
>


2023-09-22 07:37:17

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH kernel] kbuild: get rid of unwanted "+" when CONFIG_LOCALVERSION is set

On Wed, Sep 20, 2023 at 10:30 PM Alexey Kardashevskiy <[email protected]> wrote:
>
>
> On 20/9/23 21:59, Masahiro Yamada wrote:
> > On Wed, Sep 20, 2023 at 10:56 AM Alexey Kardashevskiy <[email protected]> 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 <[email protected]>
> >
> >
> > 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

2023-09-22 09:09:25

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH kernel] kbuild: get rid of unwanted "+" when CONFIG_LOCALVERSION is set

On Fri, Sep 22, 2023 at 9:59 AM Alexey Kardashevskiy <[email protected]> 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 <[email protected]>
> >> 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 <[email protected]>
> >>
> >>
> >> 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

2023-09-22 15:00:11

by Alexey Kardashevskiy

[permalink] [raw]
Subject: Re: [PATCH kernel] kbuild: get rid of unwanted "+" when CONFIG_LOCALVERSION is set


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 <[email protected]>
>> 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 <[email protected]>
>>
>>
>> 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.


--
Alexey