#2326: Command functions in grass.script.core miss a correct error reporting --------------------------------+------------------------------------------- Reporter: wenzeslaus | Owner: grass-dev@… Type: enhancement | Status: new Priority: major | Milestone: 7.1.0 Component: Python | Version: svn-trunk Keywords: script, exceptions | Platform: All Cpu: Unspecified | --------------------------------+-------------------------------------------
Comment(by wenzeslaus): Replying to [comment:8 glynn]: > Replying to [comment:6 wenzeslaus]: > > > Here are my suggestions for changes in `grass.script.core`. > > These changes are excessive. All that is required is to check the exit code and generate an error if it is non-zero. If the child process returns a zero exit code, the existing behaviour should not be affected. > > In particular, stderr should not be captured. It isn't limited to errors (e.g. messages and percentages are written to stderr), and such information should normally be sent to the terminal as its generated. > That's true but I don't know how to solve the following case. I'm currently testing the testing framework. When I make a mistake in my testing code, an exception (`ValueError` in this case) is raised: {{{ > python -m unittest discover sandbox/wenzeslaus/gunittest ..DBMI-SQLite driver error: Error in sqlite3_prepare(): SELECT cat, YEAR_BUILTX FROM bridges no such column: YEAR_BUILTX DBMI-SQLite driver error: Error in sqlite3_prepare(): SELECT cat, YEAR_BUILTX FROM bridges no such column: YEAR_BUILTX ERROR: Column type not supported E............................ ====================================================================== ERROR: test_assertVectorFitsUnivar (testsuite.test_assertions.TestRasterMapAssertations) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/vasek/dev/grass/sandbox/wenzeslaus/gunittest/testsuite/test_assertions.py", line 80, in test_assertVectorFitsUnivar reference=V_UNIVAR_BRIDGES_WIDTH_SUBSET) File "/usr/lib/python2.7/unittest/case.py", line 475, in assertRaises callableObj(*args, **kwargs) File "/home/vasek/dev/grass/sandbox/wenzeslaus/gunittest/case.py", line 102, in assertVectorFitsUnivar reference=reference, msg=msg, sep='=') File "/home/vasek/dev/grass/sandbox/wenzeslaus/gunittest/case.py", line 66, in assertCommandKeyValue ": %s\n" % (module, ", ".join(missing))) ValueError: v.univar output does not contain the following keys provided in reference: max, mean, min, n, nmissing, nnull, range, sum ---------------------------------------------------------------------- Ran 31 tests in 1.227s FAILED (errors=1) }}} But of course the error report is misleading because the problem is not in the "key provided" or "v.univar output", the problem is that I used current version of `read_command` which does not raise. If the `read_command` function raise, I would get `ScriptError` (or whatever) with an error message saying that `v.univar` failed. But this is not enough to fix the problem. If we catch the stderr we can report what has happened. In this case I would get a message about wrong column name. However, if we will let stderr be printed to the console, we will have to search in the output for the errors which does not contain any information about command which caused them (unfortunately, in this case they are even wrong and not in GRASS format). The only option I see is to have both functions. One capturing the stderr for cases when the module (command) is used more like a function and one letting the stderr go wherever it is directed. But it don't like it because this applies at least to three functions which would create 6 different functions. Parameter, as noted bellow, might be a more acceptable solution. > Also, checking kwargs!["args"] isn't useful. If you mean `cmd = kwargs.get("args")`, this is from `Popen`, source code, I don't know what exactly they are trying to accomplish. > > In most cases, the failure to check exit codes was inherited from the original shell script. run_command() replaces "simple" command execution, read_command() replaces backticks. pipe_command() and feed_command() are used for a GRASS command at one end of a pipeline. write_command() replaces "echo data | command". > Beginning and end of the pipe line still does not convince me about usefulness of the functions. I still see them as unnecessary complication of interface. If one need something like this, he or she can use `start_commnad` directly. Direct working with `Popen` object cannot be avoided in any case. > My suggestion would be that the functions which wait for the process to terminate (run_command, read_command, write_command) should call a check_status() function, e.g. > {{{ > def check_status(p, args, kwargs): > if p.wait() == 0: > return 0 > raise CalledCommandError(p.returncode, args, kwargs) > }}} > > run_command() and write_command() would replace > {{{ > return p.wait() > }}} > with > {{{ > return check_status(p) > }}} > I don't agree with the name. It does not `check_status` it waits and then checks status, so I would suggest `_wait_check_status`. > This usage pattern would allow check_status() to be modified to provide more options regarding error handling, e.g. raise an exception, call fatal(), or just return the exit code, with the behaviour controlled by a variable or a keyword argument. > Sure, this is the way to go (`called_command_error` from comment:7 is doing something like that). > Alternatively, we could just modify the Popen wrapper to provide a modified .wait() method which does this automatically. This would probably "do the right thing" for scripts which use start_command() etc and subsequently call p.wait() themselves. I vote against. The lower level API (when using `Popen` object) should behave in the same way as Python `Popen` to allow users/programmers switch between them easily. Moreover, it would be a violation of some OOP principles, although in case of Python not so big violation, I believe. -- Ticket URL: <http://trac.osgeo.org/grass/ticket/2326#comment:9> GRASS GIS <http://grass.osgeo.org> _______________________________________________ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev