[Bug 379074] New: get rid of sys.exit() calls in osc modules
https://bugzilla.novell.com/show_bug.cgi?id=379074 Summary: get rid of sys.exit() calls in osc modules Product: openSUSE.org Version: unspecified Platform: Other OS/Version: Other Status: NEW Severity: Normal Priority: P5 - None Component: BuildService AssignedTo: poeml@novell.com ReportedBy: mmarek@novell.com QAContact: adrian@novell.com CC: mmarek@novell.com, suse-tux@gmx.de Found By: --- I think that only osc/commandline.py should call sys.exit(). Other modules should be usable by other (think GUI) applications where aborting the execution is not the best thing to do. -- Configure bugmail: https://bugzilla.novell.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug.
https://bugzilla.novell.com/show_bug.cgi?id=379074
Michal Marek
https://bugzilla.novell.com/show_bug.cgi?id=379074
User mmarek@novell.com added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c1
--- Comment #1 from Michal Marek
https://bugzilla.novell.com/show_bug.cgi?id=379074
User suse-tux@gmx.de added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c2
--- Comment #2 from Marcus Hüwe
https://bugzilla.novell.com/show_bug.cgi?id=379074
User mmarek@novell.com added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c3
--- Comment #3 from Michal Marek
https://bugzilla.novell.com/show_bug.cgi?id=379074
User suse-tux@gmx.de added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c4
--- Comment #4 from Marcus Hüwe
https://bugzilla.novell.com/show_bug.cgi?id=379074
User poeml@novell.com added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c5
--- Comment #5 from Peter Poeml
https://bugzilla.novell.com/show_bug.cgi?id=379074
User suse-tux@gmx.de added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c6
--- Comment #6 from Marcus Hüwe
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:) -- Configure bugmail: https://bugzilla.novell.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug.
https://bugzilla.novell.com/show_bug.cgi?id=379074
User mmarek@novell.com added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c7
--- Comment #7 from Michal Marek
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 :-). -- Configure bugmail: https://bugzilla.novell.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug.
https://bugzilla.novell.com/show_bug.cgi?id=379074
User poeml@novell.com added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c8
--- Comment #8 from Peter Poeml
https://bugzilla.novell.com/show_bug.cgi?id=379074
User mmarek@novell.com added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c9
--- Comment #9 from Michal Marek
https://bugzilla.novell.com/show_bug.cgi?id=379074
User poeml@novell.com added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c10
Peter Poeml
https://bugzilla.novell.com/show_bug.cgi?id=379074
User poeml@novell.com added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c11
--- Comment #11 from Peter Poeml
https://bugzilla.novell.com/show_bug.cgi?id=379074
Peter Poeml
https://bugzilla.novell.com/show_bug.cgi?id=379074
User poeml@novell.com added comment
https://bugzilla.novell.com/show_bug.cgi?id=379074#c12
Peter Poeml
participants (1)
-
bugzilla_noreply@novell.com