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
