indirect dependencies

indirect dependencies

[Patrick Ohly]

I recently ran into a situation where due to a stupid typo (DEPEND
instead of DEPENDS) my recipe happened to work for me when the
required package was pulled in indirectly and failed for someone else.

It would be nice if such indirect dependencies triggered a QA
warning, which they do not at the moment.

I started coding that, but ran into issues with false positives. I
don't have the time and knowledge to address these problems, but
wanted to shared the code anyway, in case that someone wants to
continue.

Patrick Ohly (1):
  insane.bbclass: add indirect-build-deps QA check

 meta/classes/insane.bbclass | 52 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 7 deletions(-)

-- 
2.1.4

[PATCH] insane.bbclass: add indirect-build-deps QA check

[Patrick Ohly]

Indirect dependencies occur when a recipe does not declare a depency
on "foo" because it depends on "bar", which in turn depends on
"foo". This happens to work, but is fragile and should better be avoided
by listing "foo" explicitly, because:
* "bar" might not depend on "foo" in all configurations
* future changes to "bar" might remove the dependency on "bar"

The traditional "build-deps" QA check skips indirect dependencies
because they are listed in the task dependencies. Checking for
indirect dependencies remembers that instead of aborting the check and
then later tries to filter out false positives by looking at DEPENDS.

Some common packages which can be assumed to be present are covered by
INDIRECT_BUILD_DEPS_WHITELIST. That list needs to be reviewed and also
take into account the build configuration - glibc is listed there at
the moment, but might not actually be the configured base library.

However, there are at least two false positives left in the current code:
* aliases for recipes via PROVIDES
* unnecessarily linking against libs which are not really used

See the TODOs in the code.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/classes/insane.bbclass | 52 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 7 deletions(-)

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 156ae5a..70f110c 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -29,7 +29,7 @@ QA_SANE = "True"
 WARN_QA ?= "ldflags useless-rpaths rpaths staticdev libdir xorg-driver-abi \
             textrel already-stripped incompatible-license files-invalid \
             installed-vs-shipped compile-host-path install-host-path \
-            pn-overrides infodir build-deps file-rdeps \
+            pn-overrides infodir build-deps indirect-build-deps file-rdeps \
             unknown-configure-option \
             "
 ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
@@ -42,6 +42,8 @@ ALL_QA = "${WARN_QA} ${ERROR_QA}"
 
 UNKNOWN_CONFIGURE_WHITELIST ?= "--enable-nls --disable-nls --disable-silent-rules --disable-dependency-tracking --with-libtool-sysroot"
 
+INDIRECT_BUILD_DEPS_WHITELIST ?= "glibc"
+
 #
 # dictionary for elf headers
 #
@@ -763,6 +765,8 @@ def package_qa_walk(path, warnfuncs, errorfuncs, skip, package, d):
 
     return len(errors) == 0
 
+
+
 def package_qa_check_rdepends(pkg, pkgdest, skip, taskdeps, packages, d):
     # Don't do this check for kernel/module recipes, there aren't too many debug/development
     # packages and you can get false positives e.g. on kernel-module-lirc-dev
@@ -779,18 +783,37 @@ def package_qa_check_rdepends(pkg, pkgdest, skip, taskdeps, packages, d):
         rdepends = bb.utils.explode_deps(localdata.getVar('RDEPENDS', True) or "")
 
         # Now do the sanity check!!!
-        if "build-deps" not in skip:
+        skip_indirect = "indirect-build-deps" in skip
+        skip_direct = "build-deps" in skip
+        if not skip_direct and not skip_indirect:
+            indirect_whitelist = [] if skip_indirect else set((d.getVar('INDIRECT_BUILD_DEPS_WHITELIST', True) or '').split())
             for rdepend in rdepends:
+                bb.note("DO PACKAGE QA: rdepend %s pkg %s" % (rdepend, pkg))
+
                 if "-dbg" in rdepend and "debug-deps" not in skip:
                     error_msg = "%s rdepends on %s" % (pkg,rdepend)
                     sane = package_qa_handle_error("debug-deps", error_msg, d)
                 if (not "-dev" in pkg and not "-staticdev" in pkg) and rdepend.endswith("-dev") and "dev-deps" not in skip:
                     error_msg = "%s rdepends on %s" % (pkg, rdepend)
                     sane = package_qa_handle_error("dev-deps", error_msg, d)
