-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Arrow: Refactor auto options #23163
Arrow: Refactor auto options #23163
Conversation
I detected other pull requests that are modifying arrow/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
This comment has been minimized.
This comment has been minimized.
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.
This is definitely a much saner approach. Thanks!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Waiting for changes at #23185 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@valgur did not have much luck activating the parquet option although I think it makes sense. However, I just want to push the refactor of options management in this recipe, so let's leave the option value tweaks for a future PR |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
After discussing the changes of this PR internally, we would like to avoid throwing unnecessary warnings to the user. However, some of the auto logic on options are unknown to us and would like to ask about them upstream in the arrow project. Especially the one regarding jemalloc and BSD platform. |
I have opened an issue in the arrow project to ask for some collaboration apache/arrow#40979 |
I'm an Apache Arrow developer. What should I answer to complete this PR? |
Hi @kou and thanks again for your response. As you can see in the changes of the PR, we are trying to simplify the management of "auto" options (CMake variables) in the Arrow recipe. As some option values affected other options the UX was quite ricky for the user consuming this recipe and the maintenance of the recipe was harder at different points. We reviewed the logic and finally reduced most of the option default values to
We would like to simplify this more but we are not familiar with the intrinsics of the Arrow build, so basically we would need to answer these questions:
Also, we noticed that this recipe was somehow used in the https://github.com/apache/arrow repository, so we want to know if there would be any issue with these changes. Thanks a lot! 😄 |
We don't need to use (In general, jemalloc is better than system allocator. So we recommend jemalloc but it's not required.)
Yes.
No. re2 is required only for
We use this recipe only for CI. We're testing this recipe with unreleased Apache Arrow. It's for smooth release process. We want to avoid that downstreams including this recipe have patches that fixes problems in released Apache Arrow. If we can detect problems before release a new version, downstreams don't need to have patches. :-) But maintaining this recipe (keeping this recipe buildable) in apache/arrow difficult... It's broken recently... |
Thanks @kou for the answers. I will update the recipe accordingly! 😄 |
Conan v1 pipeline ✔️All green in build 15 (
Conan v2 pipeline ✔️
All green in build 14 (
|
@@ -332,8 +200,7 @@ def requirements(self): | |||
self.requires("lz4/1.9.4") | |||
if self.options.with_snappy: | |||
self.requires("snappy/1.1.9") | |||
if Version(self.version) >= "6.0.0" and \ | |||
self.options.get_safe("simd_level") != None or \ | |||
if self.options.get_safe("simd_level") != None or \ |
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.
if self.options.get_safe("simd_level") != None or \ | |
if self.options.get_safe("simd_level") is not None or \ |
@@ -332,8 +200,7 @@ def requirements(self): | |||
self.requires("lz4/1.9.4") | |||
if self.options.with_snappy: | |||
self.requires("snappy/1.1.9") | |||
if Version(self.version) >= "6.0.0" and \ | |||
self.options.get_safe("simd_level") != None or \ | |||
if self.options.get_safe("simd_level") != None or \ | |||
self.options.get_safe("runtime_simd_level") != None: |
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.
self.options.get_safe("runtime_simd_level") != None: | |
self.options.get_safe("runtime_simd_level") is not None: |
# FIXME: only headers components is used | ||
self.cpp_info.components["libarrow"].requires.append("boost::boost") |
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.
# FIXME: only headers components is used | |
self.cpp_info.components["libarrow"].requires.append("boost::boost") | |
self.cpp_info.components["libarrow"].requires.append("boost::headers") |
Happy to see this gets resolved. Thanks! BTW, I port this to the Apache Arrow repo and let's see how it goes: apache/arrow#39729 |
no need to bypass recipe bugs fixed in conan-io/conan-center-index#23163
* Arrow: Refactor auto options * add todo * make boost compulsory in msvc * enable parquet and thrift by default * bump boost version to avoid conflicts with thrift's * try with re2 * bool compute * back to previous default values * avoid config * fix * Arrow: remove dead code (conan-io#6) * remove dead code * fix indentation * Update recipes/arrow/all/conanfile.py * remove warnings --------- Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
no need to bypass recipe bugs fixed in conan-io/conan-center-index#23163
no need to bypass recipe bugs fixed in conan-io/conan-center-index#23163
* Arrow: Refactor auto options * add todo * make boost compulsory in msvc * enable parquet and thrift by default * bump boost version to avoid conflicts with thrift's * try with re2 * bool compute * back to previous default values * avoid config * fix * Arrow: remove dead code (#6) * remove dead code * fix indentation * Update recipes/arrow/all/conanfile.py * remove warnings --------- Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
The aim of this PR is to solve recurrent issues with the options in the Arrow recipe. We have received many PRs trying to fix this situation but instead of making the recipe simpler, it has become complex this the management of "auto" options.
Making the recipe too smart is a way to make the recipe more likely to fail and in the end users may face bad experiences at cosume time.
Hope this helps to improve the situation!
Related to #22508 (comment)