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

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux