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

["Richard Purdie" <richard.purdie@linuxfoundation.org>]

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.

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.

Cheers,

Richard