Bug 11586

Summary: scripts/yocto-bsp linux-yocto-custom.bb template faulty?
Product: [Build System, Metadata & Runtime] Meta-yocto Reporter: Robert Berger <pokylinux>
Component: meta-yoctoAssignee: Leonardo Sandoval Gonzalez <leonardo.sandoval.gonzalez>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: Medium CC: bruce.ashfield, poky.bs.watcher, poky.watcher
Version: 2.3   
Target Milestone: 2.4 M2   
Hardware: x86   
OS: Multiple   
Whiteboard:
OS type for building Yocto: --- Type of Regression: ---
Verified: Documentation change: Don't know

Description Robert Berger 2017-05-27 17:45:23 UTC
By default if you create a BSP the kernel template looks like this[1]

...
SRC_URI += "file://{{=machine}}.scc \
            file://{{=machine}}.cfg \
            file://{{=machine}}-user-config.cfg \
            file://{{=machine}}-user-patches.scc \
           "
...

The problem IMHO is, that we have double inclusing of e.g. patches, which makes bitbake apply the same patch twize, which does not work.

I suggest something like this:

SRC_URI += "file://{{=machine}}.scc \
             file://{{=machine}}.cfg \
-            file://{{=machine}}-user-config.cfg \
-            file://{{=machine}}-user-patches.scc \
            "
+# {{=machine}}-user-config.cfg and
+# {{=machine}}-user-patches.scc are
+# included by {{=machine}}.scc
+# removed from SRC_URI to avoid double inclusion
+# and like this avoiding applying same patch twice

[1] http://git.yoctoproject.org/cgit/cgit.cgi/poky/tree/scripts/lib/bsp/substrate/target/arch/common/recipes-kernel/linux/linux-yocto-custom.bb?h=pyro
Comment 1 Leonardo Sandoval Gonzalez 2017-06-12 16:46:36 UTC
Actually a I just sent a patch which may cause trouble for all archs [1]. Adding Bruce, he can provide better directions.
Comment 2 Leonardo Sandoval Gonzalez 2017-06-12 16:49:17 UTC
(In reply to comment #1)
> Actually a I just sent a patch which may cause trouble for all archs [1].
> Adding Bruce, he can provide better directions.

Forgot to include the reference:

[1] https://lists.yoctoproject.org/pipermail/poky/2017-June/011020.html
Comment 3 Bruce Ashfield 2017-06-13 07:19:28 UTC
Let me ask a couple of potentially dumb questions .. and make a couple of obvious statements. Since I'm not 100% clear on the issue.

Whatever is on the SRC_URI will be expanded and processed, whether it is configs or patches.

So if any of the .scc files that we are adding to the bbappend have patches, and then include other .scc files with those same patches, we will have the double application of patches and failure.

If we are creating a new BSP that inherits the configuration of an existing kernel type, then the "include with config only should be used", otherwise, the .scc files will be expanded, patches found and re-applied.

i.e. "include ktypes/standard/standard.scc nopatch" would be in the bbappend to inherit all of the standard configurations, but not the patches (since all of ktypes standard is already on standard/base, you'll fail to re-apply the patches).

So I don't see the recent patch as making things any worse, but we still probably don't have the optimal set of .scc files in the bbappends. They were initially written when the tools were far more complex and analyzed the entire branch to see if patches were already applied and skipped them if they were .. while that was nice, it was error prone and lead to some surprise failures. We are now "if it is on the SRC_URI, do what it says and add the patches to the patch queue".
Comment 4 Robert Berger 2017-06-13 08:26:09 UTC
Just to elaborate a bit more how I came up with the bug:

1) I use the bsp tool and create with it a BSP for a custom (mainline) kernel

2) The bsp tool spits out a kernel recipe based on this template:

...
SRC_URI += "file://{{=machine}}.scc \
            file://{{=machine}}.cfg \
            file://{{=machine}}-user-config.cfg \
            file://{{=machine}}-user-patches.scc \
           "
...

3) I use the yocto-kernel tool to add patches

yocto-kernel patch add <BSP name> <patch>

The yocto-kernel tool adds the patch to {{=machine}}-user-patches.scc.

The problem is, that {{=machine}}-user-config.cfg and {{=machine}}-user-patches.scc are included by {{=machine}}.scc hence we have double inclusion and obviously the second time the patch is being applied it fails.

Apparently doing all this manually without the bsp tool and the yocto-kernel
tool things can be fixed and also one can manually (or with a patch) fix the stuff. 

