Bug 379074

Currently, there's:
$ grep -n 'sys\.exit' osc/{build,conf,core,fetch,meter}.py 
osc/build.py:54:            sys.exit(1)
osc/build.py:62:            sys.exit(1)
osc/build.py:70:            sys.exit(1)
osc/build.py:291:                    sys.exit(1)
osc/build.py:295:                    sys.exit(1)
osc/build.py:299:                sys.exit(1)
osc/build.py:396:        sys.exit(rc)
osc/conf.py:240:        #sys.exit(0)
osc/conf.py:251:        sys.exit(1)
osc/conf.py:265:            sys.exit('option %s requires an integer value' % i)
osc/core.py:256:                sys.exit(1)
osc/core.py:420:                    sys.exit(1)
osc/core.py:460:                    sys.exit(1)
osc/core.py:682:            sys.exit(1)
osc/core.py:700:            sys.exit(1)
osc/core.py:804:                sys.exit(1)
osc/core.py:940:            sys.exit(1)
osc/core.py:1006:                sys.exit(1)
osc/core.py:1010:                sys.exit(1)     
osc/core.py:1382:                    sys.exit('\n\n%s\nThe file \'%s\' could not be memory mapped. It is ' \
osc/core.py:1428:        sys.exit(1)
osc/core.py:1457:        sys.exit(1)
osc/core.py:1509:        sys.exit(1)
osc/core.py:1523:        sys.exit(1)
osc/core.py:1586:        sys.exit(1)
osc/core.py:1606:        sys.exit(1)
osc/core.py:1705:        sys.exit('unknown metatype %s' % metatype)
osc/core.py:1732:        sys.exit(1)
osc/core.py:1798:        sys.exit(1)
osc/core.py:1816:            sys.exit(1)
osc/core.py:1825:            sys.exit(1)
osc/core.py:1832:            sys.exit(1)
osc/core.py:2250:        sys.exit(1)
osc/core.py:2310:        sys.exit(1)
osc/core.py:2345:        sys.exit(1)
osc/core.py:2408:            sys.exit(1)
osc/core.py:2412:            sys.exit(1)            
osc/core.py:2422:            sys.exit(1)
osc/core.py:2426:            sys.exit(1)
osc/core.py:2717:        sys.exit(1)
osc/core.py:2729:        sys.exit(1)                         
osc/core.py:2738:        sys.exit(1)
osc/core.py:2791:        sys.exit(1)
osc/core.py:2823:        sys.exit(1)
osc/core.py:3030:        sys.exit(1)
osc/core.py:3040:        sys.exit(1)
osc/core.py:3081:                sys.exit(1)
osc/core.py:3129:            sys.exit(1)
osc/core.py:3240:            sys.exit(1)
osc/core.py:3249:                    sys.exit(1)
osc/fetch.py:89:            sys.exit(1)
osc/fetch.py:100:                sys.exit(1)
osc/fetch.py:125:                    sys.exit(0)
osc/fetch.py:163:            sys.exit(1)
osc/fetch.py:193:            sys.exit(1)

I fully agree with you. IMHO the application should decide when to stop the execution and not the library/module. But it might be a bit tricky to get rid of "sys.exit(n)". Mostly it is used with some print statements to inform the user that something unexpected happened. For example:
    print 'commit failed: package \'%s\' is missing' % pac
    sys.exit(1)

My proposal would be to raise an exception in these cases. So we could use something like "raise OSCError(error-code, message)". The advantage is that the application can decide what to do (e.g. if error-code == xy: try again...). But at the same time this also means that we have to do a lot of exception handling in the application...

Yes, printing errors to stderr is also suboptimal. Actually, I also had exceptions in mind. And yes, the application (even commandline.py itself) would then need to check virtually every call to core.py, because everything can fail when talking to the API...

Peter: what do you think about this enhancement?

I think this might come handy for the gsoc gosc project (http://en.opensuse.org/Summer_of_Code_2008#GNOME_Build_service_client) (although I don't know if someone applied for it..). IMHO it would be easier for such a client to catch a "meaningful" exception (e.g. with a message what/where/why happened) instead of catching a "SystemExit" without any information.
I think the biggest challenge is to define which information such an exception object should contain...

I agree with most of what you both wrote. In particular, I also see the
problem that meaningful exceptions are hard to come up with often,
because in case of BS quirks more than an "unspecified error" is hardly
possible. There are some case of course, in which it is clearer which
exception to raise. 

The only problem is, that if those exceptions are not handled in the
upper layer, it is of no benefit at all to add them... because the
end result will still be a crash with either a 
	"404 not found"
or a
	"500 unknown error"
which is the most frequent errors we see, right?

I also see the issue that it will be a lot of effort to add that
meaningful error handling in lots of places. In fact, I think it is
contrary to another goal: keeping the code stupid and simple, so the
hurdle for others to improve on it and extend it stays as small as
possible.

We should (continue to) take great caution when it comes to possible
data loss and security. That's definitely cases where error handling
must be done considerate and appropriately. 
But frankly, while I'm more interested in rock-solid and extensible
code, I tend to have more important things on my todo list if it comes
down to cosmetics.

Those sys.exit() calls are not good for a library, of course. That's not
cosmetics. I think a wrapping application could replace them with its
own handler, I guess, so it's not a show-stopper, but in the long term
it will probably be good to come up with a way to raise exceptions in a
systematic manner.

All in all, a good plan though. Will need a day of work or so, I guess.
Maybe more. Or how would you estimate it?

(In reply to comment #5 from Peter Poeml)
> I agree with most of what you both wrote. In particular, I also see the
> problem that meaningful exceptions are hard to come up with often,
> because in case of BS quirks more than an "unspecified error" is hardly
> possible. There are some case of course, in which it is clearer which
> exception to raise. 
> 
> The only problem is, that if those exceptions are not handled in the
> upper layer, it is of no benefit at all to add them... because the
> end result will still be a crash with either a 
>         "404 not found"
> or a
>         "500 unknown error"
> which is the most frequent errors we see, right?
> 
Hmm yes and no. You're right atm it's nearly impossible to interpret the errors
from the backend correctly because they're pretty "inconsistent". Example:

marcus@linux:~> curl -u Marcus_H -p -X GET https://api.opensuse.org/source/server:Kolasb/_meta
Enter host password for user 'Marcus_H':
<?xml version="1.0" encoding="UTF-8"?>
<status code="unknown_project">
  <summary>Unknown project 'server:Kolasb'</summary>
  <details></details>
</status>

here the summary might be usable for an informative exception but

marcus@linux:~> curl -u Marcus_H -p -X GET https://api.opensuse.org/source/server:Kolab/kolababc/_meta
Enter host password for user 'Marcus_H':
<?xml version="1.0" encoding="UTF-8"?>
<status code="unknown">
  <summary>package query "@name='kolababc' and @project='server:Kolab'" produced no results</summary>
  <details></details>
</status>

this one isn't really usable.

Nevertheless I think we aren't talking mainly about the urllib exceptions but rather about "local" errors e.g.:
- 'foobar' is not a valid project or package dir
- file xy is already under version control
- 'can\'t add package \'%s\': Object already exists'
- commit failed because...
- your working copy is out of date..
- file xy not found/cannot open xy
- etc.

My main focus is to throw exceptions in those cases because atm we just
print this to stderr or stdout and then do a "sys.exit(n)".

> I also see the issue that it will be a lot of effort to add that
> meaningful error handling in lots of places. In fact, I think it is
> contrary to another goal: keeping the code stupid and simple, so the
> hurdle for others to improve on it and extend it stays as small as
> possible.
> 
I don't know if it would really make the code more complex - basically we just replace the "print + sys.exit(n)" statements with "raise OSCError("meaningful message"...). But if we do so we also have to ensure that our OSCError exceptions are catched appropriately which will result in more lines of code. In theory it would look like this:
try:
  some_osc_cmds
except OSCError, e:
  print >>sys.stderr, e.msg
  sys.exit(1)

(so the behaviour of the cmdline client won't change)


> We should (continue to) take great caution when it comes to possible
> data loss and security. That's definitely cases where error handling
> must be done considerate and appropriately. 
> But frankly, while I'm more interested in rock-solid and extensible
> code, I tend to have more important things on my todo list if it comes
> down to cosmetics.
> 
I fully agree with you - it's just an enhancement:)

> Those sys.exit() calls are not good for a library, of course. That's not
> cosmetics. I think a wrapping application could replace them with its
> own handler, I guess, so it's not a show-stopper, but in the long term
> it will probably be good to come up with a way to raise exceptions in a
> systematic manner.
> 
What do you mean with a wrapping application? Are you talking about the cmdline client or a GUI? If so this probably won't work out because the application doesn't know why a "SystemExit" exception was issued in the core.py.

> All in all, a good plan though. Will need a day of work or so, I guess.
> Maybe more. Or how would you estimate it?
> 
Yes I think one up to two days. The "major" task is to model the exception class and map something like error codes to the appropriate error message (e.g. code 10 => "xy is not a working copy" etc.).
If the time is a show stopper for you I'm of course willing to help as I have now enough time for osc again:)

(In reply to comment #6 from Marcus Hüwe)
> I don't know if it would really make the code more complex - basically we just
> replace the "print + sys.exit(n)" statements with "raise OSCError("meaningful
> message"...). But if we do so we also have to ensure that our OSCError
> exceptions are catched appropriately which will result in more lines of code.
> In theory it would look like this:
> try:
>   some_osc_cmds
> except OSCError, e:
>   print >>sys.stderr, e.msg
>   sys.exit(1)

In the osc command line program, you only need to handle the OSCError
exceptions in osc-wrapper.py. No noticeable code growth :-).

Michal, svn up and enjoy ;-)

Ah, sorry. I was away for the last 6 days and I didn't notice the branch at all. Cool work :-)

 % grep sys.exit osc/core.py | wc -l
17

This is the current state (down from 40). I'll leave the bug open for
now, because we are going to do more work on this later. 

I suppose the state isn't too bad. Experience shows that most errors are nowadays handled where they should be handled -- in the babysitter.py wrapper script.

Nearly all remaning exits are in the local build (where it doesn't matter as much), or in esoteric commands (parsing local specfiles), or in very weird error scenarios (file cannot be memory mapped). 

So I think for normal library use, the status quo is fine. 


There are some remaining edge cases, I'll leave the bug open.

I think we can just as well close this bug. If a specific problem is
found, a new bug can be opened.