rok commented on code in PR #49259: URL: https://github.com/apache/arrow/pull/49259#discussion_r2885568092
########## python/_build_backend/__init__.py: ########## @@ -0,0 +1,69 @@ +# 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. + +""" +Build backend wrapper that resolves license symlinks before delegating +to scikit-build-core. + +Arrow's LICENSE.txt and NOTICE.txt live at the repository root, one level +above python/. They are symlinked into python/ so that license-files in +pyproject.toml can reference them otherwise project metadata fails validation. +This is done before any build backend is invoked that's why symlinks are necessary. +But when building sdist tarballs symlinks are not copied and we end up with +broken LICENSE.txt and NOTICE.txt. + +This custom build backend only replace the symlinks with hardlinks +before scikit_build_core.build.build_sdist so +that sdist contains the actual file content. The symlinks are restored +afterwards so the git working tree stays clean. Review Comment: Rephrasing: ```suggestion This custom build backend replaces the symlinks with actual file copies before scikit_build_core.build.build_sdist so that the sdist contains the real file content. The symlinks are restored afterwards to keep the git working tree clean. ``` ########## python/.gitignore: ########## Review Comment: Nit - perhaps remove this `build/`? ########## python/CMakeLists.txt: ########## @@ -352,7 +352,21 @@ 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(TOUPPER ${CMAKE_BUILD_TYPE} UPPERCASE_PYBUILD_TYPE) +if(CMAKE_BUILD_TYPE) + string(TOUPPER "${CMAKE_BUILD_TYPE}" UPPERCASE_PYBUILD_TYPE) +else() + # For multi-config generators (XCode and Visual Studio), + # CMAKE_BUILD_TYPE is not set at configure time. + # scikit-build-core does the right thing with cmake.build-type and + # adds the corresponding --config but does not populate CMAKE_BUILD_TYPE + # for those. On this specific case, we set the default to "RELEASE" + # as it's the most common build type for users building from source. + # This is mainly relevant for our Windows wheels, which are built with + # Visual Studio and thus use a multi-config generator with Release. + # As a note this is only to populate config_internal.h.cmake. Review Comment: Suggestion as per codex 🤖 : ```diff diff --git i/python/CMakeLists.txt w/python/CMakeLists.txt index 31ce2f149e..7b572823e7 100644 --- i/python/CMakeLists.txt +++ w/python/CMakeLists.txt @@ -359,11 +359,9 @@ else() # CMAKE_BUILD_TYPE is not set at configure time. # scikit-build-core does the right thing with cmake.build-type and # adds the corresponding --config but does not populate CMAKE_BUILD_TYPE - # for those. On this specific case, we set the default to "RELEASE" - # as it's the most common build type for users building from source. - # This is mainly relevant for our Windows wheels, which are built with - # Visual Studio and thus use a multi-config generator with Release. - # As a note this is only to populate config_internal.h.cmake. + # for those. On this specific case, we set the default to "RELEASE". + # The actual build type is injected through target compile definitions + # for multi-config generators. set(UPPERCASE_PYBUILD_TYPE "RELEASE") endif() @@ -515,6 +513,9 @@ else() endif() target_link_libraries(arrow_python PUBLIC Python3::NumPy) target_compile_definitions(arrow_python PRIVATE ARROW_PYTHON_EXPORTING) +if(CMAKE_CONFIGURATION_TYPES) + target_compile_definitions(arrow_python PRIVATE PYARROW_BUILD_TYPE="$<CONFIG>") +endif() set_target_properties(arrow_python PROPERTIES VERSION "${PYARROW_FULL_SO_VERSION}" SOVERSION "${PYARROW_SO_VERSION}") install(TARGETS arrow_python diff --git i/python/pyarrow/src/arrow/python/config_internal.h.cmake w/python/pyarrow/src/arrow/python/config_internal.h.cmake index e8a6e78c48..f76edccb69 100644 --- i/python/pyarrow/src/arrow/python/config_internal.h.cmake +++ w/python/pyarrow/src/arrow/python/config_internal.h.cmake @@ -15,4 +15,6 @@ // specific language governing permissions and limitations // under the License. -#define PYARROW_BUILD_TYPE "@UPPERCASE_PYBUILD_TYPE@" \ No newline at end of file +#ifndef PYARROW_BUILD_TYPE +#define PYARROW_BUILD_TYPE "@UPPERCASE_PYBUILD_TYPE@" +#endif ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
