grass.script: Do not print stderr when not captured (#2246)

Even when the global _capture_stderr is set, the functions allow users to not capture stderr (e.g., by using devnull). In these cases, sys.stderr.write fails because stderr is None. An example is grass.script.db.db_connection where db.connect regularly fails during grass.script.setup.finish when there is no database connection. With this fix, the function no longer fails and correctly handles the error state with except CalledModuleError.

The issue with the original code is visible in the code itself: _capture_stderr is used in a condition to set stderr=PIPE, but that's conditional on values in kwargs. However, this second condition is not considered when output stderr value is used later on.

The test function named test_init_finish_global_functions_capture_strerr fails with the previous version of the code.

python/grass/script/core.py

@@ -534,7 +534,7 @@ def run_command(*args, **kwargs):
             stdout = _make_unicode(stdout, encoding)
             stderr = _make_unicode(stderr, encoding)
         returncode = ps.poll()
-        if returncode:
+        if returncode and stderr:
             sys.stderr.write(stderr)
     else:
         returncode = ps.wait()
@@ -601,7 +601,9 @@ def read_command(*args, **kwargs):
         stdout = _make_unicode(stdout, encoding)
         stderr = _make_unicode(stderr, encoding)
     returncode = process.poll()
-    if _capture_stderr and returncode:
+    if returncode and _capture_stderr and stderr:
+        # Print only when we are capturing it and there was some output.
+        # (User can request ignoring the subprocess stderr and then we get only None.)
         sys.stderr.write(stderr)
     return handle_errors(returncode, stdout, args, kwargs)
 
@@ -689,7 +691,7 @@ def write_command(*args, **kwargs):
         unused = _make_unicode(unused, encoding)
         stderr = _make_unicode(stderr, encoding)
     returncode = process.poll()
-    if _capture_stderr and returncode:
+    if returncode and _capture_stderr and stderr:
         sys.stderr.write(stderr)
     return handle_errors(returncode, None, args, kwargs)
 
@@ -854,8 +856,8 @@ def set_capture_stderr(capture=True):
 
     .. note::
 
-        This is advantages for interactive shells such as the one in GUI
-        and interactive notebooks such as Jupyer Notebook.
+        This is advantageous for interactive shells such as the one in GUI
+        and interactive notebooks such as Jupyter Notebook.
 
     The capturing can be applied only in certain cases, for example
     in case of run_command() it is applied because run_command() nor

python/grass/script/tests/grass_script_setup_test.py

@@ -1,12 +1,82 @@
 """Test functions in grass.script.setup"""
 
+import multiprocessing
+import os
+
+import pytest
+
 import grass.script as gs
 import grass.script.setup as grass_setup
 
 
+# All init tests change the global environment, but when it really matters,
+# we use a separate process.
+# Ideally, the functions would support env parameter and the test
+# would mostly use that.
+def run_in_subprocess(function):
+    """Run function in a separate process
+
+    The function must take a Queue and put its result there.
+    The result is then returned from this function.
+    """
+    queue = multiprocessing.Queue()
+    process = multiprocessing.Process(target=function, args=(queue,))
+    process.start()
+    result = queue.get()
+    process.join()
+    return result
+
+
 def test_init_as_context_manager(tmp_path):
     """Check that init function return value works as a context manager"""
     location = "test"
     gs.core._create_location_xy(tmp_path, location)  # pylint: disable=protected-access
     with grass_setup.init(tmp_path / location):
         gs.run_command("g.region", flags="p")
+        session_file = os.environ["GISRC"]
+    assert not os.path.exists(session_file)
+
+
+def test_init_session_finish(tmp_path):
+    """Check that init works with finish on the returned session object"""
+    location = "test"
+    gs.core._create_location_xy(tmp_path, location)  # pylint: disable=protected-access
+    session = grass_setup.init(tmp_path / location)
+    gs.run_command("g.region", flags="p")
+    session_file = os.environ["GISRC"]
+    session.finish()
+    with pytest.raises(ValueError):
+        session.finish()
+    assert not session.active
+    assert not os.path.exists(session_file)
+
+
+def test_init_finish_global_functions(tmp_path):
+    """Check that init and finish global functions work"""
+    location = "test"
+    gs.core._create_location_xy(tmp_path, location)  # pylint: disable=protected-access
+    grass_setup.init(tmp_path / location)
+    gs.run_command("g.region", flags="p")
+    session_file = os.environ["GISRC"]
+    grass_setup.finish()
+
+    assert not os.path.exists(session_file)
+
+
+def test_init_finish_global_functions_capture_strerr(tmp_path):
+    """Check that init and finish global functions work"""
+
+    def init_finish(queue):
+        gs.set_capture_stderr(True)
+        location = "test"
+        gs.core._create_location_xy(  # pylint: disable=protected-access
+            tmp_path, location
+        )
+        grass_setup.init(tmp_path / location)
+        gs.run_command("g.region", flags="p")
+        queue.put(os.environ["GISRC"])
+        grass_setup.finish()
+
+    session_file = run_in_subprocess(init_finish)
+    assert session_file, "Expected file name from the subprocess"
+    assert not os.path.exists(session_file), "Session file not deleted"