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

[Mike Crowe <mac@mcrowe.com>]

On Thursday 14 October 2021 at 22:48:07 +0100, Richard Purdie wrote:
> On Thu, 2021-10-14 at 18:09 +0100, Mike Crowe wrote:
> > On Thursday 14 October 2021 at 17:54:18 +0100, Richard Purdie wrote:
> > > On Thu, 2021-10-14 at 17:42 +0100, Mike Crowe via lists.openembedded.org wrote:
> > > > 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.
> > 
> > [snip]
> > 
> > > > Patch series v4 coming soon.
> > > 
> > > I did have to rework it a bit as I also ran into a few issues and was just
> > > trying to get around to writing them up.
> > > 
> > > I added:
> > > 
> > > http://git.yoctoproject.org/cgit.cgi/poky/commit/?h=master-next&id=9e980f1a71e8b6af5ff6da9b02fac0f8bfd9d4fb
> > > 
> > > which means you can drop the imports.
> > > 
> > > You can use the nonlocal option to help the "sane" problem:
> > > 
> > > http://git.yoctoproject.org/cgit.cgi/poky/diff/meta/classes/license.bbclass?h=master-next&id=110fa545ac84e560691c7d9e0d1e6e8f70c66980
> > > 
> > > The autobuilder also shows issues as some of the functions you moved are
> > > referenced in other classes. The patch became:
> > > 
> > > http://git.yoctoproject.org/cgit.cgi/poky/commit/?h=master-next&id=110fa545ac84e560691c7d9e0d1e6e8f70c66980
> > 
> > Thanks for doing that. I thought I'd searched for other occurrences, but I
> > must have not done so properly. :(
> > 
> > > I was also just thinking we should probably add a new function and add calls at
> > > the end of the tasks like:
> > > 
> > > oe.qa.exit_if_errors(d)
> > > 
> > > with a definition similar to:
> > > 
> > > def exit_if_errors(d):
> > >     qa_sane = d.getVar("QA_SANE")
> > >     if not qa_sane:
> > >         bb.fatal("Fatal QA errors were found.")
> > > 
> > > and then renaming QA_SANE to something like QA_FATAL_ERRORS and exitting if set.
> > 
> > That sounds like it would neaten things up greatly and avoid the risk of
> > accidentally failing to make the error fatal. It ought to mean that many of the sane local
> > variables can be removed too.
> > 
> > Would you like me to provide new versions of the existing patches or add
> > new patches on top of the ones already in master-next?
> 
> I had half a patch for this last bit so I pushed it into master-next. I also
> merged by oe.qa import change.

I think that I already had equivalent changes to everything in
master-next:a53f5758bb2e501dfd34c6a6369303e91b382792. It was slow testing
over night! I've gone with QA_ERRORS_FOUND for the variable name since I
thought that more accurately captured what it meant. I also have only a
single call to oe.qa.exit_if_errors at the end of each task.

> I'm happy for you to take over from here and rework the patches into whatever
> makes the best sense for a series now we know what we're aiming for? I think
> there is a more cleanup of the "sane" variables that can be done beyond my quick
> patch.

Thanks. I'll hopefully send a new series for review today.

I'm finding myself wondering whether having oe.qa.add_messages collect
together all the messages and reporting them all in one go is worth the
complexity. The code that works out in advance which checks generate
warnings and which generate errors appears to be quite old and perhaps it
predates the current mechanism for classifying checks when they fail.
However, any changes here would be rather large, so it would probably make
sense to tackle this separately.

Mike.