-                if rdepend not in packages:
+                internaldep = rdepend in packages
+                if not internaldep and not skip_indirect:
+                    # Might be a name provided by some of the packages (example:
+                    # libpam RPROVIDES libpam-suffix, which libpam-runtime RDEPENDS on).
+                    # If we do not check for this, the rdepend will treated like
+                    # an invalid indirect dependency although this is okay.
+                    if rdepend in (d.getVar("PROVIDES", True) or "").split():
+                        internaldep = True
+                    else:
+                        for internalpkg in packages:
+                            if rdepend in (d.getVar("RPROVIDES_%s" % internalpkg, True) or "").split():
+                               internaldep = True
+                               break
+                if not internaldep:
                     rdep_data = oe.packagedata.read_subpkgdata(rdepend, d)
+                    indirect = False
                     if rdep_data and 'PN' in rdep_data and rdep_data['PN'] in taskdeps:
-                        continue
+                        indirect = True
                     if not rdep_data or not 'PN' in rdep_data:
                         pkgdata_dir = d.getVar("PKGDATA_DIR", True)
                         try:
@@ -802,9 +825,24 @@ def package_qa_check_rdepends(pkg, pkgdest, skip, taskdeps, packages, d):
                             if rdep_data and 'PN' in rdep_data and rdep_data['PN'] in taskdeps:
                                 break
                     if rdep_data and 'PN' in rdep_data and rdep_data['PN'] in taskdeps:
-                        continue
-                    error_msg = "%s rdepends on %s, but it isn't a build dependency?" % (pkg, rdepend)
-                    sane = package_qa_handle_error("build-deps", error_msg, d)
+                        indirect = True
+                    if indirect:
+                        if skip_indirect or rdepend in indirect_whitelist:
+                            continue
+                        # Need to avoid false positives when the potential indirect dependency is
+                        # in fact listed as a direct dependency. Also need to handle aliases here.
+                        depends = set((d.getVar('DEPENDS', True) or '').split())
+                        if rdepend in depends:
+                            continue
+                        # TODO: check PROVIDES of the rdepend recipe to see whether that is
+                        # listed in DEPENDS.
+                        error_msg = "%s rdepends on %s, but it only an indirect build dependency?" % (pkg, rdepend)
+                        sane = package_qa_handle_error("indirect-build-deps", error_msg, d)
+                    else:
+                        if skip_direct:
+                            continue
+                        error_msg = "%s rdepends on %s, but it isn't a build dependency?" % (pkg, rdepend)
+                        sane = package_qa_handle_error("build-deps", error_msg, d)
 
         if "file-rdeps" not in skip:
             ignored_file_rdeps = set(['/bin/sh', '/usr/bin/env', 'rtld(GNU_HASH)'])
-- 
2.1.4

Re: [PATCH] insane.bbclass: add indirect-build-deps QA check

[Patrick Ohly]

On Wed, 2015-06-17 at 12:25 +0200, Patrick Ohly wrote:
> However, there are at least two false positives left in the current code:
> * aliases for recipes via PROVIDES
> * unnecessarily linking against libs which are not really used
> 
> See the TODOs in the code.

Actually, I had written more about that which didn't make it into the
posted patch. Here's the more complete comment:


                        # TODO: check PROVIDES of the rdepend recipe to see whether that is
                        # listed in DEPENDS. Example: "smack" instead of "userspace-smack" in
                        # https://github.com/01org/meta-intel-iot-security/blob/master/meta-security-smack/recipes-extended/libpam/libpam_%25.bbappend which is an alias for
                        # https://github.com/01org/meta-intel-iot-security/blob/master/meta-security-smack/recipes-security/smack/smack-userspace_git.bb

                        # TODO: pam-plugin-cracklib/lib/security/pam_cracklib.so is unecessarily
                        # linked against libz.so.1, probably because of link flags from libcrack,
                        # which is what libpam calls and (correctly) lists as DEPENDS.
                        # Because libz is not listed as DEPENDS, we warn about it here
                        # unnecessarily. This could be fairly common; not sure how to detect
                        # it though, short of comparing symbol tables.


> +                        # TODO: check PROVIDES of the rdepend recipe to see whether that is
> +                        # listed in DEPENDS.
> +                        error_msg = "%s rdepends on %s, but it only an indirect build dependency?" % (pkg, rdepend)
> +                        sane = package_qa_handle_error("indirect-build-deps", error_msg, d)

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.