insane: Split do_package_qa into a separate task (from do_package)

RFC PATCH In review Richard Purdie 4 participants 11 messages Jul 9, 2014 – Jul 18, 2014

Share & tools

Richard Purdie

author

2014-07-09 20:15 UTC #0 lore raw

RFC PATCH 1/3 insane: Split do_package_qa into a separate task (from do_package)

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 <[email protected]>

meta/classes/insane.bbclass +22 -3

index 825ff04..2c60917 100644
@@ -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)

Otavio Salvador

2014-07-11 02:43 UTC #1 lore raw

Hello Richard,

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

Quoted 8 lines · Richard Purdie

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.

Quoted 7 lines

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 <[email protected]>

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?

Signature · 3 lines

Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Richard Purdie

author

2014-07-11 08:27 UTC #2 lore raw

Quoted 10 lines · Otavio Salvador

Quoted 8 lines · Richard Purdie
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.

Quoted 7 lines

Quoted 7 lines
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 <[email protected]>

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 19 lines

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")
...
}

Quoted 4 lines

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

2014-07-11 12:14 UTC #3 lore raw

Hello Richard,

Quoted 17 lines · Richard Purdie

Quoted 16 lines · Otavio Salvador
Quoted 14 lines · Richard Purdie
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.

Quoted 5 lines

Quoted 1 line
Quoted 1 line
Signed-off-by Richard Purdie <[email protected]>
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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 5 lines

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.

Quoted 5 lines

Quoted 2 lines
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 ;-)

Quoted 19 lines

Quoted 19 lines
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")
...
}

Quoted 8 lines

Quoted 4 lines
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.

Quoted 2 lines

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?

Signature · 3 lines

Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Richard Purdie

author

2014-07-11 13:22 UTC #4 lore raw

Quoted 8 lines · Otavio Salvador

Quoted 8 lines
Quoted 3 lines
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.

Quoted 8 lines

Quoted 5 lines
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.

Quoted 7 lines

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.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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.

Quoted 28 lines

Quoted 19 lines
Quoted 19 lines
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")
...
}
Quoted 8 lines
Quoted 4 lines
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.

Quoted 6 lines

I will review it.
Quoted 2 lines
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

2014-07-11 16:46 UTC #5 lore raw

Quoted 1 line · Richard Purdie

On Fri, 2014-07-11 at 09:14 -0300, Otavio Salvador wrote:

...

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
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?

Signature · 3 lines

Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Richard Purdie

author

2014-07-11 17:41 UTC #6 lore raw

Quoted 7 lines · Otavio Salvador

Quoted 1 line · Richard Purdie
On Fri, 2014-07-11 at 09:14 -0300, Otavio Salvador wrote:
...
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
I would not recommend setting variables in function prepends like this, its ugly and error prone, as you've just found out.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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

Burton, Ross

2014-07-11 19:37 UTC #7 lore raw

In reply to Richard Purdie

Quoted 3 lines · Richard Purdie

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

2014-07-11 19:46 UTC #8 lore raw

Quoted 10 lines · Ross

Quoted 3 lines · Richard Purdie
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.

Signature · 3 lines

Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Richard Purdie

author

2014-07-11 21:12 UTC #9 lore raw

In reply to Burton, Ross

Quoted 10 lines · Ross

Quoted 3 lines · Richard Purdie
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...

It shouldn't have broken the dependencies, you can still inject things there and that has not changed. If that has stopped working, there is something else wrong.

What you can't now do is change the package QA process through populate_packages but that makes much less sense.

Cheers,

Richard

Martin Jansa

2014-07-18 10:26 UTC #10 lore raw

In reply to Otavio Salvador

Quoted 48 lines · Otavio Salvador

Hello Richard,
(added meta-freescale mailing list as it is related to meta-fsl-arm)
Quoted 8 lines · Richard Purdie
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.
Quoted 7 lines
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 <[email protected]>
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")
...
}

Quoted 2 lines

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.

Signature · 1 line

Martin 'JaMa' Jansa     jabber: [email protected]