diff mbox series

[v2] kernel.bbclass: hoist up "unset S" bbfatal from kernel-yocto.bbclass to kernel.bbclass

Message ID 20230626135024.34405-1-luca.ceresoli@bootlin.com
State New
Headers show
Series [v2] kernel.bbclass: hoist up "unset S" bbfatal from kernel-yocto.bbclass to kernel.bbclass | expand

Commit Message

Luca Ceresoli June 26, 2023, 1:50 p.m. UTC 
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 be moved into work-shared, which results in an empty
work-shared/.../kernel-source directory.

Setting S in recipes is usually not required when downloading a tarball, so
it 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. Move it to
kernel.bbclass so that even 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.

As an additional benefit, the check is now taking place both when
downloading a tarball and when downloading from git, so even when using git
the recipe writer will be presented the explanatory error message.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v2:
 - don't check for WORKDIR/git dir existence (Suggested by Richard)
---
 meta/classes-recipe/kernel-yocto.bbclass | 8 --------
 meta/classes-recipe/kernel.bbclass       | 6 ++++++
 2 files changed, 6 insertions(+), 8 deletions(-)

Comments

Bruce Ashfield June 26, 2023, 10:41 p.m. UTC | #1 
In message: [PATCH v2] kernel.bbclass: hoist up "unset S" bbfatal from kernel-yocto.bbclass to kernel.bbclass
on 26/06/2023 luca.ceresoli@bootlin.com 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 be moved into work-shared, which results in an empty
> work-shared/.../kernel-source directory.
> 
> Setting S in recipes is usually not required when downloading a tarball, so
> it 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. Move it to
> kernel.bbclass so that even 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.

That's the current task flow, but the checkout has moved in the past,
and could move again.

My "don't fix it if it isn't broken" inclination tells me to just
as for the new check to be added for the use case, and leave the
one in kernel-yocto as a backup, in case the task ordering changes
or some other strange path through the code appears later.

But either way, the change looks ok to me.

Bruce

> 
> As an additional benefit, the check is now taking place both when
> downloading a tarball and when downloading from git, so even when using git
> the recipe writer will be presented the explanatory error message.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Changed in v2:
>  - don't check for WORKDIR/git dir existence (Suggested by Richard)
> ---
>  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 4ac977b12207..3f2ce17aeb88 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 e82b696d1a14..75f43cb1134e 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)
> +
> +    # Setting S is required with this class when fetching a tarball because
> +    # we cannot figure out automatically the extracted directory name. The
> +    # check is beneficial even when using git so don't check for git here.
> +    if 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
> -- 
> 2.34.1
>
Alexandre Belloni June 30, 2023, 3:06 p.m. UTC | #2 
Hello,

This caused the following failures on the AB:

stdio: ERROR: linux-yocto-6.1.35+gitAUTOINC+b358c237cf_915f4d2237-r0 do_symlink_kernsrc: S is not set to the linux source directory. Check the recipe and set S to the proper extracted subdirectory.
stdio: ERROR: Logfile of failure stored in: /home/pokybuild/yocto-worker/qemuarm/build/build/tmp/work/qemuarm-poky-linux-gnueabi/linux-yocto/6.1.35+gitAUTOINC+b358c237cf_915f4d2237-r0/temp/log.do_symlink_kernsrc.3914759
stdio: ERROR: Task (/home/pokybuild/yocto-worker/qemuarm/build/meta/recipes-kernel/linux/linux-yocto_6.1.bb:do_symlink_kernsrc) failed with exit code '1'
stdio: ERROR: Command . ./oe-init-build-env; bitbake core-image-sato core-image-sato-sdk core-image-minimal core-image-minimal-dev core-image-sato:do_populate_sdk -k failed with exit code 1, see errors above. (1687981572.9: 11986.3)

On 26/06/2023 15:50:24+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 be moved into work-shared, which results in an empty
> work-shared/.../kernel-source directory.
> 
> Setting S in recipes is usually not required when downloading a tarball, so
> it 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. Move it to
> kernel.bbclass so that even 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.
> 
> As an additional benefit, the check is now taking place both when
> downloading a tarball and when downloading from git, so even when using git
> the recipe writer will be presented the explanatory error message.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Changed in v2:
>  - don't check for WORKDIR/git dir existence (Suggested by Richard)
> ---
>  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 4ac977b12207..3f2ce17aeb88 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 e82b696d1a14..75f43cb1134e 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)
> +
> +    # Setting S is required with this class when fetching a tarball because
> +    # we cannot figure out automatically the extracted directory name. The
> +    # check is beneficial even when using git so don't check for git here.
> +    if 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
> -- 
> 2.34.1
> 

> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#183420): https://lists.openembedded.org/g/openembedded-core/message/183420
> Mute This Topic: https://lists.openembedded.org/mt/99787873/3617179
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alexandre.belloni@bootlin.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Luca Ceresoli July 4, 2023, 8:58 p.m. UTC | #3 
Hello Richard, Bruce,

On Fri, 30 Jun 2023 17:06:51 +0200
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> Hello,
> 
> This caused the following failures on the AB:
> 
> stdio: ERROR: linux-yocto-6.1.35+gitAUTOINC+b358c237cf_915f4d2237-r0 do_symlink_kernsrc: S is not set to the linux source directory. Check the recipe and set S to the proper extracted subdirectory.
> stdio: ERROR: Logfile of failure stored in: /home/pokybuild/yocto-worker/qemuarm/build/build/tmp/work/qemuarm-poky-linux-gnueabi/linux-yocto/6.1.35+gitAUTOINC+b358c237cf_915f4d2237-r0/temp/log.do_symlink_kernsrc.3914759
> stdio: ERROR: Task (/home/pokybuild/yocto-worker/qemuarm/build/meta/recipes-kernel/linux/linux-yocto_6.1.bb:do_symlink_kernsrc) failed with exit code '1'
> stdio: ERROR: Command . ./oe-init-build-env; bitbake core-image-sato core-image-sato-sdk core-image-minimal core-image-minimal-dev core-image-sato:do_populate_sdk -k failed with exit code 1, see errors above. (1687981572.9: 11986.3)

v2, where I removed the "not git" condition as discussed[0], fails
because the check is *before* do_kernel_checkout, so the move/clone has
not been done yet. And cannot be done before in the git case as it
could be a bare repo thus no Makefile exists yet anywhere.

I think there are two options:

 1. Move the check *after* do_kernel_checkout, when the source code
    must be there
 2. Revert to the v1 approach, which checks only in the tarball case as
    the current code does

I think it is better to check sooner rather than later (before git init
+ git add in the tar case). Not only because the user would be warned
as soon as reasonably possible, but also because an error might cause
a error path in do_kernel_checkout anyway.

So unless there are better ideas I would get back to initial approach,
possibly with a different patch to take into account the comments from
both of you.

[0]
https://lore.kernel.org/openembedded-core/674d0a964611cc1e5451318976f17a4a24cbca5a.camel@linuxfoundation.org/#t

Luca
diff mbox series

Patch

diff --git a/meta/classes-recipe/kernel-yocto.bbclass b/meta/classes-recipe/kernel-yocto.bbclass
index 4ac977b12207..3f2ce17aeb88 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 e82b696d1a14..75f43cb1134e 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)
+
+    # Setting S is required with this class when fetching a tarball because
+    # we cannot figure out automatically the extracted directory name. The
+    # check is beneficial even when using git so don't check for git here.
+    if 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