pitrou commented on code in PR #46755: URL: https://github.com/apache/arrow/pull/46755#discussion_r2137128020
########## python/pyarrow/lib.pyx: ########## @@ -96,6 +96,15 @@ def is_threading_enabled() -> bool: return libarrow_python.IsThreadingEnabled() +def get_pybuild_type() -> str: + """ + Returns the PyArrow build type (debug, minsizerel, release, + relwithdebinfo). The default build type is release, regardless of if C++ + was built in debug mode. Review Comment: Can we keep using the summary line + longer description convention? ########## python/pyarrow/lib.pyx: ########## @@ -96,6 +96,15 @@ def is_threading_enabled() -> bool: return libarrow_python.IsThreadingEnabled() +def get_pybuild_type() -> str: Review 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? ########## python/pyarrow/src/arrow/python/helpers.cc: ########## @@ -499,6 +500,14 @@ bool IsThreadingEnabled() { #endif } +namespace { + const std::string kPyBuildType = PYARROW_CYTHON_BUILD_TYPE; +} + +std::string GetPyBuildType() { + return kPyBuildType; +} Review Comment: The separate constant seems a bit pointless to me? This will return a copy anyway. ```suggestion std::string GetPyBuildType() { return PYARROW_CYTHON_BUILD_TYPE; } ``` ########## python/pyarrow/tests/test_gdb.py: ########## @@ -25,6 +25,7 @@ import pytest import pyarrow as pa +from pyarrow.lib import get_pybuild_type Review 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) ########## python/pyarrow/tests/test_misc.py: ########## @@ -23,6 +23,7 @@ import pyarrow as pa from pyarrow.lib import ArrowInvalid +from pyarrow.lib import get_pybuild_type Review Comment: Same here. ########## python/pyarrow/src/arrow/python/pybuild_type.h.cmake: ########## @@ -0,0 +1,18 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#define PYARROW_CYTHON_BUILD_TYPE "@LOWERCASE_PYBUILD_TYPE@" Review Comment: I would simply call it `PYARROW_BUILD_TYPE`, because this is not related to Cython. ########## python/CMakeLists.txt: ########## @@ -350,6 +350,11 @@ endif() # PyArrow C++ set(PYARROW_CPP_ROOT_DIR pyarrow/src) set(PYARROW_CPP_SOURCE_DIR ${PYARROW_CPP_ROOT_DIR}/arrow/python) + +# 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) Review Comment: Why not make this something like `config.h.cmake`? We may want to add other constants there in the future. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org