Patchwork [RFC,1/3] insane: Split do_package_qa into a separate task (from do_package)

login
register
mail settings
Submitter Richard Purdie
Date July 9, 2014, 8:15 p.m.
Message ID <1404936934.15985.48.camel@ted>
Download mbox | patch
Permalink /patch/75287/
State New
Headers show

Comments

Richard Purdie - July 9, 2014, 8:15 p.m.
Its possible to run the package QA checks as a separate task rather than
as part of the do_package task. This offers more parallelism but the
fact that made me propose this is that ideally we'd like to access
pkgdata to help add new tests and to do that, we need to run later in
the task list. We also need to add in RDEPENDS to the task which apply
to do_package_write_* but not do_package. See the subsequent patches
for why this is desireable.

If we split into a separate task, we need to add in calls to read
the sub package data, build the cache structure used by do_package and
cover the task with sstate (which is empty and just acts as a stamp
saying it passed package QA). We also need to handle our own
dependencies.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Otavio Salvador - July 11, 2014, 2:43 a.m.
Hello Richard,

(added meta-freescale mailing list as it is related to meta-fsl-arm)

On Wed, Jul 9, 2014 at 5:15 PM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> Its possible to run the package QA checks as a separate task rather than
> as part of the do_package task. This offers more parallelism but the
> fact that made me propose this is that ideally we'd like to access
> pkgdata to help add new tests and to do that, we need to run later in
> the task list. We also need to add in RDEPENDS to the task which apply
> to do_package_write_* but not do_package. See the subsequent patches
> for why this is desireable.
>
> If we split into a separate task, we need to add in calls to read
> the sub package data, build the cache structure used by do_package and
> cover the task with sstate (which is empty and just acts as a stamp
> saying it passed package QA). We also need to handle our own
> dependencies.
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>

When I see a RFC serie I expect people to wait some days /for
comments/. I learned today those are already merged and /no comment
time/ has been given.

The problem I found is that running the QA checks in another task
seems to break some use-cases.

In the libfslcodec (it is a binary blob provided by Freescale for
multimedia) we have:

...
python populate_packages_prepend() {
    ...
    # FIXME: All binaries lack GNU_HASH in elf binary but as we don't have
    # the source we cannot fix it. Disable the insane check for now.
    for p in d.getVar('PACKAGES', True).split():
        d.setVar("DEBIAN_NOAUTONAME_%s" % p, "1")

        if p == 'libfslcodec-test-bin':
            # FIXME: includes the DUT .so files so we need to deploy those
            d.appendVar("INSANE_SKIP_%s" % p, "ldflags textrel libdir")
        else:
            d.appendVar("INSANE_SKIP_%s" % p, "ldflags textrel")
    ...
}

In this case the INSANE_SKIP var is not being set on time. If we check
the value it is being set but it seems to be too late.

How we can address it?
Richard Purdie - July 11, 2014, 8:27 a.m.
On Thu, 2014-07-10 at 23:43 -0300, Otavio Salvador wrote:
> On Wed, Jul 9, 2014 at 5:15 PM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > Its possible to run the package QA checks as a separate task rather than
> > as part of the do_package task. This offers more parallelism but the
> > fact that made me propose this is that ideally we'd like to access
> > pkgdata to help add new tests and to do that, we need to run later in
> > the task list. We also need to add in RDEPENDS to the task which apply
> > to do_package_write_* but not do_package. See the subsequent patches
> > for why this is desireable.
> >
> > If we split into a separate task, we need to add in calls to read
> > the sub package data, build the cache structure used by do_package and
> > cover the task with sstate (which is empty and just acts as a stamp
> > saying it passed package QA). We also need to handle our own
> > dependencies.
> >
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>

I've just seen this email. I did see the failure from the overnight
build and already sent a reply suggesting how this can be fixed, it
should be a simple change.

> When I see a RFC serie I expect people to wait some days /for
> comments/. I learned today those are already merged and /no comment
> time/ has been given.

The whole RFC was not merged. The previous RFC was and when that
happened, to avoid more rebuilds, I chose to merge some parts of the
second one since there appeared low risk.

I send out a lot of patches, in general I get zero replies to most of
them so its hard to know when I've waited "long enough". I did seek out
and talk to some people about those RFCs and the feedback was positive,
we all agree that the improvements to the dependency checking outweigh
any downsides, including to be this issue.

> The problem I found is that running the QA checks in another task
> seems to break some use-cases.

I think in this case the usage is rather horrid and not something I'd
say we've ever suggested or supported.

