h-vetinari commented on PR #1889: URL: https://github.com/apache/orc/pull/1889#issuecomment-2071079098
> @h-vetinari @xhochy Could you help check if this breaks conda? Thanks for the ping - happy to provide feedback and to try things out! CC also @nehaljwani, who has been taking care of orc in conda-forge for much longer than I have. > I saw there are some patches https://github.com/conda-forge/orc-feedstock/tree/main/recipe/patches. Is it possible to add `ORC_PACKAGE_KIND=conda` with some code change to remove the patches? I just went through the patches in the feedstock (after rebasing them on current `main` here): * [`0001-Deactivate-aggressive-failures-on-warnings.patch`](https://github.com/conda-forge/orc-feedstock/blob/8586762a73116fec8053ac0682f9a26e47c62955/recipe/patches/0001-Deactivate-aggressive-failures-on-warnings.patch) - we don't want `-Wall -Werror` by default, but this patch comes a time before https://github.com/apache/orc/commit/b7fd75ce616cb0a6368fb48b61eccb6685254e64 existed, so we can probably just use that option * [`0002-Don-t-force-orc-to-be-a-static-library-let-end-user-.patch`](https://github.com/conda-forge/orc-feedstock/blob/8586762a73116fec8053ac0682f9a26e47c62955/recipe/patches/0002-Don-t-force-orc-to-be-a-static-library-let-end-user-.patch) - this is really important to us (we need shared libs). ORC should not hardcode that the library must be static, but respect `-DBUILD_SHARED_LIBS=ON` * [`0003-CMake-Add-more-hints-for-libraries-on-Windows.patch`](https://github.com/conda-forge/orc-feedstock/blob/8586762a73116fec8053ac0682f9a26e47c62955/recipe/patches/0003-CMake-Add-more-hints-for-libraries-on-Windows.patch) - this could very likely be absorbed into this PR as follows: ```diff diff --git a/cmake_modules/FindLZ4.cmake b/cmake_modules/FindLZ4.cmake index b1557f496..8061dda38 100644 --- a/cmake_modules/FindLZ4.cmake +++ b/cmake_modules/FindLZ4.cmake @@ -33,7 +33,7 @@ find_path (LZ4_INCLUDE_DIR lz4.h HINTS NO_DEFAULT_PATH PATH_SUFFIXES "include") -find_library (LZ4_LIBRARY NAMES lz4 HINTS +find_library (LZ4_LIBRARY NAMES lz4 liblz4 HINTS ${_lz4_path} PATH_SUFFIXES "lib" "lib64") ``` * [`0004-don-t-force-lib-destination.patch`](https://github.com/conda-forge/orc-feedstock/blob/8586762a73116fec8053ac0682f9a26e47c62955/recipe/patches/0004-don-t-force-lib-destination.patch) - could be adopted here ```diff diff --git a/c++/src/CMakeLists.txt b/c++/src/CMakeLists.txt index 3612c1c8f..3ebabe10d 100644 --- a/c++/src/CMakeLists.txt +++ b/c++/src/CMakeLists.txt @@ -234,4 +234,4 @@ endif () add_dependencies(orc orc-format_ep) -install(TARGETS orc DESTINATION lib) +install(TARGETS orc DESTINATION "${CMAKE_INSTALL_LIBDIR}") ``` * [`0005-Check-for-protobuf-config-based-module.patch`](https://github.com/conda-forge/orc-feedstock/blob/8586762a73116fec8053ac0682f9a26e47c62955/recipe/patches/0005-Check-for-protobuf-config-based-module.patch) - already done in #1529 So aside from patch Nr. 2 (if ORC insists to hardcode the `STATIC` bit), it should be possible to avoid the need for `ORC_PACKAGE_KIND=conda` in the first place, AFAICT. -- 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]
