diff mbox series

[1/3] kernel-yocto, devtool-source.bbclass: fix 'devtool modify' for kernels

Message ID 20231012202535.2902235-1-chris.laplante@agilent.com
State New
Headers show
Series [1/3] kernel-yocto, devtool-source.bbclass: fix 'devtool modify' for kernels | expand

Commit Message

chris.laplante@agilent.com Oct. 12, 2023, 8:25 p.m. UTC 
From: Chris Laplante <chris.laplante@agilent.com>

Fixes a couple of different issues that all conspired to break 'devtool
modify' for many use cases with kernel-yocto recipes.

To explain, we need to consider the basic flow of how 'devtool modify'
works for a recipe using kernel-yocto.bbclass:

     ┌──────────────────┐
     │do_kernel_checkout│
     ├──────────────────┘
     │ Sets up ${S}
     │ devtool_post_unpack: sets up 'devtool' branch
     ▼
     ┌────────────────────┐
     │do_validate_branches│
     ├────────────────────┘
     │ Checks out the machine branch (derived from ${KBRANCH}) and performs validation
     ▼
     ┌──────────────────┐
     │do_kernel_metadata│
     ├──────────────────┘
     │ Generates the config and patch series.
     │
     ▼
     ┌────────┐
     │do_patch│
     └────────┘
       Applies patches from patch series.

The first issue becomes clear when visualizing the flow above. The
'devtool' branch is checked out during 'do_kernel_checkout', but then
'do_validate_branches' stomps on it by checking out its machine branch.
So fix (1) is to add a postfunc to 'do_validate_branches' to checkout
the 'devtool' branch again.

Next, we need to look at the flow and consider how things work when
SRC_URI override branches are involved.

Consider:

    SRC_URI:append:fake-machine = " file://0001-my-patch.patch"
    SRC_URI:append:fake-machine-2 = " file://0001-my-patch.patch"

Assuming neither overrides are active, we'd expect a 'devtool' branch
that just points to the initial rev, then
'devtool-override-fake-machine' and 'devtool-override-fake-machine-2'
branches that each point to the same commit for 0001-my-patch.

