Skip to contentSkip to conversation

bbclass: bb.fatal() clean up

PATCH In review Robert Yang 3 participants 10 messages May 8, 2013 – Jun 17, 2013

lore
Share & tools
lore

Robert Yang

author
#0 lore raw

cover bbclass: bb.fatal() clean up

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(-)
Signature · 1 line
1.7.10.4
author
#1 lore raw

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

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 <[email protected]>
---
 meta/classes/insane.bbclass      |    1 -
 meta/classes/package.bbclass     |    1 -
 meta/classes/package_rpm.bbclass |    2 --
 3 files changed, 4 deletions(-)
meta/classes/insane.bbclass +0 -1
index 336beaa..6b696b1 100644
@@ -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(('< ', '= ', '> ', '<= ', '>=')):
meta/classes/package.bbclass +0 -1
index 1a8da40..a31e6bd 100644
@@ -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")
meta/classes/package_rpm.bbclass +0 -2
index 3a29976..edd90da 100644
@@ -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)
Signature · 1 line
1.7.10.4
#2 lore raw
Quoted 6 lines · Robert Yang

The bb.fatal() is defined as:

def fatal(*args):

     logger.critical(''.join(args))
     sys.exit(1)
Quoted 7 lines

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.

author
#3 lore raw
Quoted 10 lines · Mike Looijmans
Quoted 9 lines · 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.:

Quoted 7 lines
Quoted 4 lines
     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

Quoted 30 lines

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
[email protected]
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core

#4 lore raw
Quoted 29 lines · Robert Yang
Quoted 11 lines · Mike Looijmans
Quoted 9 lines · 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.:

Quoted 9 lines
Quoted 5 lines
     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():

Quoted 8 lines
        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.

Signature · 1 line
Christopher Larson
author
#5 lore raw
Quoted 38 lines · Chris Larson
Quoted 36 lines · Robert Yang
Quoted 11 lines · Mike Looijmans
Quoted 9 lines · 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.:

Quoted 9 lines
Quoted 5 lines
      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

Quoted 6 lines
Quoted 1 line

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

#6 lore raw
Quoted 69 lines · Robert Yang
Quoted 39 lines · Chris Larson
Quoted 36 lines · Robert Yang
Quoted 11 lines · Mike Looijmans
Quoted 9 lines · 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.:

Quoted 9 lines
Quoted 5 lines
      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

Quoted 7 lines
Quoted 1 line

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.

Quoted 7 lines

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.

author
#7 lore raw
Quoted 90 lines · Mike Looijmans
Quoted 69 lines · Robert Yang
Quoted 39 lines · Chris Larson
Quoted 36 lines · Robert Yang
Quoted 11 lines · Mike Looijmans
Quoted 9 lines · 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.:

Quoted 9 lines
Quoted 5 lines
      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

Quoted 7 lines
Quoted 1 line

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.

Quoted 7 lines

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)

Quoted 7 lines

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

Quoted 9 lines

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.

author
#8 lore raw
In reply to Mike Looijmans
Quoted 80 lines · Mike Looijmans
Quoted 69 lines · Robert Yang
Quoted 39 lines · Chris Larson
Quoted 36 lines · Robert Yang
Quoted 11 lines · Mike Looijmans
Quoted 9 lines · 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.:

Quoted 9 lines
Quoted 5 lines
      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

Quoted 7 lines
Quoted 1 line

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.

Quoted 7 lines

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):

Quoted 9 lines

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

Quoted 17 lines

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.

author
#9 lore raw
In reply to Robert Yang

Ping.

// Robert

Quoted 17 lines · 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(-)