From 7adfb961eccd24cdf444969498ddd7af995ea9e1 Mon Sep 17 00:00:00 2001 From: Joris Van den Bossche Date: Sun, 2 Apr 2023 11:58:51 +0200 Subject: [PATCH 1/8] add test that checks stderr --- pyogrio/tests/test_core.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pyogrio/tests/test_core.py b/pyogrio/tests/test_core.py index 8dbac663..225e2332 100644 --- a/pyogrio/tests/test_core.py +++ b/pyogrio/tests/test_core.py @@ -12,6 +12,7 @@ get_gdal_config_option, get_gdal_data_path, ) +from pyogrio.errors import DataSourceError from pyogrio._env import GDALEnv @@ -288,3 +289,14 @@ def test_reset_config_options(): set_gdal_config_options({"foo": None}) assert get_gdal_config_option("foo") is None + + +def test_error_handling(capfd): + # an operation that triggers a GDAL Failure + # -> error translated into Python exception + not printed to stderr + with pytest.raises(DataSourceError, match="No such file or directory"): + read_info("non-existent.shp") + + assert ( + "ERROR 4: non-existent.shp: No such file or directory" in capfd.readouterr().err + ) From f1822598ad07fc153d75e862d460cb5ce08abb8c Mon Sep 17 00:00:00 2001 From: Joris Van den Bossche Date: Sun, 2 Apr 2023 11:36:21 +0200 Subject: [PATCH 2/8] ENH: register custom error handler to suppress printing error messages to stderr --- pyogrio/_ogr.pxd | 5 +++++ pyogrio/_ogr.pyx | 27 +++++++++++++++++++++++++++ pyogrio/core.py | 2 ++ 3 files changed, 34 insertions(+) diff --git a/pyogrio/_ogr.pxd b/pyogrio/_ogr.pxd index ef1a5f30..aacc6070 100644 --- a/pyogrio/_ogr.pxd +++ b/pyogrio/_ogr.pxd @@ -27,6 +27,11 @@ cdef extern from "cpl_error.h": const char* CPLGetLastErrorMsg() int CPLGetLastErrorType() + ctypedef void (*CPLErrorHandler)(CPLErr, int, const char*) + void CPLDefaultErrorHandler(CPLErr, int, const char *) + void CPLPushErrorHandler(CPLErrorHandler handler) + void CPLPopErrorHandler() + cdef extern from "cpl_string.h": char** CSLAddNameValue(char **list, const char *name, const char *value) diff --git a/pyogrio/_ogr.pyx b/pyogrio/_ogr.pyx index fd938ef8..4fdb1df2 100644 --- a/pyogrio/_ogr.pyx +++ b/pyogrio/_ogr.pyx @@ -284,6 +284,33 @@ def _register_drivers(): GDALAllRegister() +cdef void error_handler(CPLErr err_class, int err_no, const char* msg) with gil: + """Send CPL debug messages and warnings to Python's logger.""" + + # Generally we want to suppress error printing to stderr (behaviour of the + # default GDAL error handler) because we already raise a Python exception + # that includes the error message + + + # If the error class is CE_Fatal, we want to have a message issued + # because the CPL support code does an abort() before any exception + # can be generated + if err_class == CPLErr.CE_Fatal: + CPLDefaultErrorHandler(err_class, err_no, msg) + + # We do not want to interfere with non-failure messages since + # they won't be translated into exceptions. + elif err_class != CPLErr.CE_Failure: + CPLDefaultErrorHandler(err_class, err_no, msg) + + else: + pass + + +def _register_error_handler(): + CPLPushErrorHandler(error_handler) + + def _get_driver_metadata_item(driver, metadata_item): """ Query driver metadata items. diff --git a/pyogrio/core.py b/pyogrio/core.py index 08ca40ec..e906821c 100644 --- a/pyogrio/core.py +++ b/pyogrio/core.py @@ -16,12 +16,14 @@ init_proj_data as _init_proj_data, remove_virtual_file, _register_drivers, + _register_error_handler, ) from pyogrio._io import ogr_list_layers, ogr_read_bounds, ogr_read_info _init_gdal_data() _init_proj_data() _register_drivers() + _register_error_handler() __gdal_version__ = get_gdal_version() __gdal_version_string__ = get_gdal_version_string() From 631f57b198a9b35312bcc5a2064f9fb47779fab3 Mon Sep 17 00:00:00 2001 From: Joris Van den Bossche Date: Sun, 2 Apr 2023 12:00:47 +0200 Subject: [PATCH 3/8] update test: now no stderr --- pyogrio/tests/test_core.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pyogrio/tests/test_core.py b/pyogrio/tests/test_core.py index 225e2332..90bbd8e8 100644 --- a/pyogrio/tests/test_core.py +++ b/pyogrio/tests/test_core.py @@ -297,6 +297,4 @@ def test_error_handling(capfd): with pytest.raises(DataSourceError, match="No such file or directory"): read_info("non-existent.shp") - assert ( - "ERROR 4: non-existent.shp: No such file or directory" in capfd.readouterr().err - ) + assert capfd.readouterr().err == "" From 8bf609a0e22cc666fc0ed5b5859588ad1e54a84d Mon Sep 17 00:00:00 2001 From: Joris Van den Bossche Date: Mon, 3 Apr 2023 09:10:20 +0200 Subject: [PATCH 4/8] clean-up handler function --- pyogrio/_ogr.pyx | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/pyogrio/_ogr.pyx b/pyogrio/_ogr.pyx index 4fdb1df2..7b1466ff 100644 --- a/pyogrio/_ogr.pyx +++ b/pyogrio/_ogr.pyx @@ -284,27 +284,28 @@ def _register_drivers(): GDALAllRegister() -cdef void error_handler(CPLErr err_class, int err_no, const char* msg) with gil: - """Send CPL debug messages and warnings to Python's logger.""" +cdef void error_handler(CPLErr err_class, int err_no, const char* msg): + """Custom CPL error handler to match the Python behaviour. - # Generally we want to suppress error printing to stderr (behaviour of the - # default GDAL error handler) because we already raise a Python exception - # that includes the error message - - - # If the error class is CE_Fatal, we want to have a message issued - # because the CPL support code does an abort() before any exception - # can be generated - if err_class == CPLErr.CE_Fatal: + Generally we want to suppress error printing to stderr (behaviour of the + default GDAL error handler) because we already raise a Python exception + that includes the error message. + """ + if err_class == CE_Fatal: + # If the error class is CE_Fatal, we want to have a message issued + # because the CPL support code does an abort() before any exception + # can be generated CPLDefaultErrorHandler(err_class, err_no, msg) + return - # We do not want to interfere with non-failure messages since - # they won't be translated into exceptions. - elif err_class != CPLErr.CE_Failure: - CPLDefaultErrorHandler(err_class, err_no, msg) + elif err_class == CE_Failure: + # For Failures, do nothing as those are explicitly catched + # with error return codes and translated into Python exceptions + return - else: - pass + # Fall back to the default handler for non-failure messages since + # they won't be translated into exceptions. + CPLDefaultErrorHandler(err_class, err_no, msg) def _register_error_handler(): From effc93b346e571a052ee6945830869f0530f6662 Mon Sep 17 00:00:00 2001 From: Joris Van den Bossche Date: Mon, 3 Apr 2023 09:14:31 +0200 Subject: [PATCH 5/8] move to _err.pyx --- pyogrio/_err.pyx | 31 ++++++++++++++++++++++++++++++- pyogrio/_ogr.pyx | 28 ---------------------------- pyogrio/core.py | 2 +- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index c1b28aa4..b88b62e0 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -3,7 +3,8 @@ from enum import IntEnum from pyogrio._ogr cimport ( CE_None, CE_Debug, CE_Warning, CE_Failure, CE_Fatal, CPLErrorReset, - CPLGetLastErrorType, CPLGetLastErrorNo, CPLGetLastErrorMsg, OGRErr) + CPLGetLastErrorType, CPLGetLastErrorNo, CPLGetLastErrorMsg, OGRErr, + CPLErr, CPLErrorHandler, CPLDefaultErrorHandler, CPLPushErrorHandler) # CPL Error types as an enum. @@ -207,3 +208,31 @@ cdef int exc_wrap_ogrerr(int err) except -1: raise CPLE_BaseError(3, err, f"OGR Error code {err}") return err + + +cdef void error_handler(CPLErr err_class, int err_no, const char* msg): + """Custom CPL error handler to match the Python behaviour. + + Generally we want to suppress error printing to stderr (behaviour of the + default GDAL error handler) because we already raise a Python exception + that includes the error message. + """ + if err_class == CE_Fatal: + # If the error class is CE_Fatal, we want to have a message issued + # because the CPL support code does an abort() before any exception + # can be generated + CPLDefaultErrorHandler(err_class, err_no, msg) + return + + elif err_class == CE_Failure: + # For Failures, do nothing as those are explicitly catched + # with error return codes and translated into Python exceptions + return + + # Fall back to the default handler for non-failure messages since + # they won't be translated into exceptions. + CPLDefaultErrorHandler(err_class, err_no, msg) + + +def _register_error_handler(): + CPLPushErrorHandler(error_handler) diff --git a/pyogrio/_ogr.pyx b/pyogrio/_ogr.pyx index 7b1466ff..fd938ef8 100644 --- a/pyogrio/_ogr.pyx +++ b/pyogrio/_ogr.pyx @@ -284,34 +284,6 @@ def _register_drivers(): GDALAllRegister() -cdef void error_handler(CPLErr err_class, int err_no, const char* msg): - """Custom CPL error handler to match the Python behaviour. - - Generally we want to suppress error printing to stderr (behaviour of the - default GDAL error handler) because we already raise a Python exception - that includes the error message. - """ - if err_class == CE_Fatal: - # If the error class is CE_Fatal, we want to have a message issued - # because the CPL support code does an abort() before any exception - # can be generated - CPLDefaultErrorHandler(err_class, err_no, msg) - return - - elif err_class == CE_Failure: - # For Failures, do nothing as those are explicitly catched - # with error return codes and translated into Python exceptions - return - - # Fall back to the default handler for non-failure messages since - # they won't be translated into exceptions. - CPLDefaultErrorHandler(err_class, err_no, msg) - - -def _register_error_handler(): - CPLPushErrorHandler(error_handler) - - def _get_driver_metadata_item(driver, metadata_item): """ Query driver metadata items. diff --git a/pyogrio/core.py b/pyogrio/core.py index e906821c..4965b501 100644 --- a/pyogrio/core.py +++ b/pyogrio/core.py @@ -16,8 +16,8 @@ init_proj_data as _init_proj_data, remove_virtual_file, _register_drivers, - _register_error_handler, ) + from pyogrio._err import _register_error_handler from pyogrio._io import ogr_list_layers, ogr_read_bounds, ogr_read_info _init_gdal_data() From c7f290e865c30b9437b00918dc6165d3a9b3e25c Mon Sep 17 00:00:00 2001 From: Joris Van den Bossche Date: Mon, 3 Apr 2023 09:33:34 +0200 Subject: [PATCH 6/8] make error_handler nogil --- pyogrio/_err.pyx | 6 +++--- pyogrio/_ogr.pxd | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index b88b62e0..64d05776 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -210,7 +210,7 @@ cdef int exc_wrap_ogrerr(int err) except -1: return err -cdef void error_handler(CPLErr err_class, int err_no, const char* msg): +cdef void error_handler(CPLErr err_class, int err_no, const char* err_msg) nogil: """Custom CPL error handler to match the Python behaviour. Generally we want to suppress error printing to stderr (behaviour of the @@ -221,7 +221,7 @@ cdef void error_handler(CPLErr err_class, int err_no, const char* msg): # If the error class is CE_Fatal, we want to have a message issued # because the CPL support code does an abort() before any exception # can be generated - CPLDefaultErrorHandler(err_class, err_no, msg) + CPLDefaultErrorHandler(err_class, err_no, err_msg) return elif err_class == CE_Failure: @@ -231,7 +231,7 @@ cdef void error_handler(CPLErr err_class, int err_no, const char* msg): # Fall back to the default handler for non-failure messages since # they won't be translated into exceptions. - CPLDefaultErrorHandler(err_class, err_no, msg) + CPLDefaultErrorHandler(err_class, err_no, err_msg) def _register_error_handler(): diff --git a/pyogrio/_ogr.pxd b/pyogrio/_ogr.pxd index aacc6070..b6202d97 100644 --- a/pyogrio/_ogr.pxd +++ b/pyogrio/_ogr.pxd @@ -14,7 +14,7 @@ cdef extern from "cpl_conv.h": void CPLSetConfigOption(const char* key, const char* value) -cdef extern from "cpl_error.h": +cdef extern from "cpl_error.h" nogil: ctypedef enum CPLErr: CE_None CE_Debug From 5f02d77badcad90724712fa0f4bdbf024fa2704d Mon Sep 17 00:00:00 2001 From: Joris Van den Bossche Date: Tue, 4 Apr 2023 08:04:15 +0200 Subject: [PATCH 7/8] Update pyogrio/_err.pyx Co-authored-by: Brendan Ward --- pyogrio/_err.pyx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 64d05776..4d80db56 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -225,7 +225,7 @@ cdef void error_handler(CPLErr err_class, int err_no, const char* err_msg) nogil return elif err_class == CE_Failure: - # For Failures, do nothing as those are explicitly catched + # For Failures, do nothing as those are explicitly caught # with error return codes and translated into Python exceptions return From b172c37786556a0d712058ab274f2ae44c3cab50 Mon Sep 17 00:00:00 2001 From: Joris Van den Bossche Date: Tue, 4 Apr 2023 08:06:27 +0200 Subject: [PATCH 8/8] add changelog note --- CHANGES.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 858c07d4..36f39888 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -12,6 +12,8 @@ specifying a mask manually for missing values in `write` (#219) - Standardized 3-dimensional geometry type labels from "2.5D " to " Z" for consistency with well-known text (WKT) formats (#234) +- Failure error messages from GDAL are no longer printed to stderr (they were + already translated into Python exceptions as well) (#236). ## 0.5.1 (2023-01-26)