arrow: fix recipe when arrow/*:parquet=True #24044
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
| from conan.errors import ConanInvalidConfiguration, ConanException | ||
| from conan.tools.build import check_min_cppstd, cross_building | ||
| from conan.tools.cmake import CMake, CMakeDeps, CMakeToolchain, cmake_layout | ||
| from conan.tools.files import apply_conandata_patches, copy, export_conandata_patches, get, rmdir, save, replace_in_file | ||
| from conan.tools.microsoft import is_msvc, is_msvc_static_runtime | ||
| from conan.tools.scm import Version | ||
|
|
||
|
|
@@ -72,7 +72,7 @@ class ArrowConan(ConanFile): | |
| "shared": False, | ||
| "fPIC": True, | ||
| "gandiva": False, | ||
| "parquet": True, | ||
| "skyhook": False, | ||
| "substrait": False, | ||
| "acero": False, | ||
|
|
@@ -87,7 +87,7 @@ class ArrowConan(ConanFile): | |
| "simd_level": "default", | ||
| "runtime_simd_level": "max", | ||
| "with_backtrace": False, | ||
| "with_boost": True, | ||
| "with_brotli": False, | ||
| "with_bz2": False, | ||
| "with_csv": False, | ||
|
|
@@ -101,7 +101,7 @@ class ArrowConan(ConanFile): | |
| "with_glog": False, | ||
| "with_grpc": False, | ||
| "with_json": False, | ||
| "with_thrift": True, | ||
| "with_llvm": False, | ||
| "with_openssl": False, | ||
| "with_opentelemetry": False, | ||
|
|
@@ -112,7 +112,7 @@ class ArrowConan(ConanFile): | |
| "with_utf8proc": False, | ||
| "with_lz4": False, | ||
| "with_snappy": False, | ||
| "with_zlib": True, | ||
| "with_zstd": False, | ||
| } | ||
| short_paths = True | ||
|
|
@@ -162,7 +162,7 @@ def _requires_rapidjson(self): | |
|
|
||
| def requirements(self): | ||
| if self.options.with_thrift: | ||
| self.requires("thrift/0.20.0") | ||
| if self.options.with_protobuf: | ||
| self.requires("protobuf/3.21.12") | ||
| if self.options.with_jemalloc: | ||
|
|
@@ -202,12 +202,15 @@ def requirements(self): | |
| if self.options.with_snappy: | ||
| self.requires("snappy/1.1.9") | ||
| if self.options.get_safe("simd_level") != None or \ | ||
| self.options.get_safe("runtime_simd_level") != None: | ||
| if Version(self.version) < 8: | ||
| self.requires("xsimd/9.0.1") | ||
| else: | ||
| self.requires("xsimd/13.0.0") | ||
| if self.options.with_zlib: | ||
| self.requires("zlib/[>=1.2.11 <2]") | ||
| if self.options.with_zstd: | ||
| self.requires("zstd/[>=1.5 <1.6]") | ||
| if self.options.with_re2: | ||
| self.requires("re2/20230301") | ||
| if self.options.with_utf8proc: | ||
|
|
@@ -228,18 +231,18 @@ def validate(self): | |
| # From https://github.com/conan-io/conan-center-index/pull/23163#issuecomment-2039808851 | ||
| if self.options.gandiva: | ||
| if not self.options.with_re2: | ||
| raise ConanException("'with_re2' option should be True when 'gandiva=True'") | ||
| if not self.options.with_boost: | ||
| raise ConanException("'with_boost' option should be True when 'gandiva=True'") | ||
| if not self.options.with_utf8proc: | ||
| raise ConanException("'with_utf8proc' option should be True when 'gandiva=True'") | ||
| if self.options.with_thrift and not self.options.with_boost: | ||
| raise ConanException("'with_boost' option should be True when 'thrift=True'") | ||
| if self.options.parquet: | ||
| if not self.options.with_thrift: | ||
| raise ConanException("'with_thrift' option should be True when 'parquet=True'") | ||
| if self.options.with_flight_rpc and not self.options.with_protobuf: | ||
| raise ConanException("'with_protobuf' option should be True when 'with_flight_rpc=True'") | ||
|
|
||
| if self.settings.compiler.get_safe("cppstd"): | ||
| check_min_cppstd(self, self._min_cppstd) | ||
|
|
@@ -261,6 +264,11 @@ def validate(self): | |
| if self.dependencies["jemalloc"].options.enable_cxx: | ||
| raise ConanInvalidConfiguration("jemmalloc.enable_cxx of a static jemalloc must be disabled") | ||
|
|
||
| if self.options.with_thrift and not self.options.with_zlib: | ||
|
There was a problem hiding this comment. There are more checks in There was a problem hiding this comment. That file is over 5000 lines long :D. I do not have the bandwidth to read through it to see what's going on. Otherwise I suggest we leave this to be addressed in another PR. There was a problem hiding this comment. This is the only one that stood out to me, so I'm happy with keeping that on a follow-up PR, yes :) |
||
| raise ConanInvalidConfiguration("arrow:with_thrift requires arrow:with_zlib") | ||
|
|
||
| if self.options.parquet and not self.options.with_thrift: | ||
| raise ConanInvalidConfiguration("arrow:parquet requires arrow:with_thrift") | ||
|
|
||
| def build_requirements(self): | ||
| if Version(self.version) >= "13.0.0": | ||
|
|
@@ -318,6 +326,7 @@ def generate(self): | |
| tc.variables["GLOG_SOURCE"] = "SYSTEM" | ||
| tc.variables["ARROW_WITH_BACKTRACE"] = bool(self.options.with_backtrace) | ||
| tc.variables["ARROW_WITH_BROTLI"] = bool(self.options.with_brotli) | ||
| tc.variables["ARROW_WITH_RE2"] = bool(self.options.with_re2) | ||
| tc.variables["brotli_SOURCE"] = "SYSTEM" | ||
| if self.options.with_brotli: | ||
| tc.variables["ARROW_BROTLI_USE_SHARED"] = bool(self.dependencies["brotli"].options.shared) | ||
|
|
@@ -349,8 +358,10 @@ def generate(self): | |
| tc.variables["ARROW_ZSTD_USE_SHARED"] = bool(self.dependencies["zstd"].options.shared) | ||
| tc.variables["ORC_SOURCE"] = "SYSTEM" | ||
| tc.variables["ARROW_WITH_THRIFT"] = bool(self.options.with_thrift) | ||
| tc.variables["ARROW_THRIFT"] = bool(self.options.with_thrift) | ||
| tc.variables["Thrift_SOURCE"] = "SYSTEM" | ||
| if self.options.with_thrift: | ||
| tc.variables["ARROW_THRIFT"] = True | ||
|
There was a problem hiding this comment. As far as I can see, this variable was not set, but maybe we would need to set There was a problem hiding this comment. I am not super familiar with Arrow's code base, so I am not 100% sure on what's the difference between A quick search on Arrow's codebase suggests that $ find arrow -type f -exec grep -P "\bARROW_THRIFT\b" {} +
arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake:if(ARROW_THRIFT)
arrow/python/build/lib.linux-x86_64-cpython-312/cmake_modules/ThirdpartyToolchain.cmake:if(ARROW_THRIFT)Where it seems to be used only to turn on ZLIB support - link if(ARROW_THRIFT)
set(ARROW_WITH_ZLIB ON)
endif()So I think setting both |
||
| tc.variables["THRIFT_VERSION"] = bool(self.dependencies["thrift"].ref.version) # a recent thrift does not require boost | ||
| tc.variables["ARROW_THRIFT_USE_SHARED"] = bool(self.dependencies["thrift"].options.shared) | ||
| tc.variables["ARROW_USE_OPENSSL"] = self.options.with_openssl | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| diff --git a/cpp/cmake_modules/FindThriftAlt.cmake b/cpp/cmake_modules/FindThriftAlt.cmake | ||
| index f3e49021d..95177c2a6 100644 | ||
| --- a/cpp/cmake_modules/FindThriftAlt.cmake | ||
| +++ b/cpp/cmake_modules/FindThriftAlt.cmake | ||
| @@ -45,22 +45,21 @@ endif() | ||
| # * https://github.com/apache/thrift/pull/2725 | ||
perseoGI marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| # * https://github.com/apache/thrift/pull/2726 | ||
| # * https://github.com/conda-forge/thrift-cpp-feedstock/issues/68 | ||
| -if(NOT WIN32) | ||
| - set(find_package_args "") | ||
| - if(ThriftAlt_FIND_VERSION) | ||
| - list(APPEND find_package_args ${ThriftAlt_FIND_VERSION}) | ||
| - endif() | ||
| - if(ThriftAlt_FIND_QUIETLY) | ||
| - list(APPEND find_package_args QUIET) | ||
| - endif() | ||
| - find_package(Thrift ${find_package_args}) | ||
| - if(Thrift_FOUND) | ||
| - set(ThriftAlt_FOUND TRUE) | ||
| - add_executable(thrift::compiler IMPORTED) | ||
| - set_target_properties(thrift::compiler PROPERTIES IMPORTED_LOCATION | ||
| - "${THRIFT_COMPILER}") | ||
| - return() | ||
| - endif() | ||
| + | ||
| +set(find_package_args "") | ||
| +if(ThriftAlt_FIND_VERSION) | ||
| + list(APPEND find_package_args ${ThriftAlt_FIND_VERSION}) | ||
| +endif() | ||
| +if(ThriftAlt_FIND_QUIETLY) | ||
| + list(APPEND find_package_args QUIET) | ||
| +endif() | ||
| +find_package(Thrift ${find_package_args}) | ||
| +if(Thrift_FOUND) | ||
| + set(ThriftAlt_FOUND TRUE) | ||
| + add_executable(thrift::compiler IMPORTED) | ||
| + set_target_properties(thrift::compiler PROPERTIES IMPORTED_LOCATION | ||
| + "${THRIFT_COMPILER}") | ||
| + return() | ||
| endif() | ||
|
|
||
| function(extract_thrift_version) | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| diff --git a/cpp/cmake_modules/FindThriftAlt.cmake b/cpp/cmake_modules/FindThriftAlt.cmake | ||
| index f3e49021d..3e63f1edf 100644 | ||
| --- a/cpp/cmake_modules/FindThriftAlt.cmake | ||
| +++ b/cpp/cmake_modules/FindThriftAlt.cmake | ||
| @@ -45,23 +45,23 @@ endif() | ||
| # * https://github.com/apache/thrift/pull/2725 | ||
| # * https://github.com/apache/thrift/pull/2726 | ||
| # * https://github.com/conda-forge/thrift-cpp-feedstock/issues/68 | ||
| -if(NOT WIN32) | ||
| - set(find_package_args "") | ||
| - if(ThriftAlt_FIND_VERSION) | ||
| - list(APPEND find_package_args ${ThriftAlt_FIND_VERSION}) | ||
| - endif() | ||
| - if(ThriftAlt_FIND_QUIETLY) | ||
| - list(APPEND find_package_args QUIET) | ||
| - endif() | ||
| - find_package(Thrift ${find_package_args}) | ||
| - if(Thrift_FOUND) | ||
| - set(ThriftAlt_FOUND TRUE) | ||
| - add_executable(thrift::compiler IMPORTED) | ||
| - set_target_properties(thrift::compiler PROPERTIES IMPORTED_LOCATION | ||
| - "${THRIFT_COMPILER}") | ||
| - return() | ||
| - endif() | ||
| + | ||
| +set(find_package_args "") | ||
| +if(ThriftAlt_FIND_VERSION) | ||
| + list(APPEND find_package_args ${ThriftAlt_FIND_VERSION}) | ||
| +endif() | ||
| +if(ThriftAlt_FIND_QUIETLY) | ||
| + list(APPEND find_package_args QUIET) | ||
| endif() | ||
| +find_package(Thrift ${find_package_args}) | ||
| +if(Thrift_FOUND) | ||
| + set(ThriftAlt_FOUND TRUE) | ||
| + add_executable(thrift::compiler IMPORTED) | ||
| + set_target_properties(thrift::compiler PROPERTIES IMPORTED_LOCATION | ||
| + "${THRIFT_COMPILER}") | ||
| + return() | ||
| +endif() | ||
| + | ||
|
|
||
| function(extract_thrift_version) | ||
| if(ThriftAlt_INCLUDE_DIR) | ||
| diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt | ||
| index 93f2e72d8..e00f73f7d 100644 | ||
| --- a/cpp/src/parquet/CMakeLists.txt | ||
| +++ b/cpp/src/parquet/CMakeLists.txt | ||
| @@ -262,11 +262,11 @@ if(NOT PARQUET_MINIMAL_DEPENDENCY) | ||
|
|
||
| # These are libraries that we will link privately with parquet_shared (as they | ||
| # do not need to be linked transitively by other linkers) | ||
| - list(APPEND PARQUET_SHARED_PRIVATE_LINK_LIBS thrift::thrift) | ||
| + list(APPEND PARQUET_SHARED_PRIVATE_LINK_LIBS Boost::headers thrift::thrift) | ||
robomics marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| # Link publicly with parquet_static (because internal users need to | ||
| # transitively link all dependencies) | ||
| - list(APPEND PARQUET_STATIC_LINK_LIBS thrift::thrift) | ||
| + list(APPEND PARQUET_STATIC_LINK_LIBS Boost::headers thrift::thrift) | ||
| if(NOT THRIFT_VENDORED) | ||
| list(APPEND PARQUET_STATIC_INSTALL_INTERFACE_LIBS thrift::thrift) | ||
| endif() | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt | ||
| index dff5b1a59..1189a2957 100644 | ||
| --- a/cpp/CMakeLists.txt | ||
| +++ b/cpp/CMakeLists.txt | ||
| @@ -161,11 +161,7 @@ else() | ||
| set(MSVC_TOOLCHAIN FALSE) | ||
| endif() | ||
|
|
||
| -find_package(ClangTools) | ||
| -find_package(InferTools) | ||
| -if("$ENV{CMAKE_EXPORT_COMPILE_COMMANDS}" STREQUAL "1" | ||
| - OR CLANG_TIDY_FOUND | ||
| - OR INFER_FOUND) | ||
| +if("$ENV{CMAKE_EXPORT_COMPILE_COMMANDS}" STREQUAL "1") | ||
| # Generate a Clang compile_commands.json "compilation database" file for use | ||
| # with various development tools, such as Vim's YouCompleteMe plugin. | ||
| # See http://clang.llvm.org/docs/JSONCompilationDatabase.html |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good reason to have this set to true? Is this just for testing the changes n the CI? We could then revert it now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
with_thriftneeds to be True in order to build Arrow with Parquet support (which is now enabled by default based on #24044 (comment)).