Re: [OE-core] [PATCH] kernel.bbclass: hoist up "unset S" bbfatal from kernel-yocto.bbclass to kernel.bbclass

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Richard,

thanks for reviewing!

On Fri, 23 Jun 2023 12:07:43 +0100
"Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:

> Hi Luca,
> 
> On Mon, 2023-06-05 at 16:13 +0200, Luca Ceresoli via lists.openembedded.org wrote:
> > From: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > 
> > Writing a simple recipe that inherits kernel.bbclass and downloads a kernel
> > tarball (e.g. a mainline release from kernel.org) via http or ftp fails
> > with either:
> > 
> >   ERROR: linux-acme-6.3.3-r0 do_configure: oe_runmake failed
> >   ...
> >   | make: *** No rule to make target 'oldnoconfig'.  Stop.
> > 
> > or (seen on a different setup, based on kirkstone):
> > 
> >   ... do_populate_lic: QA Issue: ... LIC_FILES_CHKSUM points to an invalid file: .../work-shared/.../kernel-source/COPYING [license-checksum]
> > 
> > This happens when not setting S in the recipe. In this case, kernel.bbclass
> > sets it to ${STAGING_KERNEL_DIR}
> > (${TMPDIR}/work-shared/${MACHINE}/kernel-source).  This means that in
> > do_symlink_kernsrc(), the 'if s != kernsrc' never triggers and thus the
> > kernel tree will not me moved into work/shared, which results in an empty
> > work-shared/.../kernel-source directory.
> > 
> > When downloading a tarball it is usually not required to set S in recipes,
> > so this is not obvious here and the error message does not point to the
> > problem or its solution.
> > 
> > There is such a check in kernel-yocto.bbclass though, so move it to
> > kernel.bbclass so that also kernel recipes not based on kernel-yocto can
> > benefit from it.
> > 
> > The check is moved:
> > 
> >  - from the beginning of do_kernel_checkout() in kernel-yocto
> >  - to the end of do_symlink_kernsrc() in kernel.bbclass
> > 
> > and since do_kernel_checkout is executed 'after do_symlink_kernsrc', the
> > code flow does not change in a relevant way when using linux-yocto.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > ---
> >  meta/classes-recipe/kernel-yocto.bbclass | 8 --------
> >  meta/classes-recipe/kernel.bbclass       | 6 ++++++
> >  2 files changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/meta/classes-recipe/kernel-yocto.bbclass b/meta/classes-recipe/kernel-yocto.bbclass
> > index 108b7e675212..ceb451b69969 100644
> > --- a/meta/classes-recipe/kernel-yocto.bbclass
> > +++ b/meta/classes-recipe/kernel-yocto.bbclass
> > @@ -394,16 +394,8 @@ do_kernel_checkout() {
> >  		# case: we have no git repository at all. 
> >  		# To support low bandwidth options for building the kernel, we'll just 
> >  		# convert the tree to a git repo and let the rest of the process work unchanged
> > -		
> > -		# if ${S} hasn't been set to the proper subdirectory a default of "linux" is 
> > -		# used, but we can't initialize that empty directory. So check it and throw a
> > -		# clear error
> >  
> >  	        cd ${S}
> > -		if [ ! -f "Makefile" ]; then
> > -			bberror "S is not set to the linux source directory. Check "
> > -			bbfatal "the recipe and set S to the proper extracted subdirectory"
> > -		fi
> >  		rm -f .gitignore
> >  		git init
> >  		check_git_config
> > diff --git a/meta/classes-recipe/kernel.bbclass b/meta/classes-recipe/kernel.bbclass
> > index 9c8036f4df01..5ed4a2e03c72 100644
> > --- a/meta/classes-recipe/kernel.bbclass
> > +++ b/meta/classes-recipe/kernel.bbclass
> > @@ -195,6 +195,12 @@ python do_symlink_kernsrc () {
> >              import shutil
> >              shutil.move(s, kernsrc)
> >              os.symlink(kernsrc, s)
> > +
> > +    # When not using git, we cannot figure out automatically the extracted
> > +    # directory. So check it and throw a clear error.
> > +    if not os.path.isdir(os.path.join(d.getVar("WORKDIR"), "git")) and \
> > +       not os.path.exists(os.path.join(s, "Makefile")):
> > +        bb.fatal("S is not set to the linux source directory. Check the recipe and set S to the proper extracted subdirectory.")
> >  }
> >  # do_patch is normally ordered before do_configure, but
> >  # externalsrc.bbclass deletes do_patch, breaking the dependency of  
> 
> Sorry this has taken a bit of time to get to.

No problem.

> We can't check for "git" as the workdir directory in this test as it
> can be overridden in the fetcher. We'll need to find some other better
> approach. Can't we just use ${S}/Makefile ?

I'm not sure I got your point, however I agree. :-)

I'm not sure I got your point because in my patch I did try to mimick as
far as possible the same logic as the original code, which is:

        if [ -d "${WORKDIR}/git/" ]; then
                # case: git repository

		...
        else
                # case: we have no git repository at all. 

		...

		### This is the check I have moved to kernel.bbclass ###
                if [ ! -f "Makefile" ]; then
                        bberror "S is not set to the linux source directory. Check "
                        bbfatal "the recipe and set S to the proper extracted subdirectory"
                fi
		###

		...
	fi

So the bberror/bbfatal I moved is de facto inside a (if ! -d
${WORKDIR}/git && ! -f ${S}/Makefile) check, which is what I replicated
in kernel.bbclass.

On the other hand I agree that we could only check for ${S}/Makefile,
which is what I did in a draft version, and has the small side effect
of printing  the explanatory error message _also_ in the git case,
instead of a long-lined log concluded by "No rule to make target
'oldnoconfig'. Stop.".

So it looks like I'll be sending a v2 with simply the ${WORKDIR}/git
check removed.

> Please also copy Bruce on these changes going forward.

Sure, I will.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com