Re: [OE-core] [poky-contrib][PATCH 3/4] package.bbclass: Add handle_qa_error check

["Peter Kjellerstedt" <peter.kjellerstedt@axis.com>]

> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-
> core@lists.openembedded.org> On Behalf Of Meh Mbeh Ida Delphine
> Sent: den 10 mars 2021 00:31
> To: openembedded-core@lists.openembedded.org
> Subject: [OE-core] [poky-contrib][PATCH 3/4] package.bbclass: Add
> handle_qa_error check
> 
> package_qa_handle_error() from insane.bbclass handles display of warnings
> by allowing the user decide whether they want the issues treated as
> warnings, errors or hidden completely. This is done by setting a variable
> "license_source_spdx" in local.conf.
> 
> Signed-off-by: Ida Delphine <idadelm@gmail.com>
> ---
>  meta/classes/package.bbclass | 128 ++++++++++++++++++-----------------
>  1 file changed, 65 insertions(+), 63 deletions(-)
> 
> diff --git a/meta/classes/package.bbclass b/meta/classes/package.bbclass
> index 4e1be661b3..8c0a49ad76 100644
> --- a/meta/classes/package.bbclass
> +++ b/meta/classes/package.bbclass
> @@ -1552,6 +1552,7 @@ PKGDATA_VARS = "PN PE PV PR PKGE PKGV PKGR LICENSE
> DESCRIPTION SUMMARY RDEPENDS
> 
>  python emit_pkgdata() {
>      import oe.license
> +    import bb.utils
>      from glob import glob
>      import json
>      import subprocess
> @@ -1712,70 +1713,71 @@ fi
>          write_extra_runtime_pkgs(global_variants, packages, pkgdatadir)
> 
>      sourceresult = d.getVar('TEMPDBGSRCMAPPING', False)
> -    sources = {}
> -    if sourceresult:
> -        for r in sourceresult:
> -            sources[r[0]] = r[1]
> -        with open(data_file + ".srclist", 'w') as f:
> -            f.write(json.dumps(sources, sort_keys=True))
> -
> -        filelics = {}
> -        for dirent in [d.getVar('PKGD'), d.getVar('STAGING_DIR_TARGET')]:
> -            p = subprocess.Popen(["grep", 'SPDX-License-Identifier:', '-R', '-I'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=dirent)
> -            out, err = p.communicate()
> -            if p.returncode == 0:
> -                for l in out.decode("utf-8").split("\n"):
> -                    l = l.strip()
> -                    if not l:
> -                        continue
> -                    l = l.split(":")
> -                    if len(l) < 3:
> -                        bb.warn(str(l))
> -                        continue
> -                    fn = "/" + l[0]
> -                    lic = l[2].strip()
> -                    if lic.endswith("*/"):
> -                        lic = lic[:-2]
> -                    lic = lic.strip()
> -                    filelics[fn] = lic
> -        with open(data_file + ".filelics", 'w') as f:
> -            f.write(json.dumps(filelics, sort_keys=True))
> -
> -        computedlics = {}
> -        computedpkglics = {}
> -        for r in sourceresult:
> -            for subf in r[1]:
> -                if subf in filelics:
> -                    if r[0] not in computedlics:
> -                        computedlics[r[0]] = set()
> -                    computedlics[r[0]].add(filelics[subf])
> -
> -
> -        dvar = d.getVar('PKGD')
> -        for f in computedlics:
> -            shortf = f.replace(dvar, "")
> -            found = False
> -            for pkg in filemap:
> -                if shortf in filemap[pkg]:
> -                    found = True
> -                    if pkg not in computedpkglics:
> -                        computedpkglics[pkg] = set()
> -                    computedpkglics[pkg].update(computedlics[f])
> -            if not found:
> -                bb.warn("%s not in %s" % (f, str(filemap)))
> -
> -        for pkg in computedpkglics:
> -            lic = d.getVar('LICENSE_%s' % (pkg))
> -            if not lic:
> -                lic = d.getVar('LICENSE')
> -
> -            # Splits the LICENSE values and canonicalise each license
> -            # in the set of split license(s)
> -            spdx_lic = oe.license.split_spdx_lic(lic, d)
> +    if bb.utils.contains("license_source_spdx", "1", True, False, d):

This kind of bitbake variable that controls functionality is typically in 
upper case. It also needs to be documented in the manual, and it should have 
a default value (??=). It would also make more sense to use 
bb.utils.to_boolean() than bb.utils.contains(). You can also do:

    if not bb.utils.to_boolean(d.getVar("license_source_spdx")):
        return

thereby avoiding the reindentation, making code review easier.

> +        sources = {}
> +        if sourceresult:
> +            for r in sourceresult:
> +                sources[r[0]] = r[1]
> +            with open(data_file + ".srclist", 'w') as f:
> +                f.write(json.dumps(sources, sort_keys=True))
> +
> +            filelics = {}
> +            for dirent in [d.getVar('PKGD'), d.getVar('STAGING_DIR_TARGET')]:
> +                p = subprocess.Popen(["grep", 'SPDX-License-Identifier:', '-R', '-I'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=dirent)
> +                out, err = p.communicate()
> +                if p.returncode == 0:
> +                    for l in out.decode("utf-8").split("\n"):
> +                        l = l.strip()
> +                        if not l:
> +                            continue
> +                        l = l.split(":")
> +                        if len(l) < 3:
> +                            bb.warn(str(l))
> +                            continue
> +                        fn = "/" + l[0]
> +                        lic = l[2].strip()
> +                        if lic.endswith("*/"):
> +                            lic = lic[:-2]
> +                        lic = lic.strip()
> +                        filelics[fn] = lic
> +            with open(data_file + ".filelics", 'w') as f:
> +                f.write(json.dumps(filelics, sort_keys=True))
> +
> +            computedlics = {}
> +            computedpkglics = {}
> +            for r in sourceresult:
> +                for subf in r[1]:
> +                    if subf in filelics:
> +                        if r[0] not in computedlics:
> +                            computedlics[r[0]] = set()
> +                        computedlics[r[0]].add(filelics[subf])
> +
> +
> +            dvar = d.getVar('PKGD')
> +            for f in computedlics:
> +                shortf = f.replace(dvar, "")
> +                found = False
> +                for pkg in filemap:
> +                    if shortf in filemap[pkg]:
> +                        found = True
> +                        if pkg not in computedpkglics:
> +                            computedpkglics[pkg] = set()
> +                        computedpkglics[pkg].update(computedlics[f])
> +                if not found:
> +                    bb.warn("%s not in %s" % (f, str(filemap)))
> 
> -            # Displays warnings for licenses found in the recipes and not sources
> -            if spdx_lic - computedpkglics[pkg]:
> -                bb.warn("License for package %s is %s vs %s" % (pkg, computedpkglics[pkg], spdx_lic))
> +            for pkg in computedpkglics:
> +                lic = d.getVar('LICENSE_%s' % (pkg))
> +                if not lic:
> +                    lic = d.getVar('LICENSE')
> +
> +                # Splits the LICENSE values and canonicalise each license
> +                # in the set of split license(s)
> +                spdx_lic = oe.license.split_spdx_lic(lic, d)
> +
> +                # Displays warnings for licenses found in the recipes and not sources
> +                if spdx_lic - computedpkglics[pkg]:
> +                    package_qa_handle_error("license_source_spdx", f'License for package {pkg} is {computedpkglics[pkg]}(source SPDX headers) vs {spdx_lic} (LICENSE)', d)

I'm all for using f'' strings, as I think it makes for a lot more readable 
code. However, I am not sure it has been approved for use in the OE code 
yet.

>  }
>  emit_pkgdata[dirs] = "${PKGDESTWORK}/runtime ${PKGDESTWORK}/runtime-reverse ${PKGDESTWORK}/runtime-rprovides"
> 
> --
> 2.25.1