> In the libfslcodec (it is a binary blob provided by Freescale for
> multimedia) we have:
> 
> ...
> python populate_packages_prepend() {
>     ...
>     # FIXME: All binaries lack GNU_HASH in elf binary but as we don't have
>     # the source we cannot fix it. Disable the insane check for now.
>     for p in d.getVar('PACKAGES', True).split():
>         d.setVar("DEBIAN_NOAUTONAME_%s" % p, "1")
> 
>         if p == 'libfslcodec-test-bin':
>             # FIXME: includes the DUT .so files so we need to deploy those
>             d.appendVar("INSANE_SKIP_%s" % p, "ldflags textrel libdir")
>         else:
>             d.appendVar("INSANE_SKIP_%s" % p, "ldflags textrel")
>     ...
> }
> 
> In this case the INSANE_SKIP var is not being set on time. If we check
> the value it is being set but it seems to be too late.
> 
> How we can address it?

Should be as simple as s/populate_packages_prepend//. The
DEBIAN_NOAUTONAME doesn't really fit the description of the code there
FWIW, I know why you're doing it but its not what the comment says. 

I would not recommend setting variables in function prepends like this,
its ugly and error prone, as you've just found out.

Cheers,

Richard
Otavio Salvador - July 11, 2014, 12:14 p.m.
Hello Richard,

On Fri, Jul 11, 2014 at 5:27 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> On Thu, 2014-07-10 at 23:43 -0300, Otavio Salvador wrote:
>> On Wed, Jul 9, 2014 at 5:15 PM, Richard Purdie
>> <richard.purdie@linuxfoundation.org> wrote:
>> > Its possible to run the package QA checks as a separate task rather than
>> > as part of the do_package task. This offers more parallelism but the
>> > fact that made me propose this is that ideally we'd like to access
>> > pkgdata to help add new tests and to do that, we need to run later in
>> > the task list. We also need to add in RDEPENDS to the task which apply
>> > to do_package_write_* but not do_package. See the subsequent patches
>> > for why this is desireable.
>> >
>> > If we split into a separate task, we need to add in calls to read
>> > the sub package data, build the cache structure used by do_package and
>> > cover the task with sstate (which is empty and just acts as a stamp
>> > saying it passed package QA). We also need to handle our own
>> > dependencies.
>> >
>> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
>
> I've just seen this email. I did see the failure from the overnight
> build and already sent a reply suggesting how this can be fixed, it
> should be a simple change.

Yes; I saw the proposed fix and I will do it here and test.

>> When I see a RFC serie I expect people to wait some days /for
>> comments/. I learned today those are already merged and /no comment
>> time/ has been given.
>
> The whole RFC was not merged. The previous RFC was and when that
> happened, to avoid more rebuilds, I chose to merge some parts of the
> second one since there appeared low risk.
>
> I send out a lot of patches, in general I get zero replies to most of
> them so its hard to know when I've waited "long enough". I did seek out
> and talk to some people about those RFCs and the feedback was positive,
> we all agree that the improvements to the dependency checking outweigh
> any downsides, including to be this issue.

Right but we could have it tried in AB and, as other times we did, do
the fixes to avoid master breakage.

I liked the patches and I did look at them. I was going to test the
stuff when I noticed it were merged. For RFC patches to be sincere I
expect them to be send /again/ without being RFC prior merging.

When you intend to give a short time for review/comment please comment
this in the series; so if someone wants more time can comment.

>> The problem I found is that running the QA checks in another task
>> seems to break some use-cases.
>
> I think in this case the usage is rather horrid and not something I'd
> say we've ever suggested or supported.

Well ... I'd prefer Freescale to fix their binaries for sure but I
cannot do it myself ;-)

>> In the libfslcodec (it is a binary blob provided by Freescale for
>> multimedia) we have:
>>
>> ...
>> python populate_packages_prepend() {
>>     ...
>>     # FIXME: All binaries lack GNU_HASH in elf binary but as we don't have
>>     # the source we cannot fix it. Disable the insane check for now.
>>     for p in d.getVar('PACKAGES', True).split():
>>         d.setVar("DEBIAN_NOAUTONAME_%s" % p, "1")
>>
>>         if p == 'libfslcodec-test-bin':
>>             # FIXME: includes the DUT .so files so we need to deploy those
>>             d.appendVar("INSANE_SKIP_%s" % p, "ldflags textrel libdir")
>>         else:
>>             d.appendVar("INSANE_SKIP_%s" % p, "ldflags textrel")
>>     ...
>> }
>>
>> In this case the INSANE_SKIP var is not being set on time. If we check
>> the value it is being set but it seems to be too late.
>>
>> How we can address it?
>
> Should be as simple as s/populate_packages_prepend//. The
> DEBIAN_NOAUTONAME doesn't really fit the description of the code there
> FWIW, I know why you're doing it but its not what the comment says.

