#2326 closed enhancement (fixed)
Command functions in grass.script.core miss a correct error reporting
| Reported by: | wenzeslaus | Owned by: | |
|---|---|---|---|
| Priority: | major | Milestone: | 7.2.0 |
| Component: | Python | Version: | svn-trunk |
| Keywords: | script, exceptions | Cc: | |
| CPU: | Unspecified | Platform: | All |
Description
There is a lot of functions in grass.core which can run a GRASS module. One is allowing this the other that but none of them is actually providing the convenient interface to a (the most?) common case where you not only want stdout as a string (and perhaps stdin as a string too) but also you want an error message to be reported in program(mer)-friendly way.
The later is actually a motivation for this ticket because I see this as a critical issue which is very dangerous for beginners (writing scripts with run_command or others) and not checking the return code or stderr with an expectation that the function will report the error (in Python sense, thus raising an exception). And this issue is valid also for advanced GRASS Python programmers/users because there is a need to still check output of each command and report error manually. Moreover, the current state goes against the philosophy of C library which takes the burden of dealing with errors from the programmer.
The fact is that you then and up with implementing your own start_command wrappers. For example, animation tool uses its own function based on start_command returning return code (int), stdout (str), stderr (str):
def read2_command(*args, **kwargs):
kwargs['stdout'] = gcore.PIPE
kwargs['stderr'] = gcore.PIPE
ps = gcore.start_command(*args, **kwargs)
stdout, stderr = ps.communicate()
return ps.returncode, stdout, stderr
I recently used this code inside some function which gets stdin as string, uses stdout and in case of non-zero return code throws/raises an exception (RuntimeError) with error message containing module name and stderr:
proc = gcore.start_command('m.proj', input='-', separator=' , ',
flags='od',
stdin=gcore.PIPE,
stdout=gcore.PIPE,
stderr=gcore.PIPE)
proc.stdin.write(proj_in)
proc.stdin.close()
proc.stdin = None
proj_out, errors = proc.communicate()
if proc.returncode:
raise RuntimeError("m.proj error: %s" % errors)
I would probably just commit the combination of the code samples above as a new function but I want to be sure that it will be right this time. I don't know whether my requirements are the same of the majority and finally, I don't know what name to choose for the new function since it seems that functions in grass.script.core already used every possible name. Also, I'm not sure what is the PyGRASS's answer to this problem.
Attachments (4)
Change History (45)
follow-up: 10 comment:1 by , 10 years ago
follow-up: 3 comment:2 by , 10 years ago
Replying to wenzeslaus:
Also, I'm not sure what is the PyGRASS's answer to this problem.
At the moment in the Module class the returncode is not checked at all.
>>> from subprocess import PIPE
>>> from grass.pygrass.modules import Module
>>>
>>> greg = Module('g.region', flags='p', rast='ELEV', stdout_=PIPE, stderr_=PIPE)
>>> greg.popen.returncode
1
>>> greg.outputs.stderr
'ERROR: Raster map <ELEV> not found\n'
I can modify the Module class behaviour changing the default parameters for stdout_ and stderr_ to PIPE (at the moment are None), and raise a RuntimeError if the returncode is not 0.
comment:3 by , 10 years ago
Replying to zarch:
I can modify the Module class behaviour changing the default parameters for stdout_ and stderr_ to PIPE
That's probably unwise. It would force the caller to either explicitly set them to None or to consume the output, e.g. via the .communicate() method.
I wouldn't be surprised if many scripts do neither; the result being that the call works fine unless it writes more than a buffer's worth of output, in which case it will just hang.
I also wouldn't be surprised if scripts try to re-implement logic similar to .communicate() but get it wrong. You need to use either threads or non-blocking I/O. If you perform a blocking read on either pipe and more than a buffer's worth of data is written to the other pipe, you get deadlock (i.e. the script hangs).
Also, when both stdout and stderr are associated with different pipes, it's impossible to reconstruct the "normal" output because there's no way to determine the relative order. Meaning that you can't e.g. associate an error message with any particular line of progress output. This is why the support for "stdout=PIPE, stderr=STDOUT" exists.
comment:4 by , 10 years ago
Replying to wenzeslaus:
I missed this issue in my previous reply:
proc.stdin.write(proj_in) proc.stdin.close() proc.stdin = None proj_out, errors = proc.communicate()
You need to use
proj_out, errors = proc.communicate(proj_in)
If you try to .write() the data to stdin, and the child process prints progress/error/etc messages as it processes the input, you'll get deadlock if the amount of data written to either pipe exceeds the amount which will fit in the pipe's buffer.
The .communicate() method uses either non-blocking I/O (Unix) or threads (Windows) in order to consume output as it becomes available, rather than waiting until all data has been written to stdin and hoping that the pipe's buffer can contain everything written to stdout/stderr in the meantime.
comment:5 by , 10 years ago
| Priority: | normal → major |
|---|
Here is the usage of *_command functions from grass.script.core and how their output is checked. The statistics is alarming, so raising priority.
| function | checks rc | does not check |
run_command | 109+33 | 285+501 |
write_command | 7+2 | 17+33 |
Both run_command (just runs the command synchronously) and write_command (puts string to stdin) returns a subprocess return code but most of the code does not check it (0.7 in trunk and 0.9 in addons). We must expect that user scripts might be same or worse than addons.
| function | usages |
start_command | 4+2 |
pipe_command | 24+1 |
feed_command | 18+1 |
read_command | 119+76 |
parse_command | 66+9 |
For start_command, pipe_command and feed_command it is hard to tell if the return code is checked without really looking to the source code.
In case of read_command (returns stdout as string) and parse_command (applies function to read_command result) we know that the return code is not checked since the function should do this and it does not. In case of parse_command the error might be at least generated during the parsing. In case of read_command, the standard output might checked too. In case the checking accepts things like empty sting or None or no error is generated during parsing, it is completely the same as with run_command.
Note that results are approximate, e.g., (grass\.)? might not be completely correct. Used grep commands:
grep --exclude="*svn*" --exclude-dir="*dist.*" -IrnE '(grass\.)?run_command' . | grep -E "(if | = ).*run_command" grep --exclude="*svn*" --exclude-dir="*dist.*" -IrnE '^\s*(grass\.)?run_command' . grep --exclude="*svn*" --exclude-dir="*dist.*" -IrnE 'pipe_command' . grep --exclude="*svn*" --exclude-dir="*dist.*" -IrnE 'feed_command' . grep --exclude="*svn*" --exclude-dir="*dist.*" -IrnE 'read_command' . grep --exclude="*svn*" --exclude-dir="*dist.*" -IrnE 'parse_command' . grep --exclude="*svn*" --exclude-dir="*dist.*" -IrnE 'write_command' . | grep -v "test_rcategory_doctest" | grep -E '(if | = ).*write_command' grep --exclude="*svn*" --exclude-dir="*dist.*" -IrnE 'write_command' . | grep -v "test_rcategory_doctest" | grep -vE '(if | = ).*write_command'
follow-ups: 7 8 comment:6 by , 10 years ago
| Keywords: | exceptions added |
|---|
Here are my suggestions for changes in grass.script.core.
The read_command function is similar to subprocess.check_output function, so I used the implementation from Python subprocess (cpython: Lib/subprocess.py). It check the stderr parameter, uses communicate() and poll() and raises on non-zero return code. The main difference from subprocess.check_output (expect from GRASS parameter handing) is that stderr is stored to exception to provide a detailed message at the right place.
def read_command(*args, **kwargs):
"""Run command and return its output."""
# implemenation inspired by subprocess.check_output() function
if 'stderr' in kwargs:
raise ValueError('stderr argument not allowed, it would be overridden.')
kwargs['stderr'] = PIPE
process = pipe_command(*args, **kwargs)
output, errors = process.communicate()
returncode = process.poll()
if returncode:
cmd = kwargs.get("args")
if cmd is None:
cmd = args[0]
raise CalledCommandError(returncode, cmd, errors=errors)
return output
Note that read_command() is used by parse_command() function and that pipe_command() might be replaced by start_command() and stdout=PIPE.
write_command() is similar to read_command() but without stdin and win stdout as a string:
def write_command(*args, **kwargs):
"""!Passes all arguments to feed_command, with the string specified
by the 'stdin' argument fed to the process' stdin.
"""
# implemenation inspired by subprocess.check_output() function
if 'stdin' not in kwargs:
raise ValueError('stdin argument is required.')
if 'stderr' in kwargs:
raise ValueError('stderr argument not allowed, it would be overridden.')
stdin = kwargs['stdin'] # get a string for stdin
kwargs['stdin'] = PIPE # to be able to send data to stdin
kwargs['stderr'] = PIPE
process = start_command(*args, **kwargs)
unused, errors = process.communicate(stdin)
returncode = process.poll()
if returncode:
cmd = kwargs.get("args")
if cmd is None:
cmd = args[0]
raise CalledCommandError(returncode, cmd, erros=errors)
# subprocess.check_call() function returns 0 for success
# but this would create only confusion since we don't return for failure
write_read_command() is a new proposed function which is a combination of write_command() and read_command() functions.
def write_read_command(*args, **kwargs):
"""Uses stdin string as stdin and returns stdout"""
# implemenation inspired by subprocess.check_output() function
if 'stdin' not in kwargs:
raise ValueError('stdin argument is required.')
if 'stdout' in kwargs:
raise ValueError('stdout argument not allowed, it would be overridden.')
if 'stderr' in kwargs:
raise ValueError('stderr argument not allowed, it would be overridden.')
stdin = kwargs['stdin'] # get a string for stdin
kwargs['stdin'] = PIPE # to be able to send data to stdin
kwargs['stdout'] = PIPE
kwargs['stderr'] = PIPE
process = start_command(*args, **kwargs)
output, errors = process.communicate(stdin)
returncode = process.poll()
if returncode:
cmd = kwargs.get("args")
if cmd is None:
cmd = args[0]
raise CalledCommandError(returncode, cmd, erros=errors)
return output
I'm not sure with limitations of write_command(), read_command() and write_read_command() considering large inputs and outputs but they are using out, err = p.communicate(stdin) and p.poll() which should be the best way according to what Glynn is saying, library (subprocess) is using, and documentation is suggesting. Moreover, they are not worse that the existing functions and I expect that for special cases you need to use start_command() directly anyway.
The missing function is now a function which would connect stdout and stderr (stdout=PIPE, stderr=STDOUT). But this is again something special (e.g. used in GUI) which might not need a convenient function and user should use start_command() directly.
Considering how run_command() function is used I would suggest to change it to raise an exception. This will break the code which was using it correctly and this code will have to be fixed manually and perhaps adding try-except will be necessary. However, most of the code will improve since it will at least fail with proper error message at proper place and not few lines further with hard-to-understand message. Adding return 0 to the end of the function may provide a compatibility with previous behavior for cases when the return value was used. It will not create an error when everything is OK but it will not use the error handling at that place when the command fails. At the end, return 0 should be removed anyway.
- ps = start_command(*args, **kwargs)
- return ps.wait()
+ if 'stdin' not in kwargs:
+ raise ValueError('stdin argument is required.')
+ else:
+ stdin = kwargs['stdin']
+ del kwargs['stdin'] # clean non-standard argument
+ process = feed_command(*args, **kwargs)
+ unused, errors = process.communicate(stdin)
+ returncode = process.poll()
+ if returncode:
+ cmd = kwargs.get("args")
+ if cmd is None:
+ cmd = args[0]
+ raise CalledCommandError(returncode, cmd, erros=errors)
+ return 0
I don't see particular advantage in functions pipe_command() (sets kwargs['stdout'] = PIPE) and feed_command() (sets kwargs['stdin'] = PIPE). The the parameters can be set directly with the function start_command() and for me the functions does not contribute to readability because the feed does not tell me if the string is the input (as for write_command() function) or it justs sets stdin=PIPE. Similarly, pipe does not tell me if everything is piped (stdin, stdout and stderr), stdin and stdout are piped (as for write_read_command() function) or just one of stdin and stdout is piped. It is even more confusing name when I consider piping in command line where I connect multiple processes (which is not done by PIPE constant in subprocess).
I wanted to change (instead of adding) the functions, especially run_command(), originally but then I was afraid of problems with releasing 7.0 and changing API. However, the usage statistics and the fact that Glynn suggested the same convinced me that we should really do it. However, it is challenging, we have to change some code (at least 150 occurrences of run_command() and write_command() in trunk and addons but maybe also other functions) and I'm still not sure with some detail such as if stderr should be always captured to get the right error message (consider e.g. the result with DEBUG=1).
Another consideration is raising exception versus calling fatal() function which can be set to raise exception but this exception would have to contain everything in a message while a custom exception (I used one derived from subprocess.CalledProcessError) can store return code, command (or command/module name) and stderr separately.
For the 7.0 this actually should not be applied completely but the API should be changed to fit the improved implementation/new API, so run_command() function should have return 0 but a waring in documentation. Then perhaps pipe_command() and feed_command() functions should be marked as depreciated.
Adding (not tested) draft of proposed changes.
by , 10 years ago
| Attachment: | command_fun_raise.diff added |
|---|
Draft of proposed changes: run, write, read raises; write_read added (not tested, comment:6)
comment:7 by , 10 years ago
Replying to wenzeslaus:
Another consideration is raising exception versus calling
fatal()function which can be set to raise exception but this exception would have to contain everything in a message while a custom exception (I used one derived from subprocess.CalledProcessError) can store return code, command (or command/module name) and stderr separately.
To take advantage of custom exception with additional info and keeping fatal() with exit as a default behavior we can use the following function in the *_command functions.
def called_command_error(cmd, returncode, errors)
global raise_on_error
if raise_on_error:
raise CalledCommandError(returncode, cmd, erros=errors)
# the same as CalledCommandError.__str__ is doing
msg = _("Command '{cmd}' returned non-zero exit status {rc}"
" and the following errors.\n"
"%{errors}").format(cmd=cmd, rc=returncode,
errors=errors)
# fatal function will test raise_on_error again
# the alternative is to mirror code from fatal function completely
# which would be: error(msg); sys.exit(1)
fatal(msg)
follow-up: 9 comment:8 by , 10 years ago
Replying to 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.
Also, checking kwargs["args"] isn't useful.
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".
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)
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.
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.
follow-up: 13 comment:9 by , 10 years ago
Replying to glynn:
Replying to 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.
comment:10 by , 10 years ago
When trying to implement new version of what we are talking about I run into the following problem. G7:g.message which is called by core.fatal exits with non-zero return code.
# test runs of g.message > g.message "no flag"; echo "$?" no flag 0 > g.message -v "flag -v"; echo "$?" 0 > g.message -i "flag -i"; echo "$?" flag -i 0 > g.message -w "flag -w"; echo "$?" WARNING: flag -w 0 > g.message -e "flag -e"; echo "$?" ERROR: flag -e 1
This causes run_command which is used to call g.message to call core.fatal and infinite recursion goes on. g.message calls G_fatal_error which in causes the exit with return code 1.
This can be fixed in core.fatal (core.error) for example by using start_command directly. However, the behavior of g.message is not expected. When it successfully prints the error, it should end successfully. Wouldn't be better to change g.message to return 0 with -e flag?
by , 10 years ago
| Attachment: | command_fun_exit_raise_return.diff added |
|---|
Second draft of proposed changes: run, write, read raises or exists; run can return; write_read added (functions tested)
comment:11 by , 10 years ago
I've added new patch. Functions call one of the following error handing functions:
def _called_command_error(returncode, args, kwargs):
msg = _("Command %s %s returned non-zero exit status %s") % (args, kwargs, returncode)
if _RAISE_ON_COMMAND_ERROR:
raise CalledCommandError(msg, returncode, args, kwargs)
fatal(msg) # this exits or raises
def _called_command_return(returncode, args, kwargs):
if _RETURN_ON_COMMAND_ERROR:
return returncode
elif returncode:
_called_command_error(returncode, args, kwargs)
Examples of functions:
# we can return return code, raise or exit
def run_command(*args, **kwargs):
ps = start_command(*args, **kwargs)
returncode = ps.wait()
return _called_command_return(returncode, args, kwargs)
# we want to always return output here, so raise or exit
def read_command(*args, **kwargs):
process = pipe_command(*args, **kwargs)
output = process.communicate()[0]
returncode = process.poll()
if returncode:
_called_command_error(returncode, args, kwargs)
return output
- I don't call
wait()orpoll()in the checking method, this is done in the functions in different (appropriate) ways. - There is no interface yet for the global variables which controls if the functions will raise, exit/fatal or return (if applicable).
- I'm using
message(msg, flag='w')instead ofmessage(msg, flag='w')to avoid issue described in comment:10. - Besides global variables there may or probably should be also a parameter which will apply to individual functions. But I still have a feeling that I would like to use special function, no a function with parameter.
- Functions
read_commandandwrite_read_commandcapture stdout, so I don't see a reason to not capture also stderr and print it (perhaps begging and end) in the error message, except for inconsistency with other functions. Again I still have a feeling that I want this also for other functions. - I was looking to gui/wxpython/core/gcmd.py and I'm wondering why there is so much code around
subprocess.Popencall and why it is not needed inscript.core. - I'm still thinking how to include this changes into trunk and 7_0. It is a change of API and it requires to go to almost 200 places in trunk and addons (see comment:5).
Download the test, e.g. to /lib/python/script/testsuite directory, and run it (inside GRASS session):
python -m unittest discover lib/python/script/testsuite
follow-up: 14 comment:12 by , 10 years ago
To start GUI with the changes I had to add
import grass.script.core as gcore gcore._RAISE_ON_COMMAND_ERROR = True
to wxgui.py which is expected but to make GUI start I had to add try-except to core.find_file():
def find_file(name, element='cell', mapset=None):
if element == 'raster' or element == 'rast':
verbose(_('Element type should be "cell" and not "%s"') % element)
element = 'cell'
try:
s = read_command("g.findfile", flags='n', element=element, file=name,
mapset=mapset)
except CalledCommandError:
return {'name': ''}
return parse_key_val(s)
The try-except makes sense only when _RAISE_ON_COMMAND_ERROR = True, so I would say that it should not be there. It's sure that function find_file should not cause exit or raise when the file was not found. It should return False or perhaps None in this case.
It seems that the fact that you cannot rely on the interface of the functions hardens the implementation of new functions which are supposed to be general. This disqualifies the global switching from being usable.
I think that this is different from raise_on_error which changes behavior of core.fatal(). GRASS modules/program calls in general behaves in in different ways considering error codes from error states in functions. They are generally less predictable, although GRASS modules in most of the cases behaves quite nicely and translating error code to exception raise makes sense.
Considering these difficulties (g.findfile, g.message and number of usages) it seems to me that the functions (their default behavior) cannot be changed before the 7.0 release and also that global settings _RAISE_ON_COMMAND_ERROR and _RETURN_ON_COMMAND_ERROR should not be part of API and should not be used except for some testing purposes.
Thus, I now think that the best solution is to create new functions (in different module but based on start_command) or to add a parameter to existing functions but something tells me that I will anyway create a wrapper function with the parameter set and creating new functions all over again was what was motivation for this ticket.
Appendix: G7:g.findfile behavior with not existing map is to produce output with empty values and return code 1. find_file ignores return code and caller checks if some of the values is an empty string (or None).
> g.findfile -n element=cell file=aaabbbcc; echo $? name= mapset= fullname= file= 1
Appendix: Usage of raise_on_error. The usage in GUI seems to be not well settled.
$ cd gui/wxpython
$ grep --color=auto --exclude-dir={.svn,.git,.OBJ,locale,dist.*} -IrnE "raise_on"
animation/dialogs.py:1646: gcore.set_raise_on_error(True)
animation/frame.py:46:gcore.set_raise_on_error(True)
iclass/g.gui.iclass.py:120: grass.set_raise_on_error(False)
vdigit/g.gui.vdigit.py:84: grass.set_raise_on_error(False)
psmap/dialogs.py:63:# grass.set_raise_on_error(True)
psmap/frame.py:1056: grass.set_raise_on_error(False)
dbmgr/g.gui.dbmgr.py:43: grass.set_raise_on_error(False)
follow-up: 15 comment:13 by , 10 years ago
Replying to wenzeslaus:
That's true but I don't know how to solve the following case.
I'm currently testing the testing framework.
Your testing framework probably shouldn't be using functions designed for normal scripts. Conversely (and more importantly), features which are necessary or useful for a testing framework shouldn't be imposed upon normal scripts.
Write your own "run_command" function atop grass.script.start_command(), and use that.
Also, checking kwargs["args"] isn't useful.
If you mean
cmd = kwargs.get("args"), this is fromPopen, source code, I don't know what exactly they are trying to accomplish.
Python's Popen() is at a lower level than the grass.script functions. The latter don't accept an "args" parameter, but use grass.script.make_command to generate the argument list from the keyword arguments.
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_commnaddirectly.
All of the functions (except for exec_command) can be replaced by start_command, as all of those functions are implemented using start_command. They exist as convenience interfaces for the most common cases (e.g. being able to use "data = read_command(...)" rather than having to explicitly manage a Popen object).
The cases of a single pipe to either stdin or stdout are simpler (for the caller) than those which require multiple pipes, as they avoid the potential for deadlock. This is why Unix' popen() lets you read stdout or write stderr but not both.
These interfaces could perhaps be simplified further by supporting the context manager interface, so that scripts could use e.g.
with grass.script.pipe_command(...) as p:
for line in p.stdout:
# whatever
This would avoid the need for an explicit check of the exit code (the check would be performed in the context manager's __exit__ method).
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.
I don't agree with the name. It does not
check_statusit waits and then checks status, so I would suggest_wait_check_status.
The "status" in question is the process' exit status, which doesn't exist until the process has terminated. How about check_exit_status()?
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
Popenobject) should behave in the same way as PythonPopento 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.
Another alternative is the same thing under a different name, e.g. .finish(). This would still require modifying existing scripts to use that rather than .wait(), but at least it keeps it as a method rather than a function.
comment:14 by , 10 years ago
Replying to wenzeslaus:
Thus, I now think that the best solution is to create new functions (in different module but based on
start_command) or to add a parameter to existing functions but something tells me that I will anyway create a wrapper function with the parameter set and creating new functions all over again was what was motivation for this ticket.
I suggest adding an extra keyword parameter to control error checking. The default should be to generate an error (raise an exception or call fatal()) for a non-zero exit code.
The value should be stored in the grass.script.Popen() object so that non-blocking functions (start_command, pipe_command, feed_command) behave consistently with blocking functions (run_command, read_command, write_command). I.e. the error-checking behaviour would always be specified in the call which creates the process regardless of whether that call waits for termination or returns a Popen object.
comment:15 by , 10 years ago
Replying to glynn:
Replying to wenzeslaus:
That's true but I don't know how to solve the following case.
I'm currently testing the testing framework.
Your testing framework probably shouldn't be using functions designed for normal scripts. Conversely (and more importantly), features which are necessary or useful for a testing framework shouldn't be imposed upon normal scripts.
Write your own "run_command" function atop grass.script.start_command(), and use that.
This is what I wanted to avoid, writing some other wrappers around start_command() again and again. But now I think that at least this time, for now, it it the best option. So, here they are source:sandbox/wenzeslaus/gunittest/gmodules.py?rev=60979
The most interesting function is call_module (which might replace some of the other functions):
def call_module(module, stdin=None, merge_stderr=False, **kwargs):
if stdin:
kwargs['stdin'] = subprocess.PIPE # to be able to send data to stdin
elif 'input' in kwargs and kwargs['input'] != '-':
raise ValueError(_("stdin must be set when input='-'"))
if 'stdout' in kwargs:
raise ValueError(_("stdout argument not allowed, it would be overridden"))
if 'stderr' in kwargs:
raise ValueError(_("stderr argument not allowed, it would be overridden"))
kwargs['stdout'] = subprocess.PIPE
if merge_stderr:
kwargs['stderr'] = subprocess.STDOUT
else:
kwargs['stderr'] = subprocess.PIPE
process = start_command(module, **kwargs)
# input=None means no stdin (our default)
# for stderr=STDOUT errors in None which is fine for CalledModuleError
output, errors = process.communicate(input=stdin)
returncode = process.poll()
if returncode:
raise CalledModuleError(returncode, module, kwargs, errors)
return output
I should probably add to the documentation that you should not use it for huge I/O. If stdin is used it feeds the process with it (I should add a check if it is a string). It always returns stdout. By default it captures stderr (to be used in a exception) and with merge_stderr=True it redirects stderr to stdout.
I made an experiment and used the word "module" rather than "command".
About your comment:14, it makes sense. I can try to implement it some time later. But we probably cannot apply this to 7 since the change might be too invasive as I realized. But it would be good to ensure right usages in the future of 7, so I don't know what to do.
follow-up: 17 comment:16 by , 10 years ago
In r60993 I have added more parameters to call_module() function, so the interface is more complicated but I expect (me) to use the defaults most of the time and just add one or two parameters time to time. The other functions (run_, write_, read_) are not so needed now, I may consider removing them. Or at least reimplementing them using the general call_module() function.
I hope that using communicate() will be safe most of the time. They say that The data read is buffered in memory, so do not use this method if the data size is large or unlimited. So, I should note this limitation in call_module() too. Otherwise, they seem to believe that if you use communicate(), it is safe (looking to the documentation and source code). What makes me not sure are the things like source:grass/trunk/gui/wxpython/core/gcmd.py?rev=60817#L553 and some discussions on the internet.
comment:17 by , 10 years ago
Replying to wenzeslaus:
They say that The data read is buffered in memory, so do not use this method if the data size is large or unlimited.
If the data would otherwise be sent to the terminal, memory consumption won't be an issue. But there are a few modules which can produce vast amounts of data on stdout (e.g. r.out.ascii, r.stats).
Apart from memory consumption, many modules print progress output, and you want that to appear as it's generated, not all at once when the module terminates.
follow-up: 19 comment:18 by , 10 years ago
What do you (all) think about moving the function call_module() from testing framework into the grass.script.core module?
The main point of this function is that it raises exception when return code of the module is non-zero. Additionally, it provides an convenient interface for capturing stdout and stderr and also for (optional) providing stdin (as string, or stream). It uses the safest possible way to achieve this which is Popen.communicate() method.
By default it captures stdout (stdout=PIPE) and returns it as string and captures stderr (stderr=PIPE) and puts it into the exception when return code was non-zero. This can be disabled or stderr can be merged to stdout (stderr=STDOUT).
The function is universal and it can be used to implement the other alternatives (run_, write_, and read_).
We can change the name to call_command() to be consistent with other functions or we can use call_module() to emphasize that it have different interface.
It raises exception derived from subprocess's CalledCommandError. But there is not special need for that. This is just for consistency with subprocess's check_call function which is not used by call_command or in GRASS.
It has tests to prove that it behaves how it should.
This of course does not solve the problems of wrong usage of run_command and it does not enforce usage of fatal error. However, it allows to write new code for libraries and scripts in a better, Python-friendly, way. The usage for this function is where the module is used more like a function then as a subprocess, in this case we don't care much about module progress or messages unless there was an error. Typical usage are the modules providing some stdout with key-value pairs.
follow-up: 20 comment:19 by , 10 years ago
Replying to wenzeslaus:
What do you (all) think about moving the function
call_module()from testing framework into thegrass.script.coremodule?
My concern is that people might use it when the existing commands (once the error handling is fixed) would be more appropriate.
The main point of this function is that it raises exception when return code of the module is non-zero.
The existing commands should be changed to do that.
Additionally, it provides an convenient interface for capturing stdout and stderr and also for (optional) providing stdin (as string, or stream). It uses the safest possible way to achieve this which is
Popen.communicate()method.By default it captures stdout (stdout=PIPE) and returns it as string
This shouldn't be the default. The default should be to inherit the script's stdout. And if capturing stdout is the only feature required (compared to run_command), read_command should be used.
and captures stderr (stderr=PIPE) and puts it into the exception when return code was non-zero.
This should never be done for normal scripts. Child process should inherit the script's stderr.
follow-ups: 21 24 comment:20 by , 10 years ago
Replying to glynn:
Replying to wenzeslaus:
What do you (all) think about moving the function
call_module()from testing framework into thegrass.script.coremodule?My concern is that people might use it when the existing commands (once the error handling is fixed) would be more appropriate.
The main point of this function is that it raises exception when return code of the module is non-zero.
The existing commands should be changed to do that.
This is a serious change which will require a lot of testing in both modules (core and addons) and GUI. What are the opinions on doing something like that?
The alternative is to go for 7.x and probably even further (8.x) with the current unsafe but broadly used API which can be used right in most of the cases (but not e.g. in case of read_command()). We may provide better alternatives and propose them when they are ready. We can also do some static source code checks (using regexp) which would look for using the return values of run_command() etc.
I kind of tend to the conservative alternative since it is less work over longer period and I cannot work on the ultimate solution now at least not at the level and focus it deserves.
follow-up: 22 comment:21 by , 10 years ago
Replying to wenzeslaus:
Replying to glynn:
Replying to wenzeslaus:
What do you (all) think about moving the function
call_module()from testing framework into thegrass.script.coremodule?My concern is that people might use it when the existing commands (once the error handling is fixed) would be more appropriate.
The main point of this function is that it raises exception when return code of the module is non-zero.
The existing commands should be changed to do that.
This is a serious change which will require a lot of testing in both modules (core and addons) and GUI. What are the opinions on doing something like that?
I think we should move on this direction.
The alternative is to go for 7.x and probably even further (8.x) with the current unsafe but broadly used API which can be used right in most of the cases (but not e.g. in case of
read_command()). We may provide better alternatives and propose them when they are ready. We can also do some static source code checks (using regexp) which would look for using the return values ofrun_command()etc.
To me it looks more dangerous to not check the return code than raise an exception. Since until now there are not check the author assume that there will be not problem so if this condition is true, it will still true after this change, so there will be not new problems.
We have only to change where the return code is checked, but fortunately, as you pointed out, this is quite rare case so I think should be feasible for grass7.0
the file that need to be changed in that case are mainly the temporal framework and the gui/wypython:
$ grep --color=auto --exclude-dir={.svn,.git,.OBJ,locale} -IrnE "returncode"
lib/python/pygrass/tools/multi_import.py:54: return path, name, False if popen.returncode else True
lib/python/pygrass/tools/multi_import2.py:25: ... print(mapcalc.popen.returncode)
lib/python/pygrass/modules/interface/module.py:57: ... print(mapcalc.popen.returncode)
lib/python/pygrass/modules/interface/module.py:144: if proc.popen.returncode != 0:
lib/python/pygrass/modules/interface/module.py:208: >>> mapcalc.popen.returncode
lib/python/pygrass/modules/interface/module.py:216: >>> colors.popen.returncode
lib/python/pygrass/modules/interface/module.py:230: >>> colors.popen.returncode
lib/python/pygrass/modules/interface/module.py:241: >>> colors.popen.returncode
lib/python/pygrass/modules/interface/module.py:251: >>> colors.popen.returncode
lib/python/pygrass/modules/interface/module.py:583: raise CalledModuleError(returncode=self.popen.returncode,
lib/python/gunittest/gmodules.py:30: >>> mapcalc.popen.returncode
lib/python/gunittest/gmodules.py:37: >>> colors.popen.returncode
lib/python/gunittest/gmodules.py:129: returncode = process.poll()
lib/python/gunittest/gmodules.py:130: if returncode:
lib/python/gunittest/gmodules.py:131: raise CalledModuleError(returncode, module, kwargs, errors)
lib/python/gunittest/case.py:954: raise CalledModuleError(module.popen.returncode, module.name,
lib/python/gunittest/case.py:978: # TODO: do we need special function for testing module failures or just add parameter returncode=0?
lib/python/gunittest/case.py:1012: ' with non-zero return code ({m.popen.returncode})\n'
lib/python/gunittest/invoker.py:43:def update_keyval_file(filename, module, returncode):
lib/python/gunittest/invoker.py:60: keyval['status'] = 'failed' if returncode else 'passed'
lib/python/gunittest/invoker.py:61: keyval['returncode'] = returncode
lib/python/gunittest/invoker.py:181: returncode = p.wait()
lib/python/gunittest/invoker.py:188: module=module, returncode=returncode)
lib/python/gunittest/invoker.py:190: returncode=returncode,
lib/python/gunittest/reporters.py:390: def end_file_test(self, returncode, **kwargs):
lib/python/gunittest/reporters.py:394: if returncode:
lib/python/gunittest/reporters.py:455:def returncode_to_html_text(returncode):
lib/python/gunittest/reporters.py:456: if returncode:
lib/python/gunittest/reporters.py:464:def returncode_to_html_sentence(returncode):
lib/python/gunittest/reporters.py:465: if returncode:
lib/python/gunittest/reporters.py:467: ' Test failed (return code %d)' % (returncode))
lib/python/gunittest/reporters.py:470: ' Test succeeded (return code %d)' % (returncode))
lib/python/gunittest/reporters.py:473:def returncode_to_success_html_par(returncode):
lib/python/gunittest/reporters.py:474: if returncode:
lib/python/gunittest/reporters.py:608: def end_file_test(self, module, cwd, returncode, stdout, stderr,
lib/python/gunittest/reporters.py:611: module=module, cwd=cwd, returncode=returncode,
lib/python/gunittest/reporters.py:655: status=returncode_to_html_text(returncode),
lib/python/gunittest/reporters.py:674: status=returncode_to_success_html_par(returncode),
lib/python/gunittest/reporters.py:691: status=returncode_to_html_text(returncode),
lib/python/gunittest/reporters.py:693: ptests=pass_per, rc=returncode,
lib/python/gunittest/reporters.py:734: if returncode:
lib/python/gunittest/reporters.py:741: if returncode:
lib/python/gunittest/reporters.py:774: self.files_returncodes = []
lib/python/gunittest/reporters.py:798: summary['files_returncodes'] = [str(item)
lib/python/gunittest/reporters.py:799: for item in self.files_returncodes]
lib/python/gunittest/reporters.py:833: def end_file_test(self, module, cwd, returncode, stdout, stderr,
lib/python/gunittest/reporters.py:836: module=module, cwd=cwd, returncode=returncode,
lib/python/gunittest/reporters.py:868: self.files_returncodes.append(returncode)
lib/python/gunittest/reporters.py:923: def end_file_test(self, module, cwd, returncode, stdout, stderr,
lib/python/gunittest/reporters.py:926: module=module, cwd=cwd, returncode=returncode,
lib/python/gunittest/reporters.py:929: if returncode:
lib/python/gunittest/reporters.py:1053: file_successes += 0 if summary['returncode'] else 1
lib/python/gunittest/reporters.py:1066: status=returncode_to_html_text(summary['returncode']),
lib/python/gunittest/multirunner.py:61: if p.returncode != 0:
lib/python/gunittest/multirunner.py:100: returncode = p.wait()
lib/python/gunittest/multirunner.py:108: returncode = p.wait()
lib/python/gunittest/multirunner.py:109: if returncode != 0:
lib/python/temporal/temporal_vector_algebra.py:607: returncode = TemporalAlgebraParser.overlay_map_extent(self, mapA, mapB,
lib/python/temporal/temporal_vector_algebra.py:610: if not copy and returncode == 1:
lib/python/temporal/temporal_vector_algebra.py:617: return(returncode)
lib/python/temporal/temporal_vector_algebra.py:632: returncode = 0
lib/python/temporal/temporal_vector_algebra.py:663: returncode = 1
lib/python/temporal/temporal_vector_algebra.py:670: if cmd.popen.returncode != 0:
lib/python/temporal/temporal_vector_algebra.py:680: returncode = 1
lib/python/temporal/temporal_vector_algebra.py:682: if returncode == 0:
lib/python/temporal/temporal_vector_algebra.py:828: returncode = self.overlay_map_extent(map_new, map_j, opname, \
lib/python/temporal/temporal_vector_algebra.py:831: if returncode == 0:
lib/python/temporal/temporal_vector_algebra.py:866: if returncode == 0:
lib/python/temporal/temporal_vector_algebra.py:869: if returncode == 1:
lib/python/temporal/temporal_algebra.py:746: returncode = 1
lib/python/temporal/temporal_algebra.py:762: returncode = 0
lib/python/temporal/temporal_algebra.py:768: returncode = 0
lib/python/temporal/temporal_algebra.py:774: returncode = 0
lib/python/temporal/temporal_algebra.py:782: returncode = 0
lib/python/temporal/temporal_algebra.py:788: returncode = 0
lib/python/temporal/temporal_algebra.py:794: returncode = 0
lib/python/temporal/temporal_algebra.py:796: return(returncode)
lib/python/temporal/temporal_raster_base_algebra.py:277: returncode = 0
lib/python/temporal/temporal_raster_base_algebra.py:393: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:396: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:426: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:534: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:537: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:568: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:666: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:668: print(returncode)
lib/python/temporal/temporal_raster_base_algebra.py:670: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:705: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:708: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:746: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:749: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:780: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:783: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:958: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:961: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:987: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:1048: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:1051: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:1077: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:1147: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:1150: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:1178: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:1256: returncode = self.overlay_map_extent(map_new, map_then, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:1260: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:1267: returncode = self.overlay_map_extent(map_new, map_else, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:1271: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:1312: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:1425: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:1428: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:1470: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:1544: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:1547: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:1589: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:1678: returncode = self.overlay_map_extent(map_new, map_then, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:1682: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:1689: returncode = self.overlay_map_extent(map_new, map_else, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:1693: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:1734: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:1820: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:1823: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:1849: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:1887: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:1890: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:1916: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:1967: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:1970: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:1996: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:2133: returncode = self.overlay_map_extent(map_new, map_then, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:2137: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:2144: returncode = self.overlay_map_extent(map_new, map_else, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:2148: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:2189: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:2294: returncode = self.overlay_map_extent(map_new, map_then, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:2298: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:2305: returncode = self.overlay_map_extent(map_new, map_else, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:2309: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:2350: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:2485: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:2488: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:2530: if returncode == 1:
lib/python/temporal/temporal_raster_base_algebra.py:2613: returncode = self.overlay_map_extent(map_new, map_j, 'and', \
lib/python/temporal/temporal_raster_base_algebra.py:2616: if returncode == 0:
lib/python/temporal/temporal_raster_base_algebra.py:2658: if returncode == 1:
lib/python/script/task.py:487: if p.returncode != 0:
lib/python/script/core.py:648: sys.exit(p.returncode)
lib/python/script/core.py:1328: if ps.returncode != 0 and error:
lib/python/exceptions/__init__.py:60: :param rc: process returncode
lib/python/exceptions/__init__.py:63: def __init__(self, module, code, returncode, errors=None):
lib/python/exceptions/__init__.py:64: super(CalledModuleError, self).__init__(returncode, module)
lib/python/exceptions/__init__.py:66: msg += _("\nProcess ended with non-zero return code %s") % returncode
lib/init/grass.py:547: if p.returncode == 0:
scripts/r.in.wms/wms_base.py:390: if ps.returncode != 0:
scripts/r.reclass.area/r.reclass.area.py:153: if p2.returncode != 0:
scripts/db.droptable/db.droptable.py:94: if p.returncode != 0:
scripts/v.report/v.report.py:98: if p.returncode != 0:
scripts/v.out.gps/v.out.gps.py:215: if p1.returncode != 0 or p2.returncode != 0:
scripts/v.out.gps/v.out.gps.py:232: if p3.returncode != 0:
scripts/m.proj/m.proj.py:280: if p.returncode != 0:
scripts/v.in.gps/v.in.gps.py:215:# if p1.returncode != 0 or p2.returncode != 0:
scripts/v.in.gps/v.in.gps.py:232:# if p3.returncode != 0:
gui/wxpython/vnet/vnet_core.py:136: def RunAnDone(self, cmd, returncode, results):
gui/wxpython/vnet/vnet_core.py:285: def _createTtbDone(self, cmd, returncode):
gui/wxpython/vnet/vnet_core.py:287: if returncode != 0:
gui/wxpython/vnet/vnet_core.py:305: self.ttbCreated.emit(returncode = returncode)
gui/wxpython/vnet/vnet_core.py:463: def _vnetPathRunTurnsAnDone(self, cmd, returncode):
gui/wxpython/vnet/vnet_core.py:466: self._vnetPathRunAnDone(cmd, returncode)
gui/wxpython/vnet/vnet_core.py:468: def _vnetPathRunAnDone(self, cmd, returncode):
gui/wxpython/vnet/vnet_core.py:472: self._onDone(cmd, returncode)
gui/wxpython/vnet/vnet_core.py:474: def _onDone(self, cmd, returncode):
gui/wxpython/vnet/vnet_core.py:480: self.onAnDone(cmd, returncode, output)
gui/wxpython/vnet/vnet_core.py:588: def _runTurnsAnDone(self, cmd, returncode):
gui/wxpython/vnet/vnet_core.py:592: self._onDone(cmd, returncode)
gui/wxpython/vnet/vnet_core.py:694: def _runAnDone(self, cmd, returncode):
gui/wxpython/vnet/vnet_core.py:703: self._onDone(cmd, returncode)
gui/wxpython/iscatt/iscatt_core.py:139: returncode, scatts = ComputeScatts(self.an_data.GetRegion(),
gui/wxpython/iscatt/iscatt_core.py:147: if returncode < 0:
gui/wxpython/gui_core/dialogs.py:1149: def ShowResult(self, group, returnCode, create):
gui/wxpython/gui_core/dialogs.py:1152: if returnCode is None:
gui/wxpython/gui_core/dialogs.py:1154: elif returnCode == 0:
gui/wxpython/gui_core/dialogs.py:1218: self.ShowResult(group = group, returnCode = ret, create = False)
gui/wxpython/gui_core/dialogs.py:1222: self.ShowResult(group = group, returnCode = ret, create = True)
gui/wxpython/gui_core/dialogs.py:1764: def AddLayers(self, returncode, cmd = None):
gui/wxpython/gui_core/dialogs.py:1766: if not self.add.IsChecked() or returncode != 0:
gui/wxpython/gui_core/dialogs.py:1810: def OnCmdDone(self, cmd, returncode):
gui/wxpython/gui_core/dialogs.py:1930: def OnCmdDone(self, cmd, returncode):
gui/wxpython/gui_core/dialogs.py:1935: self.AddLayers(cmd, returncode)
gui/wxpython/gui_core/dialogs.py:1940: if returncode == 0 and self.closeOnFinish.IsChecked():
gui/wxpython/gui_core/dialogs.py:2100: def OnCmdDone(self, cmd, returncode):
gui/wxpython/gui_core/dialogs.py:2105: self.AddLayers(cmd, returncode)
gui/wxpython/gui_core/forms.py:619: def OnDone(self, cmd, returncode):
gui/wxpython/gui_core/forms.py:623: :param returncode: command's return code (0 for success)
gui/wxpython/gui_core/forms.py:642: (returncode == 0):
gui/wxpython/gcp/manager.py:680: if p.returncode == 0:
gui/wxpython/gcp/manager.py:681: print 'returncode = ', str(p.returncode)
gui/wxpython/gcp/manager.py:1480: returncode = kargs['returncode']
gui/wxpython/gcp/manager.py:1482: if returncode == 0:
gui/wxpython/web_services/widgets.py:475: if event.returncode != 0:
gui/wxpython/gis_set.py:523: returncode, error = RunCommand('v.in.ogr', dsn = filePath, output = mapName,
gui/wxpython/gis_set.py:526: returncode, error = RunCommand('r.in.gdal', input = filePath, output = mapName,
gui/wxpython/gis_set.py:530: if returncode != 0:
gui/wxpython/modules/vclean.py:443: def OnDone(self, cmd, returncode):
gui/wxpython/modules/extensions.py:234: def OnDone(self, cmd, returncode):
gui/wxpython/modules/extensions.py:235: if returncode == 0:
gui/wxpython/modules/mcalc_builder.py:596: def OnDone(self, cmd, returncode):
gui/wxpython/modules/mcalc_builder.py:601: if returncode != 0:
gui/wxpython/lmgr/frame.py:497: def OnDone(self, cmd, returncode):
gui/wxpython/gmodeler/frame.py:506: def OnDone(self, cmd, returncode):
gui/wxpython/gmodeler/frame.py:1731: def OnDone(self, cmd, returncode):
gui/wxpython/psmap/frame.py:294: if event.returncode != 0:
gui/wxpython/psmap/frame.py:296: message = _("Ps.map exited with return code %s") % event.returncode)
gui/wxpython/animation/provider.py:273: returncode, stdout, messages = read2_command(cmdTuple[0], **cmdTuple[1])
gui/wxpython/animation/provider.py:275: if returncode == 0:
gui/wxpython/animation/provider.py:494: returncode, stdout, messages = read2_command(cmdTuple[0], **cmdTuple[1])
gui/wxpython/animation/provider.py:495: if returncode != 0:
gui/wxpython/animation/provider.py:533: returncode, stdout, messages = read2_command(cmdTuple[0], **cmdTuple[1])
gui/wxpython/animation/provider.py:534: if returncode != 0:
gui/wxpython/animation/provider.py:571: returncode, stdout, messages = read2_command('g.pnmcomp',
gui/wxpython/animation/provider.py:581: if returncode != 0:
gui/wxpython/animation/provider.py:740: return ps.returncode, stdout, stderr
gui/wxpython/animation/nviztask.py:301: returncode, message = RunCommand(getErrorMsg=True, prog=cmd[0], **cmd[1])
gui/wxpython/animation/nviztask.py:302: print returncode, message
gui/wxpython/core/gcmd.py:348: if cmd.returncode == None:
gui/wxpython/core/gcmd.py:350: elif cmd.returncode == 0:
gui/wxpython/core/gcmd.py:353: print 'FAILURE (%d)' % cmd.returncode
gui/wxpython/core/gcmd.py:399: self.returncode = self.cmdThread.module.returncode
gui/wxpython/core/gcmd.py:401: self.returncode = 1
gui/wxpython/core/gcmd.py:404: self.returncode = None
gui/wxpython/core/gcmd.py:406: if self.returncode is not None:
gui/wxpython/core/gcmd.py:407: Debug.msg (3, "Command(): cmd='%s', wait=%s, returncode=%d, alive=%s" % \
gui/wxpython/core/gcmd.py:408: (' '.join(cmd), wait, self.returncode, self.cmdThread.isAlive()))
gui/wxpython/core/gcmd.py:409: if rerr is not None and self.returncode != 0:
gui/wxpython/core/gcmd.py:426: Debug.msg (3, "Command(): cmd='%s', wait=%s, returncode=?, alive=%s" % \
gui/wxpython/core/gcmd.py:669: :return: returncode (read == False and getErrorMsg == False)
gui/wxpython/core/gcmd.py:670: :return: returncode, messages (read == False and getErrorMsg == True)
gui/wxpython/core/gcmd.py:672: :return: returncode, stdout, messages (read == True and getErrorMsg == True)
gui/wxpython/core/gcmd.py:708: ret = ps.returncode
gui/wxpython/core/gconsole.py:158: returncode = self.requestCmd.module.returncode
gui/wxpython/core/gconsole.py:160: returncode = 0 # being optimistic
gui/wxpython/core/gconsole.py:200: returncode=returncode,
gui/wxpython/core/gconsole.py:631: event.onDone(cmd=event.cmd, returncode=event.returncode)
gui/wxpython/core/gconsole.py:640: if event.returncode != 0 or event.aborted:
temporal/t.rast.accumulate/t.rast.accumulate.py:441: if accmod.popen.returncode != 0:
And in grass-addons:
grep --color=auto --exclude-dir={.svn,.git,.OBJ,locale} -IrnE "returncode" ../grass_addons
../grass_addons/grass6/raster/r.in.wms2/wms_base.py:350: if ps.returncode != 0:
../grass_addons/grass7/vector/v.lfp/v.lfp.py:54: if p.returncode != 0 or res == "":
../grass_addons/grass7/vector/v.lfp/v.lfp.py:80: if p.returncode != 0 or lines == "":
../grass_addons/grass7/vector/v.lfp/v.lfp.py:101: if p.returncode != 0:
../grass_addons/grass7/vector/v.lfp/v.lfp.py:126: if p.returncode != 0 or mincat == "" or maxcat == "":
../grass_addons/grass7/vector/v.lfp/v.lfp.py:148: if p.returncode != 0 or startx == "" or starty == "":
../grass_addons/grass7/vector/v.in.wfs2/wfs_base.py:226: if ps.returncode != 0:
../grass_addons/grass7/general/g.proj.all/g.proj.all.py:120: returncode = gcore.run_command('r.proj', quiet=True, **parameters)
../grass_addons/grass7/general/g.proj.all/g.proj.all.py:121: if returncode != 0:
../grass_addons/grass7/general/g.proj.all/g.proj.all.py:153: returncode = gcore.run_command('v.proj', quiet=True, **parameters)
../grass_addons/grass7/general/g.proj.all/g.proj.all.py:154: if returncode != 0:
../grass_addons/grass7/raster/r.lfp/r.lfp.py:52: if p.returncode != 0:
../grass_addons/grass7/raster/r.lfp/r.lfp.py:84: if p.returncode != 0 or max == "":
So are 255 lines that need to be checked and adapted. 46 lines in gunittest, 85 in the temporal framework and 80 in wxpython. To me it looks feasible, especially if we split the load on 4/5 people. What do you think?
follow-up: 23 comment:22 by , 10 years ago
Replying to zarch:
So are 255 lines that need to be checked and adapted. 46 lines in gunittest, 85 in the temporal framework and 80 in wxpython.
I did different search in comment:5 with different results. The problem is that both the ones which are using return code and which are not must be checked.
To me it looks feasible, especially if we split the load on 4/5 people. What do you think?
Split the load is currently the only option but we need almost everybody who is currently active to participate.
It is quite important step and it will reveal a lot of (already present) errors which will create a lot of negative feedback, so that's why is should be done well.
comment:23 by , 10 years ago
Replying to wenzeslaus:
Replying to zarch:
So are 255 lines that need to be checked and adapted. 46 lines in gunittest, 85 in the temporal framework and 80 in wxpython.
I did different search in comment:5 with different results. The problem is that both the ones which are using return code and which are not must be checked.
I don't agree on this point. :-) So I try to explain better my point.
All the code that don't check the returncode is a bug, or at least is based on a buggy assumption that everything is going fine. If the assumption for whatever reason is not honored we will have unpredictable results.
So just continue to use this buggy assumption and focus only where the return code is handled, that will change from:
returncode = dosomething(*a, **kw)
if returncode:
# do something else
in
try:
dosomething(*a, **kw)
except CalledModuleError:
# do something else
Then when our assumption will be not respect we will have a nice Exception that tell us exactly what is failed and where, and then we know that we should add a try/except check on this part of the code.
In this way we can start modifying only a sub-set, that should not take too much time, and the code it will not more buggy than how it is now. After that we can release, and then star improving all the remaining parts of the code.
follow-up: 25 comment:24 by , 10 years ago
Replying to wenzeslaus:
This is a serious change which will require a lot of testing in both modules (core and addons) and GUI. What are the opinions on doing something like that?
The existing behaviour is wrong and should be fixed before 7.0 is released.
The alternative is to go for 7.x and probably even further (8.x) with the current unsafe but broadly used API
The Python scripting API is new in 7.0, which hasn't been released yet. The fundamental feature of 7.0 was that we get to break compatibility for the sake of correctness, so it makes no sense break correctness for the sake of compatibility with something which hasn't even been released.
comment:25 by , 10 years ago
Replying to glynn:
Replying to wenzeslaus:
This is a serious change which will require a lot of testing in both modules (core and addons) and GUI. What are the opinions on doing something like that?
The existing behaviour is wrong and should be fixed before 7.0 is released.
The alternative is to go for 7.x and probably even further (8.x) with the current unsafe but broadly used API
The Python scripting API is new in 7.0, which hasn't been released yet. The fundamental feature of 7.0 was that we get to break compatibility for the sake of correctness, so it makes no sense break correctness for the sake of compatibility with something which hasn't even been released.
I was working on #2326 a little bit last week but I'm far from finishing. I have a first implementation of raise in run/write/read command functions and some changes make GUI start again. Nothing done on the rest of the code. Patch run_with_raise_core.diff attached.
by , 10 years ago
| Attachment: | run_with_raise_core.diff added |
|---|
Changes in grass.script.core to make run/write/read raise exception and changes in other functions to keep GUI starting
comment:26 by , 10 years ago
Done for trunk in r62566. Please, test functionality you are interested in, or better, write test for it, so you don't have to test it next time.
Patch for addons is prepared.
follow-up: 28 comment:27 by , 10 years ago
The vector network toolbox seems to lack some related update (trunk from 4th of Nov 2014). We tried to run v.net.alloc therein on roads_major:
Traceback (most recent call last): File "/home/matteo/software/grass_trunk/dist.x86_64 -unknown-linux-gnu/gui/wxpython/lmgr/frame.py", line 788, in OnVNet self.GetMapDisplay().OnVNet(event) File "/home/matteo/software/grass_trunk/dist.x86_64 -unknown-linux-gnu/gui/wxpython/mapdisp/frame.py", line 1420, in OnVNet self.dialogs['vnet'] = VNETDialog(parent=self, giface=self._giface) File "/home/matteo/software/grass_trunk/dist.x86_64 -unknown-linux-gnu/gui/wxpython/vnet/dialogs.py", line 134, in __init__ self._createInputDbMgrPage() File "/home/matteo/software/grass_trunk/dist.x86_64 -unknown-linux-gnu/gui/wxpython/vnet/dialogs.py", line 448, in _createInputDbMgrPage self.inpDbMgrData['dbMgr'] = DbMgrBase() File "/home/matteo/software/grass_trunk/dist.x86_64 -unknown-linux-gnu/gui/wxpython/dbmgr/base.py", line 669, in __init__ self.dbMgrData['mapDBInfo'] = VectorDBInfo(self.dbMgrData['vectName']) File "/home/matteo/software/grass_trunk/dist.x86_64 -unknown-linux-gnu/gui/wxpython/dbmgr/vinfo.py", line 74, in __init__ VectorDBInfoBase.__init__(self, map) File "/home/matteo/software/grass_trunk/dist.x86_64 -unknown-linux-gnu/gui/wxpython/gui_core/gselect.py", line 723, in __init__ if not self._CheckDBConnection(): # -> self.layers File "/home/matteo/software/grass_trunk/dist.x86_64 -unknown-linux-gnu/gui/wxpython/gui_core/gselect.py", line 731, in _CheckDBConnection self.layers = grass.vector_db(map = self.map, stderr = nuldev) File "/home/matteo/software/grass_trunk/dist.x86_64 -unknown-linux-gnu/etc/python/grass/script/vector.py", line 42, in vector_db **args) File "/home/matteo/software/grass_trunk/dist.x86_64 -unknown-linux-gnu/etc/python/grass/script/core.py", line 420, in read_command returncode=returncode) grass.exceptions . CalledModuleError : Module run None v.db.connect --q -g stderr=<open file '/dev/null', mode 'w+' at 0x7ffac45d8f60> sep=; ended with error Process ended with non-zero return code 1. See errors in the (error) output.
follow-up: 29 comment:28 by , 10 years ago
Replying to neteler:
The vector network toolbox seems to lack some related update (trunk from 4th of Nov 2014). We tried to run v.net.alloc therein on roads_major:
Actually this means that the new API works. This API exposes previously hidden problems and potential bugs. In this particular case, I fixed it in r62614, it's a quick fix, but no time to refactor the messy code there.
follow-up: 30 comment:29 by , 10 years ago
Replying to annakrat:
Replying to neteler:
The vector network toolbox seems to lack some related update (trunk from 4th of Nov 2014). We tried to run v.net.alloc therein on roads_major:
Actually this means that the new API works. This API exposes previously hidden problems and potential bugs. In this particular case, I fixed it in r62614, it's a quick fix, but no time to refactor the messy code there.
Thus, report encountered problems as separate bugs, please.
comment:30 by , 10 years ago
follow-up: 32 comment:31 by , 10 years ago
The new implementation still shows an issue:
GRASS 7.1.svn (nc_spm_08_grass7):~ > g.extension r.basin
GRASS 7.1.svn (nc_spm_08_grass7):~ > r.basin map=elevation prefix=my_basin coord=637500.0,221750.0 dir=/tmp/bla threshold=1000
WARNING: 'r.hypso' required. Please install 'r.hypso' first using
'g.extension r.hypso'
WARNING: 'r.stream.basins' required. Please install 'r.stream.basins' first
using 'g.extension r.stream.basins'
WARNING: 'r.stream.distance' required. Please install 'r.stream.distance'
first using 'g.extension r.stream.distance'
WARNING: 'r.stream.order' required. Please install 'r.stream.order' first
using 'g.extension r.stream.order'
WARNING: 'r.stream.snap' required. Please install 'r.stream.snap' first
using 'g.extension r.stream.snap'
WARNING: 'r.stream.stats' required. Please install 'r.stream.stats' first
using 'g.extension r.stream.stats'
WARNING: 'r.width.funct' required. Please install 'r.width.funct' first
using 'g.extension r.width.funct'
ERROR: An ERROR occurred running r.basin
Traceback (most recent call last):
File "/home/neteler/.grass7/addons/scripts/r.basin", line 836, in <module>
sys.exit(main())
File "/home/neteler/.grass7/addons/scripts/r.basin", line 94, in main
check_progs()
File "/home/neteler/.grass7/addons/scripts/r.basin", line 90, in check_progs
grass.error(_("An ERROR occurred running r.basin"))
File "/home/neteler/software/grass71/dist.x86_64-unknown-linux-gnu/etc/python/grass/script/core.py", line 579, in error
message(msg, flag='e')
File "/home/neteler/software/grass71/dist.x86_64-unknown-linux-gnu/etc/python/grass/script/core.py", line 517, in message
run_command("g.message", flags=flag, message=msg)
File "/home/neteler/software/grass71/dist.x86_64-unknown-linux-gnu/etc/python/grass/script/core.py", line 361, in run_command
returncode=returncode)
grass.exceptions.CalledModuleError: Module run None g.message -e message=An ERROR occurred running r.basin ended with error
Process ended with non-zero return code 1. See errors in the (error) output.
comment:32 by , 10 years ago
follow-up: 34 comment:33 by , 10 years ago
While no crash occurs any more with r62705, it doesn't stop any longer on error:
GRASS 7.1.svn (nc_spm_08_grass7):~ > r.basin map=elevation prefix=my_basin coord=637500.0,221750.0 dir=/tmp/bla threshold=1000 --o
WARNING: 'r.hypso' required. Please install 'r.hypso' first using
'g.extension r.hypso'
WARNING: 'r.stream.basins' required. Please install 'r.stream.basins' first
using 'g.extension r.stream.basins'
WARNING: 'r.stream.distance' required. Please install 'r.stream.distance'
first using 'g.extension r.stream.distance'
WARNING: 'r.stream.order' required. Please install 'r.stream.order' first
using 'g.extension r.stream.order'
WARNING: 'r.stream.snap' required. Please install 'r.stream.snap' first
using 'g.extension r.stream.snap'
WARNING: 'r.stream.stats' required. Please install 'r.stream.stats' first
using 'g.extension r.stream.stats'
WARNING: 'r.width.funct' required. Please install 'r.width.funct' first
using 'g.extension r.width.funct'
ERROR: An ERROR occurred running r.basin
SECTION 1 beginning: Initiating Variables. 4 sections total.
SECTION 1a: Mark masked and NULL cells
...
follow-ups: 35 39 comment:34 by , 10 years ago
Replying to neteler:
While no crash occurs any more with r62705, it doesn't stop any longer on error:
GRASS 7.1.svn (nc_spm_08_grass7):~ > r.basin map=elevation prefix=my_basin coord=637500.0,221750.0 dir=/tmp/bla threshold=1000 --o
...
WARNING: 'r.width.funct' required. Please install 'r.width.funct' first
using 'g.extension r.width.funct'
ERROR: An ERROR occurred running r.basin
SECTION 1 beginning: Initiating Variables. 4 sections total.
> ...
r.basin is using function error() which is something different than function fatal(). error() just prints the message and continues, fatal() actually exits and raises. So, the behavior is correct according to implementation and is the same before r62566 and after r62705.
- source:grass-addons/grass7/raster/r.basin/r.basin.py?rev=62705#L80
- source:grass/trunk/lib/python/script/core.py?rev=62705#L569
r62705 is doing two things. It adds a possibility to select method of error handling and unifies the implementation (see previous comments for discussion). Additionally it solves the issue with g.message which returns non-zero return code when an error is printed:
GRASS > g.message "Hello!" Hello! GRASS > echo $? 0 GRASS > g.message -w "Hello!" WARNING: Hello! GRASS > echo $? 0 GRASS > g.message -e "Hello!" ERROR: Hello! GRASS > echo $? 1
I suppose that g.message behavior is advantageous for terminations of shell scripts using set -e. This makes g.message special comparing to other GRASS modules (alongside with g.findfile). You may find something like this in one of the earlier patches in this ticket, I forgot about it this time.
Note also that the changes in error handling were applied to trunk only. The changes for addons are basically done but not committed because addons are supposed to be compatible with release branch. Also the changes will be done only for modules which are doing the correct error handling. The code ignoring errors will not be changed. Introduction of raise will show these ignored errors and will prevent further execution of the module/script since the the error occurred.
follow-up: 36 comment:35 by , 10 years ago
Replying to wenzeslaus:
r.basinis using functionerror()which is something different than functionfatal().error()just prints the message and continues,fatal()actually exits and raises. So, the behavior is correct according to implementation and is the same before r62566 and after r62705.
Ah, I overlooked this difference. r.basin fixed in r62708, now it exits as expected in case a dependency wasn't installed yet.
follow-up: 37 comment:36 by , 10 years ago
Replying to neteler:
Replying to wenzeslaus:
r.basinis using functionerror()which is something different than functionfatal().error()just prints the message and continues,fatal()actually exits and raises. So, the behavior is correct according to implementation and is the same before r62566 and after r62705.Ah, I overlooked this difference. r.basin fixed in r62708, now it exits as expected in case a dependency wasn't installed yet.
Yes, it might be confusing. C API has G_fatal_error() and Python API has error() and fatal(). I've tried to improve the documentation in trunk in r62709.
follow-up: 38 comment:37 by , 10 years ago
Replying to wenzeslaus:
Yes, it might be confusing. C API has
G_fatal_error()and Python API haserror()andfatal(). I've tried to improve the documentation in trunk in r62709.
THanks but why do we need error() at all? If a function does not end the execution of the program it should be called warning... (which we have as well).
comment:38 by , 10 years ago
Replying to neteler:
Replying to wenzeslaus:
Yes, it might be confusing. C API has
G_fatal_error()and Python API haserror()andfatal(). I've tried to improve the documentation in trunk in r62709.THanks but why do we need
error()at all? If a function does not end the execution of the program it should be called warning... (which we have as well).
Sometimes you have an error that is not fatal enough to end the program, but allows to continue processing. Or you want to print an error in "fatal error" style, but let a different function handle the exit strategy.
comment:39 by , 10 years ago
Replying to wenzeslaus:
I suppose that
g.messagebehavior is advantageous for terminations of shell scripts usingset -e. This makesg.messagespecial comparing to other GRASS modules
Not really ...
Like (almost?) any other GRASS module, g.message returns a non-zero exit code if a fatal error was generated, and a zero exit code otherwise.
g.message calls a different function (G_verbose_message(), G_warning() etc) depending upon the flag which was used (defaulting to G_message() if no flag was used). If the -e flag is used, it calls G_fatal_error(), hence the non-zero exit code.
comment:40 by , 10 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
The change was done, improved, backported and addons were fixed.
- commits: r62566, r62705
- backports: r62786, r62787, r62791
- addons: r62790, r62687
- related: r62709 (r62788), r62691 (r62789, r62614, #2474)
Closing the ticket.
The enhancement to this ticket is better storing of information and constructing message in CalledModuleError.
As mentioned earlier, if you hit an issue when something will end with CalledModuleError, please open a new ticket.
Replying to wenzeslaus:
Personally, I'd suggest just modifying the existing functions to raise exceptions if the command fails.
Scripts which want to do their own error handling can just use start_command() directly; the higher-level functions are only trivial wrappers around this.