GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug #46755
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
dinse
changed the title
|
|
| Returns the PyArrow build type (debug, minsizerel, release, | ||
| relwithdebinfo). The default build type is release, regardless of if C++ | ||
| was built in debug mode. |
There was a problem hiding this comment.
Can we keep using the summary line + longer description convention?
| @@ -96,6 +96,15 @@ def is_threading_enabled() -> bool: | |||
| return libarrow_python.IsThreadingEnabled() | |||
|
|
|||
|
|
|||
| def get_pybuild_type() -> str: | |||
There was a problem hiding this comment.
The function name is awkward and restrictive. Perhaps we want a function def build_info() -> dict that returns various pieces of information about the build and, more importantly, that we can extend in the future?
|
|
||
| # Write out compile-time configuration constants | ||
| string(TOLOWER ${CMAKE_BUILD_TYPE} LOWERCASE_PYBUILD_TYPE) | ||
| configure_file("${PYARROW_CPP_SOURCE_DIR}/pybuild_type.h.cmake" "${PYARROW_CPP_SOURCE_DIR}/pybuild_type.h" ESCAPE_QUOTES) |
There was a problem hiding this comment.
Why not make this something like config.h.cmake? We may want to add other constants there in the future.
| namespace { | ||
| const std::string kPyBuildType = PYARROW_CYTHON_BUILD_TYPE; | ||
| } | ||
|
|
||
| std::string GetPyBuildType() { | ||
| return kPyBuildType; | ||
| } |
There was a problem hiding this comment.
The separate constant seems a bit pointless to me? This will return a copy anyway.
| namespace { | |
| const std::string kPyBuildType = PYARROW_CYTHON_BUILD_TYPE; | |
| } | |
| std::string GetPyBuildType() { | |
| return kPyBuildType; | |
| } | |
| std::string GetPyBuildType() { | |
| return PYARROW_CYTHON_BUILD_TYPE; | |
| } |
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| #define PYARROW_CYTHON_BUILD_TYPE "@LOWERCASE_PYBUILD_TYPE@" |
There was a problem hiding this comment.
I would simply call it PYARROW_BUILD_TYPE, because this is not related to Cython.
| @@ -25,6 +25,7 @@ | |||
| import pytest | |||
|
|
|||
| import pyarrow as pa | |||
| from pyarrow.lib import get_pybuild_type | |||
There was a problem hiding this comment.
You don't need to import it explicitly, just call pa.get_pybuild_type? (and make sure this is exposed in the top-level pyarrow namespace)
| @@ -23,6 +23,7 @@ | |||
|
|
|||
| import pyarrow as pa | |||
| from pyarrow.lib import ArrowInvalid | |||
| from pyarrow.lib import get_pybuild_type | |||
Rationale for this change
As mentioned in #46728, if Arrow C++ was built debug, and PyArrow wasn't, test_gdb.py runs tests that fail.
What changes are included in this PR?
The CMAKE_BUILD_TYPE environment variable is propagated from build into PyArrow, where it's checked to skip unit tests.
Are these changes tested?
Yes. I have built PyArrow in release, debug, and relwithdebinfo and observed the new behavior. Because CMakeLists.txt was changed, I built PyArrow twice via setup.py and pip install, and checked the new function.
Are there any user-facing changes?
No changes except for devs running unit tests.