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/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.

> 
> Henning
>   
>> Jan
>>
> 
Quirin