[PATCH 0/1] bbclass: bb.fatal() clean up

[PATCH 0/1] bbclass: bb.fatal() clean up

[Robert Yang]

The following changes since commit 9895d2c074156fee338d91aed7cfb0800477c622:

  maintainers.inc: update sbc pkg maintainer (2013-05-07 13:58:26 +0100)

are available in the git repository at:

  git://git.pokylinux.org/poky-contrib robert/fatal
  http://git.pokylinux.org/cgit.cgi/poky-contrib/log/?h=robert/fatal

Robert Yang (1):
  bbclass: bb.fatal() clean up

 meta/classes/insane.bbclass      |    1 -
 meta/classes/package.bbclass     |    1 -
 meta/classes/package_rpm.bbclass |    2 --
 3 files changed, 4 deletions(-)

-- 
1.7.10.4

[PATCH 1/1] bbclass: bb.fatal() clean up

[Robert Yang]

The bb.fatal() is defined as:

def fatal(*args):
    logger.critical(''.join(args))
    sys.exit(1)

So anything after bb.fatal() in the same code block doesn't have any
effect, e.g.:

    bb.fatal("%s_%s: %s" % (var, pkg, e))
    raise e

The "raise e" should be removed.

I searched all the files which use bb.fatal(), only the following 3
classes have this issues:

  insane.bbclass
  package.bbclass
  package_rpm.bbclass