I will review it.

> I would not recommend setting variables in function prepends like this,
> its ugly and error prone, as you've just found out.

Any suggested better way of to do it?
Richard Purdie - July 11, 2014, 1:22 p.m.
On Fri, 2014-07-11 at 09:14 -0300, Otavio Salvador wrote:
> >> When I see a RFC serie I expect people to wait some days /for
> >> comments/. I learned today those are already merged and /no comment
> >> time/ has been given.
> >
> > The whole RFC was not merged. The previous RFC was and when that
> > happened, to avoid more rebuilds, I chose to merge some parts of the
> > second one since there appeared low risk.
> >
> > I send out a lot of patches, in general I get zero replies to most of
> > them so its hard to know when I've waited "long enough". I did seek out
> > and talk to some people about those RFCs and the feedback was positive,
> > we all agree that the improvements to the dependency checking outweigh
> > any downsides, including to be this issue.
> 
> Right but we could have it tried in AB and, as other times we did, do
> the fixes to avoid master breakage.

I was under the impression that we did run some tests but I think it was
hidden by other failures. We tend to test patches in batches and then
have to make a best guess as to what is ready to merge.

> I liked the patches and I did look at them. I was going to test the
> stuff when I noticed it were merged. For RFC patches to be sincere I
> expect them to be send /again/ without being RFC prior merging.
> 
> When you intend to give a short time for review/comment please comment
> this in the series; so if someone wants more time can comment.
> 
> >> The problem I found is that running the QA checks in another task
> >> seems to break some use-cases.
> >
> > I think in this case the usage is rather horrid and not something I'd
> > say we've ever suggested or supported.
> 
> Well ... I'd prefer Freescale to fix their binaries for sure but I
> cannot do it myself ;-)

What I mean specifically is that the populate_packages_prepend is
horrid. populate packages is for what it says, populating the packages,
not for package QA testing. Setting this there happened to work but I
don't think anyone ever intended that to work. Even touching
populate_packages itself is not recommended, there are lists of
functions you can append your own to now.

The right level to set this data is at the recipe level which is what an
anonymous python fragment will do.

> >> In the libfslcodec (it is a binary blob provided by Freescale for
> >> multimedia) we have:
> >>
> >> ...
> >> python populate_packages_prepend() {
> >>     ...
> >>     # FIXME: All binaries lack GNU_HASH in elf binary but as we don't have
> >>     # the source we cannot fix it. Disable the insane check for now.
> >>     for p in d.getVar('PACKAGES', True).split():
> >>         d.setVar("DEBIAN_NOAUTONAME_%s" % p, "1")
> >>
> >>         if p == 'libfslcodec-test-bin':
> >>             # FIXME: includes the DUT .so files so we need to deploy those
> >>             d.appendVar("INSANE_SKIP_%s" % p, "ldflags textrel libdir")
> >>         else:
> >>             d.appendVar("INSANE_SKIP_%s" % p, "ldflags textrel")
> >>     ...
> >> }
> >>
> >> In this case the INSANE_SKIP var is not being set on time. If we check
> >> the value it is being set but it seems to be too late.
> >>
> >> How we can address it?
> >
> > Should be as simple as s/populate_packages_prepend//. The
> > DEBIAN_NOAUTONAME doesn't really fit the description of the code there
> > FWIW, I know why you're doing it but its not what the comment says.
> 
> I will review it.
> 
> > I would not recommend setting variables in function prepends like this,
> > its ugly and error prone, as you've just found out.
> 
> Any suggested better way of to do it?

The anonymous python fragment should be fine, it sets the data at the
right level, the global recipe level rather than burying it in some
task.

Cheers,

Richard
Otavio Salvador - July 11, 2014, 4:46 p.m.
On Fri, Jul 11, 2014 at 10:22 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> On Fri, 2014-07-11 at 09:14 -0300, Otavio Salvador wrote:
...
>> > I would not recommend setting variables in function prepends like this,
>> > its ugly and error prone, as you've just found out.
>>
>> Any suggested better way of to do it?
>
> The anonymous python fragment should be fine, it sets the data at the
> right level, the global recipe level rather than burying it in some
> task.

