Re: [PATCH v5 4/5] linux-custom: rewrite to no longer depend on the kernel's builddeb

[Gylstorff Quirin <quirin.gylstorff@siemens.com>]

On 12/12/19 9:01 AM, Jan Kiszka wrote:
> On 12.12.19 08:57, Gylstorff Quirin wrote:
>>
>>
>> On 12/11/19 11:09 PM, Henning Schild wrote:
>>> On Wed, 11 Dec 2019 19:36:05 +0100
>>> Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>
>>>> On 11.12.19 16:43, [ext] Jan Kiszka wrote:
>>>>> On 11.12.19 16:20, Gylstorff Quirin wrote:
>>>>>>> +do_build() {
>>>>>>> +
>>>>>>> +    # Print a few things that are of particular interest
>>>>>>> +    print_settings
>>>>>>> +
>>>>>>> +    # Process existing kernel configuration to make sure it is
>>>>>>> complete
>>>>>>> +    # (use defaults for options that were not specified)
>>>>>>> +    ${MAKE} O=${KERNEL_BUILD_DIR} olddefconfig prepare || exit
>>>>>>> ${?} +
>>>>>>> +    # Check if the recipe's PV makes sense
>>>>>>> +    KR=$(${MAKE} O=${KERNEL_BUILD_DIR} -s --no-print-directory
>>>>>>> kernelrelease)
>>>>>>> +    eval $(grep ^CONFIG_LOCALVERSION=
>>>>>>> ${KERNEL_BUILD_DIR}/${KCONF} || true)
>>>>>>> +    if [ "${PV}-${KERNEL_LOCALVERSION}" != "${KR}" ]; then
>>>>>>> +        echo "ERROR: Recipe version
>>>>>>> (${PV}-${KERNEL_LOCALVERSION}) does not seem to match the
>>>>>>> kernelrelease (${KR})!" 1>&2
>>>>>>> +        echo "ERROR: Make sure the kernel version in your
>>>>>>> NAME/PV/PR settings and/or CONFIG_LOCALVERSION are aligned" 1>&2
>>>>>>> +        exit 1 > +    fi
>>>>>>
>>>>>> we have some CI use case where we build the latest git release
>>>>>> could we add something like this
>>>>>>
>>>>>> -    if [ "${PV}-${KERNEL_LOCALVERSION}" != "${KR}" ]; then
>>>>>> -        echo "ERROR: Recipe version
>>>>>> (${PV}-${KERNEL_LOCALVERSION}) does not seem to match the
>>>>>> kernelrelease (${KR})!" 1>&2
>>>>>> -        echo "ERROR: Make sure the kernel version in your
>>>>>> NAME/PV/PR settings and/or CONFIG_LOCALVERSION are aligned" 1>&2
>>>>>> -        exit 1
>>>>>> +    if [ "${PV}" =~ "latest" ]; then
>>>>>
>>>>> I suspect you wanted to suggest != "latest".
>>>>>    
>>>>>> +        if [ "${PV}-${KERNEL_LOCALVERSION}" != "${KR}" ]; then
>>>>>> +            echo "ERROR: Recipe version
>>>>>> (${PV}-${KERNEL_LOCALVERSION}) does not seem to match the
>>>>>> kernelrelease (${KR})!" 1>&2
>>>>>> +            echo "ERROR: Make sure the kernel version in your
>>>>>> NAME/PV/PR settings and/or CONFIG_LOCALVERSION are aligned" 1>&2
>>>>>> +            exit 1
>>>>>> +        fi
>>>>>
>>>>> We need some relaxation path for the check, yes. Given the other
>>>>> versioning issue, I'm still trying to build a complete picture.
>>>>
>>>> Looking the Henning's commit that introduced the check, it reads to me
>>>> like just addressing constraints of the old build approach. The new
>>>> one has a way to set LOCALVERSION from the recipe.
>>>
>>> Yes, the check is just early catching a weird error that would have
>>> popped up later. That must have been either the build or the step
>>> copying the kernel binary to DEPLOY.
>>>
>>> If a new way of building can deal with it, the check can be dropped.
>>>
>>>> So, what we would rather need than this hard check is the following:
>>>>
>>>>    - optional KERNEL_LOCALVERSION
>>>>    - pick-up of LOCALVERSION from the kernel config for the case it was
>>>>      defined via the config
>>>>    - KERNEL_LOCALVERSION ?= "" to avoid breaking existing users
>>>>      needlessly
>>>>
>>>> That approach would both enable CONFIG_LOCALVERSION usage via own
>>>> configs as well as convenient management in recipes via
>>>> KERNEL_LOCALVERSION. But it has a catch: We need the LOCALVERSION
>>>> information already for the templating step while
>>>> dpkg_configure_kernel is part of the build.
>>>>
>>>> So we may be left with these options:
>>>>
>>>>    - check if CONFIG_LOCALVERSION == KERNEL_LOCALVERSION, which is true
>>>>      when KERNEL_LOCALVERSION is used but could be violated when the
>>>>      custom config provides a LOCALVERSION while KERNEL_LOCALVERSION is
>>>>      empty
>>>>    - always override CONFIG_LOCALVERSION with KERNEL_LOCALVERSION, as in
>>>>      this version of the patch - may cause surprises, though >>>>    - try to pick up CONFIG_LOCALVERSION early, but only from a user-
>>>>      provided defconfig, not from fragments or templates - maybe too
>>>>      unintuitive
>>>>
>>>> Not so easy. Thoughts?
>>>
>>> I am not sure i fully get the suggestion. I think you suggest to have a
>>> bitbake variable control parts of the config ... that one localversion
>>> key in it.
>>>
>>> The user expectation would probably be that the PV will become _the_
>>> version. So i would go for a sanity check for that, and a warning if
>>> not. After that we can discuss a magic that will turn something behind
>>> the first or last "-" in PV into CONFIG_LOCALVERSION and patch that
>>> into the config.
>> You mean a warning or an error? The current version aborts the build, if
>> the versions do not match.
>>
>> As mentioned before: If a mainline kernel is used PV == KERNEL_RELEASE
>> is already not fulfilled. So we already have a two expectation the
>> Debian user and the bitbake user.
>>
>>>
>>> So instead of a new variable, come up with a new recipe naming
>>> convention. And for people that really want to call the recipe
>>> "kernel.bb" they would get the default
>>>
>>> PV = "1.0"
>>> PR = ""
>>> PLOCALV = ""
>>>
>>> Would have to check if "PR" is the thing after the first "-" ... But
>>> maybe PR is what we are looking for ...
>> PR is the revision of the recipe which comes after the first "-". Yocto
>> uses its own variable "LINUX_VERSION_EXTENSION" which sets
>> CONFIG_LOCALVERSION.
> 
> Then we should do s/KERNEL_LOCALVERSION/LINUX_VERSION_EXTENSION.
> 
> Just leaves us with the other policy questions.
> 
> Jan
> 

 From my understanding after reading the patches the KERNEL_LOCALVERSION 
does not affect the build or installation of the Kernel artifacts as the 
mandatory control and changelog elements use only KERNEL_NAME_PROVIDED + 
CHANGELOG_V (PV+PR). So the only occurrence of the LOCALVERSION is in 
uname and the Package description.
If this is the case then the check KERNEL_LOCALVERSION == 
CONFIG_LOCALVERSION and a warning should be enough.

Did I miss something?

Quirin