Re: [OE-core] [master][PATCH 4/5] package / packagedata bbclass: Change the way PRAUTO and AUTOINC are handled

["Mark Hatle" <mark.hatle@kernel.crashing.org>]

On 8/25/20 12:04 PM, Richard Purdie wrote:
> On Tue, 2020-08-25 at 11:52 -0500, Mark Hatle wrote:
>>
>> On 8/25/20 9:14 AM, Mark Hatle wrote:
>>>
>>> On 8/25/20 5:56 AM, Richard Purdie wrote:
>>>> On Mon, 2020-08-24 at 18:29 -0500, Mark Hatle wrote:
>>>>> A related change was also needed for the kernel.bbclass.  The
>>>>> kernel
>>>>>
>>>>> needs to get the AUTOINC values used by the various PV/PKGV,
>>>>> but with
>>>>> this new system we don't want to call auto_pr each time --
>>>>> instead
>>>>> call the read_subpackage_metadata to ensure a consistent
>>>>> result.
>>>>>
>>>>> This also fixes an issue in the old version where the kernel
>>>>> sometimes
>>>>> caused the PR to be incremented twice, as the auto_pr was
>>>>> called
>>>>> multiple times from different tasks.
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/meta/classes/kernel.bbclass
>>>>> b/meta/classes/kernel.bbclass
>>>>> index e2ceb6a333..4449452065 100644
>>>>> --- a/meta/classes/kernel.bbclass
>>>>> +++ b/meta/classes/kernel.bbclass
>>>>> @@ -416,7 +416,7 @@ kernel_do_install() {
>>>>>  	install -d ${D}${sysconfdir}/modules-load.d
>>>>>  	install -d ${D}${sysconfdir}/modprobe.d
>>>>>  }
>>>>> -do_install[prefuncs] += "package_get_auto_pr"
>>>>> +do_install[prefuncs] += "read_subpackage_metadata"
>>
>> I talked with Bruce on this.  We see no reason do_install[prefuncs]
>> is required
>> any longer.
>>
>> It was when that line was written, but when the kernel was refactored
>> to permit multiple kernel builds and such the need for this went
>> away.
> 
> Ok, lets start with a patch to remove that call.
> 
>>>>>  # Must be ran no earlier than after do_kernel_checkout or else
>>>>> Makefile won't be in ${S}/Makefile
>>>>>  do_kernel_version_sanity_check() {
>>>>> @@ -749,7 +749,7 @@ kernel_do_deploy() {
>>>>>  		done
>>>>>  	fi
>>>>>  }
>>>>> -do_deploy[prefuncs] += "package_get_auto_pr"
>>>>> +do_deploy[prefuncs] += "read_subpackage_metadata"
>>>>>  
>>>>>  addtask deploy after do_populate_sysroot do_packagedata
>>>>
>>>> I don't think this can be right. I understand why the kernel was
>>>> expanding these early but at this point do_package hasn't
>>>> happened so
>>>> reading the subpackage metadata is a null op at best and
>>>> potentially
>>>> changes a lot more in the data store.
>>>>
>>>> I suspect this is not doing the right thing and will be something
>>>> that
>>>> comes back to bite us so we need something more careful here and
>>>> I'd
>>>> like to better understand exactly what change to the variables
>>>> do_install and do_deploy need.
>>>
>>> I don't know exactly why it's doing this.  But I believe it is so
>>> that the
>>> PV/PKGV is expanded (AUTOINC translated).  I don't see any other
>>> reason for it
>>> to be expanded in either do_install or do_deploy.
>>>
>>> Best I can find is:
>>>
>>> kernel-artifact-names.bbclass:KERNEL_ARTIFACT_NAME ?=
>>> "${PKGE}-${PKGV}-${PKGR}-${MACHINE}${IMAGE_VERSION_SUFFIX}"
>>>
>>> (That is then used in other places.)
>>>
>>> So the PKGV replacement is desired to be that early.
>>>
>>> I'll look into it further.
>>
>> In do_deploy, I see multiple usages of the KERNEL_ARTIFACT_NAME
>> (sometimes with
>> alternative variables.. but in the end both PKGV and PKGR are used
>> here.)
>>
>> do_deploy is after do_package/do_packagedata (where the values are
>> assigned and
>> stored).  So it should be safe for us to use the
>> read_subpackage_metadata (or
>> something new if desired) to read back in the stored PKGV (AUTOINC)
>> and PKGR
>> (PRAUTO) values.
> 
> That sounds reasonable. FWIW do_deploy doesn't have to be after
> do_package but we can arrange that in the kernel case.

Still working on the refactor, but the kernel.bbclass "do_deploy" runs after
do_packagedata, and do_packagedata is when the PKGV/PKGR dynamic items are
written.  So it's safe to do it here.

All of the other users I found were after do_package (and didn't care about PKGV
and PKGR) or were after do_packagedata.

> I'd prefer to have a more granular function to call here than
> read_subpackage_metadata, just to make it clear what is happening and
> why.

My concern is that if we don't load all of the subpackage metadata, that the
PKGV and PKGR won't be loaded -exactly- they were they were defined in
(combination of) do_package and do_packagedata.

We can limit the load to just what was written by do_packagedata, but I don't
believe this is significant less efficient or will cause issues... and doing it
this way ensures we have a single routine for future modifications.

--Mark

> Cheers,
> 
> Richard
>