Setting aside the matter of how the override branch set is determined,
the flow looks like this:

     ┌──────────────────┐
     │do_kernel_checkout│
     ├──────────────────┘
     │ Sets up ${S}
     │ devtool_post_unpack: sets up 'devtool' branch
     ▼
     ┌────────────────────┐
     │do_validate_branches│
     ├────────────────────┘
     │ Checks out the machine branch (derived from ${KBRANCH} and performs validation
     ▼
     ┌──────────────────┐
     │do_kernel_metadata│
     ├──────────────────┘
     │ Generates the config and patch series.
     │
     ▼
     ┌────────┐
     │do_patch│
     └────────┘
       Applies patches from patch series.
       devtool_post_patch:
                      ┌─► for each extra override... ──┐
                      │                                ▼
                      │                   create devtool-override- branch
                      │                                │
                      │                                ▼
                      │                   set OVERRIDES/FILESOVERRIDES
                      │                                │
                      │                            ┌───▼────┐
                      │                            │do_patch│
                      │                            └───┬────┘
                      │                                │
                      └────────────────────────────────┘

In the loop, we set OVERRIDES & FILESOVERRIDES such that
SRC_URI contains the correct patches for each override. But
when we call 'do_patch', it is still using the patch series file that
was generated during the call to 'do_kernel_metadata'. So the correct
patches are not applied.

The solution to this issue is to insert a call to 'do_kernel_metadata'
in between setting OVERRIDES & FILESOVERRIDES and the call to
'do_patch'. We do need to slightly tweak 'do_kernel_metadata' to be able
to clear out the previous 'fence post' files, otherwise in the example
above, the 0001-my-patch.patch would only be applied to the first
override branch that is processed.

[YOCTO #14723]

Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
---
 meta/classes-recipe/kernel-yocto.bbclass |  4 ++++
 meta/classes/devtool-source.bbclass      | 22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+)

Comments

Alexandre Belloni Oct. 18, 2023, 6:41 p.m. UTC | #1 
Hello,

The series causes:

2023-10-17 23:07:15,509 - oe-selftest - INFO - devtool.DevtoolUpgradeTests.test_devtool_modify_kernel_overrides (subunit.RemotedTestCase)
2023-10-17 23:07:15,675 - oe-selftest - INFO -  ... FAIL
Stderr:
2023-10-17 22:28:35,773 - oe-selftest - INFO - Adding: "include selftest.inc" in /home/pokybuild/yocto-worker/oe-selftest-centos/build/build-st-2667546/conf/local.conf
2023-10-17 22:28:35,773 - oe-selftest - INFO - Adding: "include bblayers.inc" in bblayers.conf
2023-10-17 23:07:15,680 - oe-selftest - INFO - 7: 10/32 116/544 (1008.52s) (0 failed) (devtool.DevtoolUpgradeTests.test_devtool_modify_kernel_overrides)
2023-10-17 23:07:15,681 - oe-selftest - INFO - testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/home/pokybuild/yocto-worker/oe-selftest-centos/build/meta/lib/oeqa/selftest/cases/devtool.py", line 2236, in test_devtool_modify_kernel_overrides
    self.assertSetEqual(set(tags), {"devtool-base", "devtool-patched"})
  File "/usr/lib64/python3.9/unittest/case.py", line 1097, in assertSetEqual
    self.fail(self._formatMessage(msg, standardMsg))
  File "/usr/lib64/python3.9/unittest/case.py", line 676, in fail
    raise self.failureException(msg)
AssertionError: Items in the second set but not the first:
'devtool-base'

https://autobuilder.yoctoproject.org/typhoon/#/builders/79/builds/5927/steps/14/logs/stdio

On 12/10/2023 16:25:33-0400, Chris Laplante via lists.openembedded.org wrote:
> From: Chris Laplante <chris.laplante@agilent.com>
> 
> Fixes a couple of different issues that all conspired to break 'devtool
> modify' for many use cases with kernel-yocto recipes.
> 
> To explain, we need to consider the basic flow of how 'devtool modify'
> works for a recipe using kernel-yocto.bbclass:
> 
>      ┌──────────────────┐
>      │do_kernel_checkout│
>      ├──────────────────┘
>      │ Sets up ${S}
>      │ devtool_post_unpack: sets up 'devtool' branch
>      ▼
>      ┌────────────────────┐
>      │do_validate_branches│
>      ├────────────────────┘
>      │ Checks out the machine branch (derived from ${KBRANCH}) and performs validation
>      ▼
>      ┌──────────────────┐
>      │do_kernel_metadata│
>      ├──────────────────┘
>      │ Generates the config and patch series.
>      │
>      ▼
>      ┌────────┐
>      │do_patch│
>      └────────┘
>        Applies patches from patch series.
> 
> The first issue becomes clear when visualizing the flow above. The
> 'devtool' branch is checked out during 'do_kernel_checkout', but then
> 'do_validate_branches' stomps on it by checking out its machine branch.
> So fix (1) is to add a postfunc to 'do_validate_branches' to checkout
> the 'devtool' branch again.
> 
> Next, we need to look at the flow and consider how things work when
> SRC_URI override branches are involved.
> 
> Consider:
> 
>     SRC_URI:append:fake-machine = " file://0001-my-patch.patch"
>     SRC_URI:append:fake-machine-2 = " file://0001-my-patch.patch"
> 
> Assuming neither overrides are active, we'd expect a 'devtool' branch
> that just points to the initial rev, then
> 'devtool-override-fake-machine' and 'devtool-override-fake-machine-2'
> branches that each point to the same commit for 0001-my-patch.
> 
> Setting aside the matter of how the override branch set is determined,
> the flow looks like this:
> 
>      ┌──────────────────┐
>      │do_kernel_checkout│
>      ├──────────────────┘
>      │ Sets up ${S}
>      │ devtool_post_unpack: sets up 'devtool' branch
>      ▼
>      ┌────────────────────┐
>      │do_validate_branches│
>      ├────────────────────┘
>      │ Checks out the machine branch (derived from ${KBRANCH} and performs validation
>      ▼
>      ┌──────────────────┐
>      │do_kernel_metadata│
>      ├──────────────────┘
>      │ Generates the config and patch series.
>      │
>      ▼
>      ┌────────┐
>      │do_patch│
>      └────────┘
>        Applies patches from patch series.
>        devtool_post_patch:
>                       ┌─► for each extra override... ──┐
>                       │                                ▼
>                       │                   create devtool-override- branch
>                       │                                │
>                       │                                ▼
>                       │                   set OVERRIDES/FILESOVERRIDES
>                       │                                │
>                       │                            ┌───▼────┐
>                       │                            │do_patch│
>                       │                            └───┬────┘
>                       │                                │
>                       └────────────────────────────────┘
> 
> In the loop, we set OVERRIDES & FILESOVERRIDES such that
> SRC_URI contains the correct patches for each override. But
> when we call 'do_patch', it is still using the patch series file that
> was generated during the call to 'do_kernel_metadata'. So the correct
> patches are not applied.
> 
> The solution to this issue is to insert a call to 'do_kernel_metadata'
> in between setting OVERRIDES & FILESOVERRIDES and the call to
> 'do_patch'. We do need to slightly tweak 'do_kernel_metadata' to be able
> to clear out the previous 'fence post' files, otherwise in the example
> above, the 0001-my-patch.patch would only be applied to the first
> override branch that is processed.
> 
> [YOCTO #14723]
> 
> Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
> ---
>  meta/classes-recipe/kernel-yocto.bbclass |  4 ++++
>  meta/classes/devtool-source.bbclass      | 22 ++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/meta/classes-recipe/kernel-yocto.bbclass b/meta/classes-recipe/kernel-yocto.bbclass
> index 4ac977b122..a5fb6e42f3 100644
> --- a/meta/classes-recipe/kernel-yocto.bbclass
> +++ b/meta/classes-recipe/kernel-yocto.bbclass
> @@ -332,6 +332,10 @@ do_patch() {
>  		if [ "${KERNEL_DEBUG_TIMESTAMPS}" != "1" ]; then
>  		    kgit_extra_args="--commit-sha author"
>  		fi
> +		if [ "${_DEVTOOL_RUNNING_DO_KERNEL_METADATA}" = "1" ]; then
> +		    # see devtool-source.bbclass for explanation
> +		    kgit-s2q --clean
> +		fi
>  		kgit-s2q --gen -v $kgit_extra_args --patches .kernel-meta/
>  		if [ $? -ne 0 ]; then
>  			bberror "Could not apply patches for ${KMACHINE}."
> diff --git a/meta/classes/devtool-source.bbclass b/meta/classes/devtool-source.bbclass
> index a02b1e9b0e..b037c5612b 100644
> --- a/meta/classes/devtool-source.bbclass
> +++ b/meta/classes/devtool-source.bbclass
> @@ -57,6 +57,7 @@ python() {
>      if is_kernel_yocto:
>          unpacktask = 'do_kernel_checkout'
>          d.appendVarFlag('do_configure', 'postfuncs', ' devtool_post_configure')
> +        d.appendVarFlag('do_validate_branches', 'postfuncs', ' devtool_post_validate_branches')
>      else:
>          unpacktask = 'do_unpack'
>      d.appendVarFlag(unpacktask, 'postfuncs', ' devtool_post_unpack')
> @@ -187,6 +188,16 @@ python devtool_post_patch() {
>          except bb.process.ExecutionError:
>              pass
>  
> +    is_kernel_yocto = bb.data.inherits_class('kernel-yocto', d)
> +    def kernel_pre_patch(localdata):
> +        if is_kernel_yocto:
> +            # Need to run do_kernel_metadata first, since it is what generates the patch series that is applied
> +            # by the do_patch task. Also, we set a variable to tell do_kernel_metadata that it needs to cleanup
> +            # the kgit-s2q.last fence post files first. Otherwise if you have two override branches that apply the
> +            # same patch, it will only get applied for the first branch.
> +            localdata.setVar('_DEVTOOL_RUNNING_DO_KERNEL_METADATA', '1')
> +            bb.build.exec_func('do_kernel_metadata', localdata)
> +
>      extra_overrides = d.getVar('DEVTOOL_EXTRA_OVERRIDES')
>      if extra_overrides:
>          extra_overrides = set(extra_overrides.split(':'))
> @@ -206,6 +217,7 @@ python devtool_post_patch() {
>              localdata = bb.data.createCopy(d)
>              localdata.setVar('OVERRIDES', ':'.join(no_overrides))
>              localdata.setVar('FILESOVERRIDES', ':'.join(no_overrides))
> +            kernel_pre_patch(localdata)
>              bb.build.exec_func('do_patch', localdata)
>              rm_patches()
>              # Now we need to reconcile the dev branch with the no-overrides one
> @@ -225,6 +237,7 @@ python devtool_post_patch() {
>                  # Run do_patch function with the override applied
>                  localdata.setVar('OVERRIDES', ':'.join(no_overrides + [override]))
>                  localdata.setVar('FILESOVERRIDES', ':'.join(no_overrides + [override]))
> +                kernel_pre_patch(localdata)
>                  bb.build.exec_func('do_patch', localdata)
>                  rm_patches()
>                  # Now we need to reconcile the new branch with the no-overrides one
> @@ -239,3 +252,12 @@ python devtool_post_configure() {
>      tempdir = d.getVar('DEVTOOL_TEMPDIR')
>      shutil.copy2(os.path.join(d.getVar('B'), '.config'), tempdir)
>  }
> +
> +python devtool_post_validate_branches() {
> +    # do_validate_branches took us off of the 'devtool' branch, so re-checkout the branch
> +    devbranch = d.getVar('DEVTOOL_DEVBRANCH')
> +    tempdir = d.getVar('DEVTOOL_TEMPDIR')
> +    with open(os.path.join(tempdir, 'srcsubdir'), 'r') as f:
> +        srcsubdir = f.read()
> +    bb.process.run(f'git checkout {devbranch}', cwd=srcsubdir)
> +}
> -- 
> 2.34.1
> 

> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#189023): https://lists.openembedded.org/g/openembedded-core/message/189023
> Mute This Topic: https://lists.openembedded.org/mt/101926734/3617179
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alexandre.belloni@bootlin.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
chris.laplante@agilent.com Oct. 18, 2023, 6:44 p.m. UTC | #2 
> Hello,
> 
> The series causes:
> 
> 2023-10-17 23:07:15,509 - oe-selftest - INFO -
> devtool.DevtoolUpgradeTests.test_devtool_modify_kernel_overrides
> (subunit.RemotedTestCase)
> 2023-10-17 23:07:15,675 - oe-selftest - INFO -  ... FAIL
> Stderr:
> 2023-10-17 22:28:35,773 - oe-selftest - INFO - Adding: "include selftest.inc" in
> /home/pokybuild/yocto-worker/oe-selftest-centos/build/build-st-
> 2667546/conf/local.conf
> 2023-10-17 22:28:35,773 - oe-selftest - INFO - Adding: "include bblayers.inc" in
> bblayers.conf
> 2023-10-17 23:07:15,680 - oe-selftest - INFO - 7: 10/32 116/544 (1008.52s) (0
> failed) (devtool.DevtoolUpgradeTests.test_devtool_modify_kernel_overrides)
> 2023-10-17 23:07:15,681 - oe-selftest - INFO -
> testtools.testresult.real._StringException: Traceback (most recent call last):
>   File "/home/pokybuild/yocto-worker/oe-selftest-
> centos/build/meta/lib/oeqa/selftest/cases/devtool.py", line 2236, in
> test_devtool_modify_kernel_overrides
>     self.assertSetEqual(set(tags), {"devtool-base", "devtool-patched"})
>   File "/usr/lib64/python3.9/unittest/case.py", line 1097, in assertSetEqual
>     self.fail(self._formatMessage(msg, standardMsg))
>   File "/usr/lib64/python3.9/unittest/case.py", line 676, in fail
>     raise self.failureException(msg)
> AssertionError: Items in the second set but not the first:
> 'devtool-base'
> 
> https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fautobuil
> der.yoctoproject.org%2Ftyphoon%2F%23%2Fbuilders%2F79%2Fbuilds%2F5927%
> 2Fsteps%2F14%2Flogs%2Fstdio&data=05%7C01%7Cchris.laplante%40agilent.co
> m%7Ce69a6029f8c8428f140208dbd009d811%7Ca9c0bc098b46420693512ba12
> fb4a5c0%7C0%7C0%7C638332512937141361%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C3000%7C%7C%7C&sdata=ptHcRf3P8%2Fg6G%2BgA2ZXR6SSVuKJcZGORUw
> UzbYoJ70E%3D&reserved=0

I'll take a look, thanks. This series is not ready for other reasons besides this though :( 

Thanks,
Chris
chris.laplante@agilent.com Oct. 19, 2023, 4:29 p.m. UTC | #3 
Hi Alexandre,

> The series causes:
> 
> 2023-10-17 23:07:15,509 - oe-selftest - INFO -
> devtool.DevtoolUpgradeTests.test_devtool_modify_kernel_overrides
> (subunit.RemotedTestCase)
> 2023-10-17 23:07:15,675 - oe-selftest - INFO -  ... FAIL
> Stderr:
> 2023-10-17 22:28:35,773 - oe-selftest - INFO - Adding: "include selftest.inc" in
> /home/pokybuild/yocto-worker/oe-selftest-centos/build/build-st-
> 2667546/conf/local.conf
> 2023-10-17 22:28:35,773 - oe-selftest - INFO - Adding: "include bblayers.inc" in
> bblayers.conf
> 2023-10-17 23:07:15,680 - oe-selftest - INFO - 7: 10/32 116/544 (1008.52s) (0
> failed) (devtool.DevtoolUpgradeTests.test_devtool_modify_kernel_overrides)
> 2023-10-17 23:07:15,681 - oe-selftest - INFO -
> testtools.testresult.real._StringException: Traceback (most recent call last):
>   File "/home/pokybuild/yocto-worker/oe-selftest-
> centos/build/meta/lib/oeqa/selftest/cases/devtool.py", line 2236, in
> test_devtool_modify_kernel_overrides
>     self.assertSetEqual(set(tags), {"devtool-base", "devtool-patched"})
>   File "/usr/lib64/python3.9/unittest/case.py", line 1097, in assertSetEqual
>     self.fail(self._formatMessage(msg, standardMsg))
>   File "/usr/lib64/python3.9/unittest/case.py", line 676, in fail
>     raise self.failureException(msg)
> AssertionError: Items in the second set but not the first:
> 'devtool-base'
> 
> https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fautobu
> ilder.yoctoproject.org%2Ftyphoon%2F%23%2Fbuilders%2F79%2Fbuilds%2F592
> 7%2Fsteps%2F14%2Flogs%2Fstdio&data=05%7C01%7Cchris.laplante%40agilen
> t.com%7Ce69a6029f8c8428f140208dbd009d811%7Ca9c0bc098b46420693512b
> a12fb4a5c0%7C0%7C0%7C638332512937141361%7CUnknown%7CTWFpbGZsb
> 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%
> 3D%7C3000%7C%7C%7C&sdata=ptHcRf3P8%2Fg6G%2BgA2ZXR6SSVuKJcZGOR
> UwUzbYoJ70E%3D&reserved=0


I'm unable to reproduce this failure locally. Is there any way to grab the temp directories or otherwise access the filesystem on the Autobuilder to see what's going on? I suspect the answer is 'no', but just thought I'd ask.

Thanks,
Chris
diff mbox series

Patch

diff --git a/meta/classes-recipe/kernel-yocto.bbclass b/meta/classes-recipe/kernel-yocto.bbclass
index 4ac977b122..a5fb6e42f3 100644
--- a/meta/classes-recipe/kernel-yocto.bbclass
+++ b/meta/classes-recipe/kernel-yocto.bbclass
@@ -332,6 +332,10 @@  do_patch() {
 		if [ "${KERNEL_DEBUG_TIMESTAMPS}" != "1" ]; then
 		    kgit_extra_args="--commit-sha author"
 		fi
+		if [ "${_DEVTOOL_RUNNING_DO_KERNEL_METADATA}" = "1" ]; then
+		    # see devtool-source.bbclass for explanation
+		    kgit-s2q --clean
+		fi
 		kgit-s2q --gen -v $kgit_extra_args --patches .kernel-meta/
 		if [ $? -ne 0 ]; then
 			bberror "Could not apply patches for ${KMACHINE}."
diff --git a/meta/classes/devtool-source.bbclass b/meta/classes/devtool-source.bbclass
index a02b1e9b0e..b037c5612b 100644
--- a/meta/classes/devtool-source.bbclass
+++ b/meta/classes/devtool-source.bbclass
@@ -57,6 +57,7 @@  python() {
     if is_kernel_yocto:
         unpacktask = 'do_kernel_checkout'
         d.appendVarFlag('do_configure', 'postfuncs', ' devtool_post_configure')
+        d.appendVarFlag('do_validate_branches', 'postfuncs', ' devtool_post_validate_branches')
     else:
         unpacktask = 'do_unpack'
     d.appendVarFlag(unpacktask, 'postfuncs', ' devtool_post_unpack')
@@ -187,6 +188,16 @@  python devtool_post_patch() {
         except bb.process.ExecutionError:
             pass
 
+    is_kernel_yocto = bb.data.inherits_class('kernel-yocto', d)
+    def kernel_pre_patch(localdata):
+        if is_kernel_yocto:
+            # Need to run do_kernel_metadata first, since it is what generates the patch series that is applied
+            # by the do_patch task. Also, we set a variable to tell do_kernel_metadata that it needs to cleanup
+            # the kgit-s2q.last fence post files first. Otherwise if you have two override branches that apply the
+            # same patch, it will only get applied for the first branch.
+            localdata.setVar('_DEVTOOL_RUNNING_DO_KERNEL_METADATA', '1')
+            bb.build.exec_func('do_kernel_metadata', localdata)
+
     extra_overrides = d.getVar('DEVTOOL_EXTRA_OVERRIDES')
     if extra_overrides:
         extra_overrides = set(extra_overrides.split(':'))
@@ -206,6 +217,7 @@  python devtool_post_patch() {
             localdata = bb.data.createCopy(d)
             localdata.setVar('OVERRIDES', ':'.join(no_overrides))
             localdata.setVar('FILESOVERRIDES', ':'.join(no_overrides))
+            kernel_pre_patch(localdata)
             bb.build.exec_func('do_patch', localdata)
             rm_patches()
             # Now we need to reconcile the dev branch with the no-overrides one
@@ -225,6 +237,7 @@  python devtool_post_patch() {
                 # Run do_patch function with the override applied
                 localdata.setVar('OVERRIDES', ':'.join(no_overrides + [override]))
                 localdata.setVar('FILESOVERRIDES', ':'.join(no_overrides + [override]))
+                kernel_pre_patch(localdata)
                 bb.build.exec_func('do_patch', localdata)
                 rm_patches()
                 # Now we need to reconcile the new branch with the no-overrides one
@@ -239,3 +252,12 @@  python devtool_post_configure() {
     tempdir = d.getVar('DEVTOOL_TEMPDIR')
     shutil.copy2(os.path.join(d.getVar('B'), '.config'), tempdir)
 }
+
+python devtool_post_validate_branches() {
+    # do_validate_branches took us off of the 'devtool' branch, so re-checkout the branch
+    devbranch = d.getVar('DEVTOOL_DEVBRANCH')
+    tempdir = d.getVar('DEVTOOL_TEMPDIR')
+    with open(os.path.join(tempdir, 'srcsubdir'), 'r') as f:
+        srcsubdir = f.read()
+    bb.process.run(f'git checkout {devbranch}', cwd=srcsubdir)
+}