and in case we need to do it for the packages split from the
do_package_split? Use the hook mechanism?
Richard Purdie - July 11, 2014, 5:40 p.m.
On Fri, 2014-07-11 at 13:46 -0300, Otavio Salvador wrote:
> On Fri, Jul 11, 2014 at 10:22 AM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > On Fri, 2014-07-11 at 09:14 -0300, Otavio Salvador wrote:
> ...
> >> > I would not recommend setting variables in function prepends like this,
> >> > its ugly and error prone, as you've just found out.
> >>
> >> Any suggested better way of to do it?
> >
> > The anonymous python fragment should be fine, it sets the data at the
> > right level, the global recipe level rather than burying it in some
> > task.
> 
> and in case we need to do it for the packages split from the
> do_package_split? Use the hook mechanism?

That will not work, if you set data in the do_package task, the
do_package_q task cannot see it.

You'd probably have to add a prefunc to do_package_qa to set the right
skip variables if you needed to that since the package names wouldn't be
known until then.

Cheers,

Richard
Ross Burton - July 11, 2014, 7:37 p.m.
On 11 July 2014 14:22, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> The anonymous python fragment should be fine, it sets the data at the
> right level, the global recipe level rather than burying it in some
> task.

FWIW this broke the X drivers as they inject dependencies in
populate_packages_prepend().  It's probably worth reviewing all
instances of populate_packages_prepend() in oe-core at least as I
clearly copied using that hook from someone else...

Ross
Otavio Salvador - July 11, 2014, 7:46 p.m.
On Fri, Jul 11, 2014 at 4:37 PM, Burton, Ross <ross.burton@intel.com> wrote:
> On 11 July 2014 14:22, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
>> The anonymous python fragment should be fine, it sets the data at the
>> right level, the global recipe level rather than burying it in some
>> task.
>
> FWIW this broke the X drivers as they inject dependencies in
> populate_packages_prepend().  It's probably worth reviewing all
> instances of populate_packages_prepend() in oe-core at least as I
> clearly copied using that hook from someone else...

I managed to fix it here; if you need I can help you on it.

I am working on the other instances of meta-fsl-arm for it.
Martin Jansa - July 18, 2014, 10:26 a.m.
On Thu, Jul 10, 2014 at 11:43:02PM -0300, Otavio Salvador wrote:
> Hello Richard,
> 
> (added meta-freescale mailing list as it is related to meta-fsl-arm)
> 
> On Wed, Jul 9, 2014 at 5:15 PM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > Its possible to run the package QA checks as a separate task rather than
> > as part of the do_package task. This offers more parallelism but the
> > fact that made me propose this is that ideally we'd like to access
> > pkgdata to help add new tests and to do that, we need to run later in
> > the task list. We also need to add in RDEPENDS to the task which apply
> > to do_package_write_* but not do_package. See the subsequent patches
> > for why this is desireable.
> >
> > If we split into a separate task, we need to add in calls to read
> > the sub package data, build the cache structure used by do_package and
> > cover the task with sstate (which is empty and just acts as a stamp
> > saying it passed package QA). We also need to handle our own
> > dependencies.
> >
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> 
> When I see a RFC serie I expect people to wait some days /for
> comments/. I learned today those are already merged and /no comment
> time/ has been given.
> 
> The problem I found is that running the QA checks in another task
> seems to break some use-cases.
> 
> In the libfslcodec (it is a binary blob provided by Freescale for
> multimedia) we have:
> 
> ...
> python populate_packages_prepend() {
>     ...
>     # FIXME: All binaries lack GNU_HASH in elf binary but as we don't have
>     # the source we cannot fix it. Disable the insane check for now.
>     for p in d.getVar('PACKAGES', True).split():
>         d.setVar("DEBIAN_NOAUTONAME_%s" % p, "1")
> 
>         if p == 'libfslcodec-test-bin':
>             # FIXME: includes the DUT .so files so we need to deploy those
>             d.appendVar("INSANE_SKIP_%s" % p, "ldflags textrel libdir")
>         else:
>             d.appendVar("INSANE_SKIP_%s" % p, "ldflags textrel")
>     ...
> }
> 
> In this case the INSANE_SKIP var is not being set on time. If we check
> the value it is being set but it seems to be too late.

FWIW: llvm in meta-oe has similar problem

