Re: [OE-core] [PATCH v2] license: Allow treating missing license as error

[Mike Crowe <mac@mcrowe.com>]

On Tuesday 12 October 2021 at 14:21:05 +0100, Richard Purdie wrote:
> On Sun, 2021-10-10 at 18:20 +0100, Mike Crowe via lists.openembedded.org wrote:
> > Use mechanism inspired by insane.bbclass to allow individual recipes or
> > other configuration to determine whether a missing licence should be
> > treated as a warning (as it is now) or as an error. This is controlled
> > by whether the error class is in WARN_LICENSE or ERROR_LICENSE.
> > 
> > Use bb.fatal in the error case to ensure that the task really fails. If
> > only bb.error is used then do_populate_lic isn't re-run on subsequent
> > builds which could lead to the error being missed.
> > 
> > Signed-off-by: Mike Crowe <mac@mcrowe.com>
> > ---
> >  meta/classes/license.bbclass | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/meta/classes/license.bbclass b/meta/classes/license.bbclass
> > index 45d912741d..f0a6c0c20e 100644
> > --- a/meta/classes/license.bbclass
> > +++ b/meta/classes/license.bbclass
> > @@ -12,6 +12,23 @@ LICENSE_CREATE_PACKAGE ??= "0"
> >  LICENSE_PACKAGE_SUFFIX ??= "-lic"
> >  LICENSE_FILES_DIRECTORY ??= "${datadir}/licenses/"
> >  
> > +# Elect whether a given type of error is a warning or error, they may
> > +# have been set by other files.
> > +WARN_LICENSE ?= "no-license"
> > +ERROR_LICENSE ?= ""
> > +WARN_LICENSE[doc] = "Space-separated list of license problems that should be reported only as warnings"
> > +ERROR_LICENSE[doc] = "Space-separated list of license problems that should be reported as errors"
> > +
> > +def package_license_handle_error(error_class, error_msg, d):
> > +    if error_class in (d.getVar("ERROR_LICENSE") or "").split():
> > +        package_qa_write_error(error_class, error_msg, d)
> > +        bb.fatal("License Issue: %s [%s]" % (error_msg, error_class))
> > +    elif error_class in (d.getVar("WARN_LICENSE") or "").split():
> > +        package_qa_write_error(error_class, error_msg, d)
> > +        bb.warn("License Issue: %s [%s]" % (error_msg, error_class))
> > +    else:
> > +        bb.note("License Issue: %s [%s]" % (error_msg, error_class))
> > +
> >  addtask populate_lic after do_patch before do_build
> >  do_populate_lic[dirs] = "${LICSSTATEDIR}/${PN}"
> >  do_populate_lic[cleandirs] = "${LICSSTATEDIR}"
> > @@ -190,7 +207,7 @@ def find_license_files(d):
> >              # Add explicity avoid of CLOSED license because this isn't generic
> >              if license_type != 'CLOSED':
> >                  # And here is where we warn people that their licenses are lousy
> > -                bb.warn("%s: No generic license file exists for: %s in any provider" % (pn, license_type))
> > +                package_license_handle_error("no-license", "%s: No generic license file exists for: %s in any provider" % (pn, license_type), d)
> >              pass
> >  
> >      if not generic_directory:
> 
> I'm a little torn on this and whether we should make it use the same variables
> as the other QA checks? Is there a reason the user would want to configure this
> sanity check separately from the other sanity checks?

I modelled this on the QA checks in insane.bbclass because that appeared to
be the most likely to be an acceptable way to do it. I'd be happy to use
the same variables, although that does raise the question of whether
license.bbclass needs to inherit from insane.bbclass or those variables
need moving somewhere else (see below).

> I'm not sure I can see a long list of different license checks we'd want to add
> here?

There are many other warnings reported by license.bbclass. Many of them
feel like errors to me. I'd be happy to have a single switch that converted
them all to errors, but I haven't tried to do so to see what problems we'd
run into.

> The current sanity checks in insane.bbclass could do with some cleanup and
> refactoring so perhaps this could be come a common function (and common variable
> to control all the QA checks)?

Where would be the best place to put this function? A new qa.bbclass that
can be inherited by both license.bbclass and insane.bbclass?

Did you have any particular cleanups and refactorings in mind? I must admit
that I was surprised by the long list in a single variable assigned with
?=. It means that anyone overriding the variable won't benefit from
newly-added checks automatically.

The only alternative mechanism I came up with was something like:

 QA_CHECK[libdir] = "warn"
 QA_CHECK[dev-so] = "error"
 QA_CHECK[mime] = "ignore"

and then let recipes and configurations override individual checks as they
wish.

Thanks.

Mike.