Re: [PATCH v3 2/2] license: Allow treating missing license as error

[Mike Crowe <mac@mcrowe.com>]

On Wednesday 13 October 2021 at 11:48:05 +0100, Mike Crowe wrote:
> Use the same WARN_WA and ERROR_QA variables as insane.bbclass to allow
> individual recipes, the distro or other configuration to determine
> whether a missing licence should be treated as a warning (as it is now)
> or as an error.
> 
> oe.qa.handle_error isn't immediately fatal, so track the overall sanity
> state and call bb.fatal if required at the end 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.
> 
> It seems odd for the license- error classes to be listed in
> insane.bbclass but implemented in license.bbclass. All recommendations
> for somewhere else to put them gratefully received.
> 
> Signed-off-by: Mike Crowe <mac@mcrowe.com>
> ---
>  meta/classes/insane.bbclass  |  1 +
>  meta/classes/license.bbclass | 27 ++++++++++++++++++++-------
>  2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 895216d3e8..57456c99ad 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -28,6 +28,7 @@ WARN_QA ?= " libdir xorg-driver-abi \
>              invalid-packageconfig host-user-contaminated uppercase-pn patch-fuzz \
>              mime mime-xdg unlisted-pkg-lics unhandled-features-check \
>              missing-update-alternatives native-last missing-ptest \
> +            license-exists license-no-generic license-syntax license-format \
>              "
>  ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
>              perms dep-cmp pkgvarcheck perm-config perm-line perm-link \
> diff --git a/meta/classes/license.bbclass b/meta/classes/license.bbclass
> index 45d912741d..6bbb71392e 100644
> --- a/meta/classes/license.bbclass
> +++ b/meta/classes/license.bbclass
> @@ -112,6 +112,7 @@ def find_license_files(d):
>      import oe.license
>      from collections import defaultdict, OrderedDict
>  
> +    sane = True
>      # All the license files for the package
>      lic_files = d.getVar('LIC_FILES_CHKSUM') or ""
>      pn = d.getVar('PN')
> @@ -146,6 +147,7 @@ def find_license_files(d):
>              self.generic_visit(node)
>  
>      def find_license(license_type):
> +        import oe.qa

There's a "sane = True" missing here.

>          try:
>              bb.utils.mkdirhier(gen_lic_dest)
>          except:
> @@ -178,7 +180,8 @@ def find_license_files(d):
>              # The user may attempt to use NO_GENERIC_LICENSE for a generic license which doesn't make sense
>              # and should not be allowed, warn the user in this case.
>              if d.getVarFlag('NO_GENERIC_LICENSE', license_type):
> -                bb.warn("%s: %s is a generic license, please don't use NO_GENERIC_LICENSE for it." % (pn, license_type))
> +                sane &= oe.qa.handle_error("license-no-generic",
> +                    "%s: %s is a generic license, please don't use NO_GENERIC_LICENSE for it." % (pn, license_type), d)
>  
>          elif non_generic_lic and non_generic_lic in lic_chksums:
>              # if NO_GENERIC_LICENSE is set, we copy the license files from the fetched source
> @@ -190,7 +193,8 @@ 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))
> +                sane &= oe.qa.handle_error("license-exists",
> +                    "%s: No generic license file exists for: %s in any provider" % (pn, license_type), d)
>              pass

and a check for not sane missing here.

>  
>      if not generic_directory:
> @@ -215,7 +219,8 @@ def find_license_files(d):
>      except oe.license.InvalidLicense as exc:
>          bb.fatal('%s: %s' % (d.getVar('PF'), exc))
>      except SyntaxError:
> -        bb.warn("%s: Failed to parse it's LICENSE field." % (d.getVar('PF')))
> +        sane &= oe.qa.handle_error("license-syntax",
> +            "%s: Failed to parse it's LICENSE field." % (d.getVar('PF')), d)
>      # Add files from LIC_FILES_CHKSUM to list of license files
>      lic_chksum_paths = defaultdict(OrderedDict)
>      for path, data in sorted(lic_chksums.items()):
> @@ -233,6 +238,8 @@ def find_license_files(d):
>              for i, data in enumerate(files.values()):
>                  lic_files_paths.append(tuple(["%s.%d" % (basename, i)] + list(data)))
>  
> +    if not sane:
> +        bb.fatal("Fatal QA errors found, failing task.")
>      return lic_files_paths
>  
>  def return_spdx(d, license):
> @@ -398,6 +405,8 @@ def check_license_format(d):
>          Validate operators in LICENSES.
>          No spaces are allowed between LICENSES.
>      """
> +    import oe.qa
> +    sane = True
>      pn = d.getVar('PN')
>      licenses = d.getVar('LICENSE')
>      from oe.license import license_operator, license_operator_chars, license_pattern
> @@ -406,14 +415,18 @@ def check_license_format(d):
>      for pos, element in enumerate(elements):
>          if license_pattern.match(element):
>              if pos > 0 and license_pattern.match(elements[pos - 1]):
> -                bb.warn('%s: LICENSE value "%s" has an invalid format - license names ' \
> +                sane &= oe.qa.handle_error('license-format',
> +                        '%s: LICENSE value "%s" has an invalid format - license names ' \
>                          'must be separated by the following characters to indicate ' \
>                          'the license selection: %s' %
> -                        (pn, licenses, license_operator_chars))
> +                        (pn, licenses, license_operator_chars), d)
>          elif not license_operator.match(element):
> -            bb.warn('%s: LICENSE value "%s" has an invalid separator "%s" that is not ' \
> +            sane &= oe.qa.handle_error('license-format',
> +                    '%s: LICENSE value "%s" has an invalid separator "%s" that is not ' \
>                      'in the valid list of separators (%s)' %
> -                    (pn, licenses, element, license_operator_chars))
> +                    (pn, licenses, element, license_operator_chars), d)
> +    if not sane:
> +        bb.fatal("Fatal QA errors found, failing task.")
>  
>  SSTATETASKS += "do_populate_lic"
>  do_populate_lic[sstate-inputdirs] = "${LICSSTATEDIR}"

and license_image.bbclass probably needs the same treatment. Patch series v4 coming soon.

Mike.