[YOCTO #4461]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/classes/insane.bbclass      |    1 -
 meta/classes/package.bbclass     |    1 -
 meta/classes/package_rpm.bbclass |    2 --
 3 files changed, 4 deletions(-)

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 336beaa..6b696b1 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -715,7 +715,6 @@ def package_qa_check_deps(pkg, pkgdest, skip, d):
             rvar = bb.utils.explode_dep_versions2(localdata.getVar(var, True) or "")
         except ValueError as e:
             bb.fatal("%s_%s: %s" % (var, pkg, e))
-            raise e
         for dep in rvar:
             for v in rvar[dep]:
                 if v and not v.startswith(('< ', '= ', '> ', '<= ', '>=')):
diff --git a/meta/classes/package.bbclass b/meta/classes/package.bbclass
index 1a8da40..a31e6bd 100644
--- a/meta/classes/package.bbclass
+++ b/meta/classes/package.bbclass
@@ -375,7 +375,6 @@ python package_get_auto_pr() {
             auto_pr=prserv_get_pr_auto(d)
         except Exception as e:
             bb.fatal("Can NOT get PRAUTO, exception %s" %  str(e))
-            return
         if auto_pr is None:
             if d.getVar('PRSERV_LOCKDOWN', True):
                 bb.fatal("Can NOT get PRAUTO from lockdown exported file")
diff --git a/meta/classes/package_rpm.bbclass b/meta/classes/package_rpm.bbclass
index 3a29976..edd90da 100644
--- a/meta/classes/package_rpm.bbclass
+++ b/meta/classes/package_rpm.bbclass
@@ -674,12 +674,10 @@ python write_specfile () {
     pkgdest = d.getVar('PKGDEST', True)
     if not pkgdest:
         bb.fatal("No PKGDEST")
-        return
 
     outspecfile = d.getVar('OUTSPECFILE', True)
     if not outspecfile:
         bb.fatal("No OUTSPECFILE")
-        return
 
     # Construct the SPEC file...
     srcname    = strip_multilib(d.getVar('PN', True), d)
-- 
1.7.10.4

Re: [PATCH 1/1] bbclass: bb.fatal() clean up

[Mike Looijmans]

On 05/08/2013 11:06 AM, Robert Yang wrote:
> The bb.fatal() is defined as:
>
> def fatal(*args):
>      logger.critical(''.join(args))
>      sys.exit(1)
>
> So anything after bb.fatal() in the same code block doesn't have any
> effect, e.g.:
>
>      bb.fatal("%s_%s: %s" % (var, pkg, e))
>      raise e
>
> The "raise e" should be removed.

Just some random thoughts that occurred to me when I read this:

The "terminate" effect would be obvious if "fatal" were an exception to 
be raised instead of a function to call which does not really return. If 
I'm not mistaken, "sys.exit(1)" actually just raises a SystemExit exception.

So instead of:

bb.fatal("something went wrong")

the syntax would become:

raise bb.Fatal("something went wrong")


Having typed this, the next random thought I got was that a thing like

catch Exception, e:
    bb.fatal("Error: ", e)

isn't really adding anything useful, it just "translates" the exception, 
logs its message, and then throws an obscure system exit exception 
instead of the much more useful inner exception.

Re: [PATCH 1/1] bbclass: bb.fatal() clean up

[Robert Yang]

On 05/08/2013 08:03 PM, Mike Looijmans wrote:
> On 05/08/2013 11:06 AM, Robert Yang wrote:
>> The bb.fatal() is defined as:
>>
>> def fatal(*args):
>>      logger.critical(''.join(args))
>>      sys.exit(1)
>>
>> So anything after bb.fatal() in the same code block doesn't have any
>> effect, e.g.:
>>
>>      bb.fatal("%s_%s: %s" % (var, pkg, e))
>>      raise e
>>
>> The "raise e" should be removed.
>
> Just some random thoughts that occurred to me when I read this:
>

Hi Mike, thanks for your comments, but the "raise sys.exit(1)" doesn't raise
anything, e.g.:

import sys

def fatal():
         sys.exit(1)

try:
         raise fatal()
except Exception as e:
         raise e

I think that the "raise fatal()" equals to "fatal()" here.

// Robert

> The "terminate" effect would be obvious if "fatal" were an exception to be
> raised instead of a function to call which does not really return. If I'm not
> mistaken, "sys.exit(1)" actually just raises a SystemExit exception.
>
> So instead of:
>
> bb.fatal("something went wrong")
>
> the syntax would become:
>
> raise bb.Fatal("something went wrong")
>
>
> Having typed this, the next random thought I got was that a thing like
>
> catch Exception, e:
>     bb.fatal("Error: ", e)
>
> isn't really adding anything useful, it just "translates" the exception, logs
> its message, and then throws an obscure system exit exception instead of the
> much more useful inner exception.
>
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>
>

Re: [PATCH 1/1] bbclass: bb.fatal() clean up

[Chris Larson]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

On Wed, May 8, 2013 at 7:14 PM, Robert Yang <liezhi.yang@windriver.com>wrote:

> On 05/08/2013 08:03 PM, Mike Looijmans wrote:
>
>> On 05/08/2013 11:06 AM, Robert Yang wrote:
>>
>>> The bb.fatal() is defined as:
>>>
>>> def fatal(*args):
>>>      logger.critical(''.join(args))
>>>      sys.exit(1)
>>>
>>> So anything after bb.fatal() in the same code block doesn't have any
>>> effect, e.g.:
>>>
>>>      bb.fatal("%s_%s: %s" % (var, pkg, e))
>>>      raise e
>>>
>>> The "raise e" should be removed.
>>>
>>
>> Just some random thoughts that occurred to me when I read this:
>>
>>
> Hi Mike, thanks for your comments, but the "raise sys.exit(1)" doesn't
> raise
> anything, e.g.:
>
> import sys
>
> def fatal():
>         sys.exit(1)
>
> try:
>         raise fatal()
> except Exception as e:
>         raise e
>
> I think that the "raise fatal()" equals to "fatal()" here.


He didn't say raise sys.exit(1), he said sys.exit(1) is equivalent to raise
SystemExit(1), which it is.
-- 
Christopher Larson

[-- Attachment #2: Type: text/html, Size: 1734 bytes --]

Re: [PATCH 1/1] bbclass: bb.fatal() clean up

[Robert Yang]

On 05/09/2013 10:23 AM, Chris Larson wrote:
> On Wed, May 8, 2013 at 7:14 PM, Robert Yang <liezhi.yang@windriver.com>wrote:
>
>> On 05/08/2013 08:03 PM, Mike Looijmans wrote:
>>
>>> On 05/08/2013 11:06 AM, Robert Yang wrote:
>>>
>>>> The bb.fatal() is defined as:
>>>>
>>>> def fatal(*args):
>>>>       logger.critical(''.join(args))
>>>>       sys.exit(1)
>>>>
>>>> So anything after bb.fatal() in the same code block doesn't have any
>>>> effect, e.g.:
>>>>
>>>>       bb.fatal("%s_%s: %s" % (var, pkg, e))
>>>>       raise e
>>>>
>>>> The "raise e" should be removed.
>>>>
>>>
>>> Just some random thoughts that occurred to me when I read this:
>>>
>>>
>> Hi Mike, thanks for your comments, but the "raise sys.exit(1)" doesn't
>> raise
>> anything, e.g.:
>>
>> import sys
>>
>> def fatal():
>>          sys.exit(1)
>>
>> try:
>>          raise fatal()
>> except Exception as e:
>>          raise e
>>
>> I think that the "raise fatal()" equals to "fatal()" here.
>
>
> He didn't say raise sys.exit(1), he said sys.exit(1) is equivalent to raise
> SystemExit(1), which it is.
>

Hi Chris, thanks, if I understand correctly, what you mean is that change the
definition of bb.fatal() to let it can raise the exception "e" (not only change
the "sys.exit(1)" to "raise SystemExit(1)"), something like:

def fatal(e, *args):
     logger.critical(''.join(args))
     try:
	if e:
	    raise e # if there is e
     finally:
         # but this one will flush the previous "raise e"
         raise SystemExit(1)

it seems that this doesn't work (or do we have other ways to make it work that I
don't know?) or make much differences.

and not all the bb.fatal() has an exception, e.g.:

bb.fatal("No OUTSPECFILE")

we need change all the current bb.fatal()'s usage, is it worth ?

// Robert

Re: [PATCH 0/1] bbclass: bb.fatal() clean up

[Robert Yang]

Ping.

// Robert

On 05/08/2013 05:06 PM, Robert Yang wrote:
> The following changes since commit 9895d2c074156fee338d91aed7cfb0800477c622:
>
>    maintainers.inc: update sbc pkg maintainer (2013-05-07 13:58:26 +0100)
>
> are available in the git repository at:
>
>    git://git.pokylinux.org/poky-contrib robert/fatal
>    http://git.pokylinux.org/cgit.cgi/poky-contrib/log/?h=robert/fatal
>
> Robert Yang (1):
>    bbclass: bb.fatal() clean up
>
>   meta/classes/insane.bbclass      |    1 -
>   meta/classes/package.bbclass     |    1 -
>   meta/classes/package_rpm.bbclass |    2 --
>   3 files changed, 4 deletions(-)
>

Re: [PATCH 1/1] bbclass: bb.fatal() clean up

[Mike Looijmans]

On 05/09/2013 05:34 AM, Robert Yang wrote:
>
>
> On 05/09/2013 10:23 AM, Chris Larson wrote:
>> On Wed, May 8, 2013 at 7:14 PM, Robert Yang
>> <liezhi.yang@windriver.com>wrote:
>>
>>> On 05/08/2013 08:03 PM, Mike Looijmans wrote:
>>>
>>>> On 05/08/2013 11:06 AM, Robert Yang wrote:
>>>>
>>>>> The bb.fatal() is defined as:
>>>>>
>>>>> def fatal(*args):
>>>>>       logger.critical(''.join(args))
>>>>>       sys.exit(1)
>>>>>
>>>>> So anything after bb.fatal() in the same code block doesn't have any
>>>>> effect, e.g.:
>>>>>
>>>>>       bb.fatal("%s_%s: %s" % (var, pkg, e))
>>>>>       raise e
>>>>>
>>>>> The "raise e" should be removed.
>>>>>
>>>>
>>>> Just some random thoughts that occurred to me when I read this:
>>>>
>>>>
>>> Hi Mike, thanks for your comments, but the "raise sys.exit(1)" doesn't
>>> raise
>>> anything, e.g.:
>>>
>>> import sys
>>>
>>> def fatal():
>>>          sys.exit(1)
>>>
>>> try:
>>>          raise fatal()
>>> except Exception as e:
>>>          raise e
>>>
>>> I think that the "raise fatal()" equals to "fatal()" here.
>>
>>
>> He didn't say raise sys.exit(1), he said sys.exit(1) is equivalent to
>> raise
>> SystemExit(1), which it is.
>>
>
> Hi Chris, thanks, if I understand correctly, what you mean is that
> change the
> definition of bb.fatal() to let it can raise the exception "e" (not only
> change
> the "sys.exit(1)" to "raise SystemExit(1)"), something like:
>
> def fatal(e, *args):
>      logger.critical(''.join(args))
>      try:
>      if e:
>          raise e # if there is e
>      finally:
>          # but this one will flush the previous "raise e"
>          raise SystemExit(1)
>
> it seems that this doesn't work (or do we have other ways to make it
> work that I
> don't know?) or make much differences.
>
> and not all the bb.fatal() has an exception, e.g.:
>
> bb.fatal("No OUTSPECFILE")
>
> we need change all the current bb.fatal()'s usage, is it worth ?
>
> // Robert

I was actually more thinking like this (untested pseusocode follows):

class Fatal(SystemExit):
	def __init__(self, *args):
		SystemExit.__init__(self, 1, ''.join(*args)) # or so


def fatal(*args):
	'For backward compatibility'
	raise Fatal(*args)


New code should use "raise bb.Fatal(..)" instead of "fatal(..)". It has 
the added advantage of being able to explicitly catch and handle the 
Fatal error. Which could be useful in bitbake frontends.

Inheriting from SystemExit makes it behave exactly like the old code in 
all ways, so it wouldn't break things.

It makes it clear what happens. bb.fatal() is a function that doesn't 
really return. But it isn't as fatal as its name suggests, because it 
really just raises an exception, so anyone doing a catch or finally may 
be surprised by its implementation. Converting it into an exception 
makes it obvious to the world what it does without the need for 
documentation...

Mike.

Re: [PATCH 1/1] bbclass: bb.fatal() clean up

[Robert Yang]

On 05/13/2013 03:24 PM, Mike Looijmans wrote:
> On 05/09/2013 05:34 AM, Robert Yang wrote:
>>
>>
>> On 05/09/2013 10:23 AM, Chris Larson wrote:
>>> On Wed, May 8, 2013 at 7:14 PM, Robert Yang
>>> <liezhi.yang@windriver.com>wrote:
>>>
>>>> On 05/08/2013 08:03 PM, Mike Looijmans wrote:
>>>>
>>>>> On 05/08/2013 11:06 AM, Robert Yang wrote:
>>>>>
>>>>>> The bb.fatal() is defined as:
>>>>>>
>>>>>> def fatal(*args):
>>>>>>       logger.critical(''.join(args))
>>>>>>       sys.exit(1)
>>>>>>
>>>>>> So anything after bb.fatal() in the same code block doesn't have any
>>>>>> effect, e.g.:
>>>>>>
>>>>>>       bb.fatal("%s_%s: %s" % (var, pkg, e))
>>>>>>       raise e
>>>>>>
>>>>>> The "raise e" should be removed.
>>>>>>
>>>>>
>>>>> Just some random thoughts that occurred to me when I read this:
>>>>>
>>>>>
>>>> Hi Mike, thanks for your comments, but the "raise sys.exit(1)" doesn't
>>>> raise
>>>> anything, e.g.:
>>>>
>>>> import sys
>>>>
>>>> def fatal():
>>>>          sys.exit(1)
>>>>
>>>> try:
>>>>          raise fatal()
>>>> except Exception as e:
>>>>          raise e
>>>>
>>>> I think that the "raise fatal()" equals to "fatal()" here.
>>>
>>>
>>> He didn't say raise sys.exit(1), he said sys.exit(1) is equivalent to
>>> raise
>>> SystemExit(1), which it is.
>>>
>>
>> Hi Chris, thanks, if I understand correctly, what you mean is that
>> change the
>> definition of bb.fatal() to let it can raise the exception "e" (not only
>> change
>> the "sys.exit(1)" to "raise SystemExit(1)"), something like:
>>
>> def fatal(e, *args):
>>      logger.critical(''.join(args))
>>      try:
>>      if e:
>>          raise e # if there is e
>>      finally:
>>          # but this one will flush the previous "raise e"
>>          raise SystemExit(1)
>>
>> it seems that this doesn't work (or do we have other ways to make it
>> work that I
>> don't know?) or make much differences.
>>
>> and not all the bb.fatal() has an exception, e.g.:
>>
>> bb.fatal("No OUTSPECFILE")
>>
>> we need change all the current bb.fatal()'s usage, is it worth ?
>>
>> // Robert
>
> I was actually more thinking like this (untested pseusocode follows):
>
> class Fatal(SystemExit):
>      def __init__(self, *args):
>          SystemExit.__init__(self, 1, ''.join(*args)) # or so
>
>
> def fatal(*args):
>      'For backward compatibility'
>      raise Fatal(*args)
>
>
> New code should use "raise bb.Fatal(..)" instead of "fatal(..)". It has the
> added advantage of being able to explicitly catch and handle the Fatal error.
> Which could be useful in bitbake frontends.
>
> Inheriting from SystemExit makes it behave exactly like the old code in all
> ways, so it wouldn't break things.
>

Sounds good, this is a case for bitbake, I filed another enhancement bug for it:

https://bugzilla.yoctoproject.org/show_bug.cgi?id=4491

Let's wait for more people's comments on it.

@Saul
I think that this patch only removes the unused code, so it doesn't matter much
with how we define fatal().

// Robert

> It makes it clear what happens. bb.fatal() is a function that doesn't really
> return. But it isn't as fatal as its name suggests, because it really just
> raises an exception, so anyone doing a catch or finally may be surprised by its
> implementation. Converting it into an exception makes it obvious to the world
> what it does without the need for documentation...
>
> Mike.
>
>

Re: [PATCH 1/1] bbclass: bb.fatal() clean up

[Robert Yang]

On 05/13/2013 03:24 PM, Mike Looijmans wrote:
> On 05/09/2013 05:34 AM, Robert Yang wrote:
>>
>>
>> On 05/09/2013 10:23 AM, Chris Larson wrote:
>>> On Wed, May 8, 2013 at 7:14 PM, Robert Yang
>>> <liezhi.yang@windriver.com>wrote:
>>>
>>>> On 05/08/2013 08:03 PM, Mike Looijmans wrote:
>>>>
>>>>> On 05/08/2013 11:06 AM, Robert Yang wrote:
>>>>>
>>>>>> The bb.fatal() is defined as:
>>>>>>
>>>>>> def fatal(*args):
>>>>>>       logger.critical(''.join(args))
>>>>>>       sys.exit(1)
>>>>>>
>>>>>> So anything after bb.fatal() in the same code block doesn't have any
>>>>>> effect, e.g.:
>>>>>>
>>>>>>       bb.fatal("%s_%s: %s" % (var, pkg, e))
>>>>>>       raise e
>>>>>>
>>>>>> The "raise e" should be removed.
>>>>>>
>>>>>
>>>>> Just some random thoughts that occurred to me when I read this:
>>>>>
>>>>>
>>>> Hi Mike, thanks for your comments, but the "raise sys.exit(1)" doesn't
>>>> raise
>>>> anything, e.g.:
>>>>
>>>> import sys
>>>>
>>>> def fatal():
>>>>          sys.exit(1)
>>>>
>>>> try:
>>>>          raise fatal()
>>>> except Exception as e:
>>>>          raise e
>>>>
>>>> I think that the "raise fatal()" equals to "fatal()" here.
>>>
>>>
>>> He didn't say raise sys.exit(1), he said sys.exit(1) is equivalent to
>>> raise
>>> SystemExit(1), which it is.
>>>
>>
>> Hi Chris, thanks, if I understand correctly, what you mean is that
>> change the
>> definition of bb.fatal() to let it can raise the exception "e" (not only
>> change
>> the "sys.exit(1)" to "raise SystemExit(1)"), something like:
>>
>> def fatal(e, *args):
>>      logger.critical(''.join(args))
>>      try:
>>      if e:
>>          raise e # if there is e
>>      finally:
>>          # but this one will flush the previous "raise e"
>>          raise SystemExit(1)
>>
>> it seems that this doesn't work (or do we have other ways to make it
>> work that I
>> don't know?) or make much differences.
>>
>> and not all the bb.fatal() has an exception, e.g.:
>>
>> bb.fatal("No OUTSPECFILE")
>>
>> we need change all the current bb.fatal()'s usage, is it worth ?
>>
>> // Robert
>
> I was actually more thinking like this (untested pseusocode follows):
>
> class Fatal(SystemExit):
>      def __init__(self, *args):
>          SystemExit.__init__(self, 1, ''.join(*args)) # or so
>
>
> def fatal(*args):
>      'For backward compatibility'
>      raise Fatal(*args)
>

Hi Mike,

After more investigations, I'm sorry to say that I didn't see many benefits
of this change, the only one that you mentioned we can use "except SystemExit"
to handle the fatal error with the new code, but the "except SystemExit" also
works with the old code, e.g.:

import sys
try:
     sys.exit(1)
except SystemExit:
     print "Catch SystemExit"

And the new code uses:

SystemExit.__init__(self, 1, ''.join(*args))

which can raise more messages, but I think that we need the logger.critical()
here to log the messages in the log file, so we can only use
SystemExit.__init__(self, 1), it seems that it equals to sys.exit(1).

// Robert

>
> New code should use "raise bb.Fatal(..)" instead of "fatal(..)". It has the
> added advantage of being able to explicitly catch and handle the Fatal error.
> Which could be useful in bitbake frontends.
>
> Inheriting from SystemExit makes it behave exactly like the old code in all
> ways, so it wouldn't break things.
>
> It makes it clear what happens. bb.fatal() is a function that doesn't really
> return. But it isn't as fatal as its name suggests, because it really just
> raises an exception, so anyone doing a catch or finally may be surprised by its
> implementation. Converting it into an exception makes it obvious to the world
> what it does without the need for documentation...
>
> Mike.
>
>