If you need further info I'm glad to provide it.
Comment 5 Bruce Ashfield 2017-06-13 12:53:31 UTC
(In reply to comment #4)
> Just to elaborate a bit more how I came up with the bug:
> 
> 1) I use the bsp tool and create with it a BSP for a custom (mainline) kernel
> 
> 2) The bsp tool spits out a kernel recipe based on this template:
> 
> ...
> SRC_URI += "file://{{=machine}}.scc \
>             file://{{=machine}}.cfg \
>             file://{{=machine}}-user-config.cfg \
>             file://{{=machine}}-user-patches.scc \
>            "
> ...
> 
> 3) I use the yocto-kernel tool to add patches
> 
> yocto-kernel patch add <BSP name> <patch>
> 
> The yocto-kernel tool adds the patch to {{=machine}}-user-patches.scc.
> 
> The problem is, that {{=machine}}-user-config.cfg and
> {{=machine}}-user-patches.scc are included by {{=machine}}.scc hence we have
> double inclusion and obviously the second time the patch is being applied it
> fails.
> 
> Apparently doing all this manually without the bsp tool and the yocto-kernel
> tool things can be fixed and also one can manually (or with a patch) fix the
> stuff. 
> 
> If you need further info I'm glad to provide it.

definitely a valid use case, and it maps to what I was explaining. In the previous incantations, the tools jumped through hoops to validate patches and would have known how to skip the double include.

But now, they will simply do as they are told.

So the default machine.scc shouldn't include user-patches.scc if it is already on the SRC_URI .. it should just leave it up to the file on the SRC_URI. Or vice versa, if it includes the user-patches.scc, we should remove it from the SRC_URI.
Comment 6 Leonardo Sandoval Gonzalez 2017-06-13 17:35:00 UTC
Confirmed. Do(In reply to comment #5)
> (In reply to comment #4)
> > Just to elaborate a bit more how I came up with the bug:
> > 
> > 1) I use the bsp tool and create with it a BSP for a custom (mainline) kernel
> > 
> > 2) The bsp tool spits out a kernel recipe based on this template:
> > 
> > ...
> > SRC_URI += "file://{{=machine}}.scc \
> >             file://{{=machine}}.cfg \
> >             file://{{=machine}}-user-config.cfg \
> >             file://{{=machine}}-user-patches.scc \
> >            "
> > ...
> > 
> > 3) I use the yocto-kernel tool to add patches
> > 
> > yocto-kernel patch add <BSP name> <patch>
> > 
> > The yocto-kernel tool adds the patch to {{=machine}}-user-patches.scc.
> > 
> > The problem is, that {{=machine}}-user-config.cfg and
> > {{=machine}}-user-patches.scc are included by {{=machine}}.scc hence we have
> > double inclusion and obviously the second time the patch is being applied it
> > fails.
> > 
> > Apparently doing all this manually without the bsp tool and the yocto-kernel
> > tool things can be fixed and also one can manually (or with a patch) fix the
> > stuff. 
> > 
> > If you need further info I'm glad to provide it.
> 
> definitely a valid use case, and it maps to what I was explaining. In the
> previous incantations, the tools jumped through hoops to validate patches
> and would have known how to skip the double include.
> 
> But now, they will simply do as they are told.
> 
> So the default machine.scc shouldn't include user-patches.scc if it is
> already on the SRC_URI .. it should just leave it up to the file on the
> SRC_URI. Or vice versa, if it includes the user-patches.scc, we should
> remove it from the SRC_URI.

So the series [1], which is now merged at master, will have issues due to the reason posted on this bug. I believe this problem (the double inclusion) applies to patches and cfg scripts, right?

Will send a patch to fix this bug (the custom kernel) and the rest of the architectures.
Comment 7 Leonardo Sandoval Gonzalez 2017-06-13 19:50:02 UTC
Proposed patch sent to 

https://lists.yoctoproject.org/pipermail/poky/2017-June/011026.html

Thanks Bruce for your info and Robert for noticing it!
Comment 8 Bruce Ashfield 2017-06-13 22:17:28 UTC
I've acked the patch on the list, it looks good to me!
Comment 9 Leonardo Sandoval Gonzalez 2017-07-07 17:24:44 UTC
Merged at poky repo:

(From meta-yocto rev: 86ab7ab6688f250bb5777371d2cbc28c770847d4)