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 10:46 AM, [ext] Gylstorff Quirin wrote:
> 
> 
> 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?
Yep I missed the (pre/post)inst/rm scripts. These are the only 
occurrence of the KERNEL_LOCALVERSION. This would require the abort of 
the build if some mismatch occurs.

Quirin