python llvm_populate_packages() {
    libdir = bb.data.expand('${libdir}', d)
    libllvm_libdir = bb.data.expand('${libdir}/${LLVM_DIR}', d)
    split_dbg_packages = do_split_packages(d, libllvm_libdir+'/.debug', '^lib(.*)\.so$', 'libllvm${LLVM_RELEASE}-%s-dbg', 'Split debug package for %s', allow_dirs=True)
    split_packages = do_split_packages(d, libdir, '^lib(.*)\.so$', 'libllvm${LLVM_RELEASE}-%s', 'Split package for %s', allow_dirs=True, allow_links=True, recursive=True)
    split_staticdev_packages = do_split_packages(d, libllvm_libdir, '^lib(.*)\.a$', 'libllvm${LLVM_RELEASE}-%s-staticdev', 'Split staticdev package for %s', allow_dirs=True)   
    if split_packages:
        pn = d.getVar('PN', True)
        for package in split_packages:
            d.appendVar('INSANE_SKIP_' + package, ' dev-so')
        d.appendVar('RDEPENDS_' + pn, ' '+' '.join(split_packages))
        d.appendVar('RDEPENDS_' + pn + '-dbg', ' '+' '.join(split_dbg_packages))
        d.appendVar('RDEPENDS_' + pn + '-staticdev', ' '+' '.join(split_staticdev_packages))
}

PACKAGESPLITFUNCS_prepend = "llvm_populate_packages "

and now it fails with:

ERROR: QA Issue: non -dev/-dbg/-nativesdk package contains symlink .so: libllvm3.3-llvm-3.3 path '/work/armv5te-oe-linux-gnueabi/llvm3.3/3.3-r0/packages-split/libllvm3.3-llvm-3.3/usr/lib/libLLVM-3.3.so' [dev-so]
ERROR: QA run found fatal errors. Please consider fixing them.
ERROR: Function failed: do_package_qa
ERROR: Logfile of failure stored in: /home/jenkins/oe/world/shr-core/tmp-eglibc/work/armv5te-oe-linux-gnueabi/llvm3.3/3.3-r0/temp/log.do_package_qa.21132
NOTE: recipe llvm3.3-3.3-r0: task do_package_qa: Failed

I'll look at solution for fsl and do the same for llvm (and also possibly merge all dbg and staticdev packages into one.

Patch

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 825ff04..2c60917 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -17,9 +17,6 @@ 
 #   files under exec_prefix
 
 
-PACKAGE_DEPENDS += "${QADEPENDS}"
-PACKAGEFUNCS += " do_package_qa "
-
 # unsafe-references-in-binaries requires prelink-rtld from
 # prelink-native, but we don't want this DEPENDS for -native builds
 QADEPENDS = "prelink-native"
@@ -831,6 +828,8 @@  python do_package_qa () {
 
     bb.note("DO PACKAGE QA")
 
+    bb.build.exec_func("read_subpackage_metadata", d)
+
     logdir = d.getVar('T', True)
     pkg = d.getVar('PN', True)
 
@@ -858,6 +857,15 @@  python do_package_qa () {
     pkgdest = d.getVar('PKGDEST', True)
     packages = d.getVar('PACKAGES', True)
 
+    cpath = oe.cachedpath.CachedPath()
+    global pkgfiles
+    pkgfiles = {}
+    for pkg in (packages or "").split():
+        pkgfiles[pkg] = []
+        for walkroot, dirs, files in cpath.walk(pkgdest + "/" + pkg):
+            for file in files:
+                pkgfiles[pkg].append(walkroot + os.sep + file)
+
     # no packages should be scanned
     if not packages:
         return
@@ -912,6 +920,15 @@  python do_package_qa () {
     bb.note("DONE with PACKAGE QA")
 }
 
+addtask do_package_qa after do_package before do_build
+
+SSTATETASKS += "do_package_qa"
+do_package_qa[sstate-inputdirs] = ""
+do_package_qa[sstate-outputdirs] = ""
+python do_package_qa_setscene () {
+    sstate_setscene(d)
+}
+addtask do_package_qa_setscene
 
 python do_qa_staging() {
     bb.note("QA checking staging")
@@ -1021,6 +1038,8 @@  python () {
 
     issues = []
     if (d.getVar('PACKAGES', True) or "").split():
+        for dep in (d.getVar('QADEPENDS', True) or "").split():
+            d.appendVarFlag('do_package_qa', 'depends', " %s:do_populate_sysroot" % dep)
         for var in 'RDEPENDS', 'RRECOMMENDS', 'RSUGGESTS', 'RCONFLICTS', 'RPROVIDES', 'RREPLACES', 'FILES', 'pkg_preinst', 'pkg_postinst', 'pkg_prerm', 'pkg_postrm', 'ALLOW_EMPTY':
             if d.getVar(var):
                 issues.append(var)