Arrow: Refactor auto options #23163

danimtb · 2024-03-18T15:34:47Z

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)

Includes changes from Arrow: remove dead code danimtb/conan-center-index#6 that remove some dead code from previous versions that are not built anymore.

I've read the contributing guidelines.
I've used a recent Conan client version close to the currently deployed.
I've tried at least one configuration locally with the conan-center hook activated.

ericLemanissierBot · 2024-03-18T15:37:17Z

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.

valgur

This is definitely a much saner approach. Thanks!

recipes/arrow/all/conanfile.py

danimtb · 2024-03-21T13:51:15Z

Waiting for changes at #23185

danimtb · 2024-03-22T11:58:52Z

@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

recipes/arrow/all/conanfile.py

danimtb · 2024-04-02T15:18:01Z

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.

danimtb · 2024-04-03T14:30:41Z

I have opened an issue in the arrow project to ask for some collaboration apache/arrow#40979

kou · 2024-04-05T09:26:26Z

I'm an Apache Arrow developer. What should I answer to complete this PR?

danimtb · 2024-04-05T10:06:08Z

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 False, however, there are some of them that were quite dependant on their "auto" behavior to other ones and we added some warnings in the recipe:

        # Warn/error on some option values
        if not self.options.with_boost:
            if self.options.gandiva:
                self.output.warning("Option 'with_boost=True' could be required when option 'gandiva=True'")
        if not self.options.with_re2:
            if self.options.gandiva or self.options.parquet:
                self.output.warning("Option 'with_re2=True' could be required when option 'gandiva=True' or 'parquet=True'")
            if self.options.compute or self.options.dataset_modules:
                self.output.warning("Option 'with_re2=True' could be required when 'compute=True' or 'dataset_modules=True'")
        if not self.options.with_jemalloc:
            if "BSD" in self.settings.os:
                self.output.warning("Option 'with_jemalloc=True' could be required for BSD in settings.os")

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:

Should with_jemalloc (CMake's ARROW_JEMALLOC) option be True in BSD platforms? Are there any other platforms where jemalloc should be used mandatory?
Should Boost be required whenever the gandiva (ARROW_GANDIVA) option is activated?
Should re2 dependency be required whenever gandiva, parquet (ARROW_PARQUET), compute (ARROW_COMPUTE) or dataset_modules (ARROW_DATASET) are used?

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! 😄

kou · 2024-04-05T13:28:49Z

Should with_jemalloc (CMake's ARROW_JEMALLOC) option be True in BSD platforms? Are there any other platforms where jemalloc should be used mandatory?

We don't need to use True for with_jemalloc on BSD platforms. There are no platforms that require jemalloc.

(In general, jemalloc is better than system allocator. So we recommend jemalloc but it's not required.)

Should Boost be required whenever the gandiva (ARROW_GANDIVA) option is activated?

Yes.

Should re2 dependency be required whenever gandiva, parquet (ARROW_PARQUET), compute (ARROW_COMPUTE) or dataset_modules (ARROW_DATASET) are used?

No. re2 is required only for gandiva.

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.

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...

danimtb · 2024-04-09T10:16:28Z

Thanks @kou for the answers. I will update the recipe accordingly! 😄

conan-center-bot · 2024-04-09T13:06:22Z

Conan v1 pipeline ✔️

All green in build 15 (b25d3f56f9623a7e6dcc790bd7ec2fe9ad80708b):

arrow/14.0.2:
All packages built successfully! (All logs)
arrow/15.0.0:
All packages built successfully! (All logs)
arrow/13.0.0:
All packages built successfully! (All logs)
arrow/12.0.1:
All packages built successfully! (All logs)
arrow/8.0.0:
All packages built successfully! (All logs)
arrow/7.0.0:
All packages built successfully! (All logs)
arrow/14.0.1:
All packages built successfully! (All logs)
arrow/8.0.1:
All packages built successfully! (All logs)
arrow/14.0.0:
All packages built successfully! (All logs)
arrow/12.0.0:
All packages built successfully! (All logs)
arrow/11.0.0:
All packages built successfully! (All logs)
arrow/10.0.1:
All packages built successfully! (All logs)
arrow/10.0.0:
All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 14 (b25d3f56f9623a7e6dcc790bd7ec2fe9ad80708b):

arrow/14.0.2:
All packages built successfully! (All logs)
arrow/15.0.0:
All packages built successfully! (All logs)
arrow/10.0.0:
All packages built successfully! (All logs)
arrow/14.0.0:
All packages built successfully! (All logs)
arrow/14.0.1:
All packages built successfully! (All logs)
arrow/13.0.0:
All packages built successfully! (All logs)
arrow/12.0.0:
All packages built successfully! (All logs)
arrow/11.0.0:
All packages built successfully! (All logs)
arrow/12.0.1:
All packages built successfully! (All logs)
arrow/7.0.0:
All packages built successfully! (All logs)
arrow/10.0.1:
All packages built successfully! (All logs)
arrow/8.0.0:
All packages built successfully! (All logs)
arrow/8.0.1:
All packages built successfully! (All logs)

valgur · 2024-04-09T16:13:52Z

recipes/arrow/all/conanfile.py

@@ -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 \


Suggested change

if self.options.get_safe("simd_level") != None or \

if self.options.get_safe("simd_level") is not None or \

valgur · 2024-04-09T16:14:12Z

recipes/arrow/all/conanfile.py

@@ -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:


Suggested change

self.options.get_safe("runtime_simd_level") != None:

self.options.get_safe("runtime_simd_level") is not None:

valgur · 2024-04-09T16:18:11Z

recipes/arrow/all/conanfile.py

+            # FIXME: only headers components is used
+            self.cpp_info.components["libarrow"].requires.append("boost::boost")


Suggested change

# FIXME: only headers components is used

self.cpp_info.components["libarrow"].requires.append("boost::boost")

self.cpp_info.components["libarrow"].requires.append("boost::headers")

wgtmac · 2024-04-10T15:51:28Z

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

wgtmac · 2024-04-11T05:13:34Z

I have reported an issue with the new recipe: #23475. Could you please take a look? @danimtb

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

* 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>

Arrow: Refactor auto options

7efb057

This was referenced Mar 18, 2024

[service] Potentially conflicting recipes #3571

Closed

(#22506) arrow: fix config_options #22508

Closed

add todo

f13504d

conan-center-bot added the Failed label Mar 18, 2024

This comment has been minimized.

Sign in to view

valgur suggested changes Mar 19, 2024

View reviewed changes

recipes/arrow/all/conanfile.py Show resolved Hide resolved

recipes/arrow/all/conanfile.py Show resolved Hide resolved

valgur mentioned this pull request Mar 19, 2024

arrow: fix broken option handling #21669

Closed

danimtb added 3 commits March 19, 2024 11:21

make boost compulsory in msvc

527172d

enable parquet and thrift by default

796cc75

bump boost version to avoid conflicts with thrift's

e368865

This comment has been minimized.

Sign in to view

try with re2

79b9991

This comment has been minimized.

Sign in to view

bool compute

7848b47

This comment has been minimized.

Sign in to view

back to previous default values

596e566

danimtb mentioned this pull request Mar 20, 2024

Arrow: Remove older versions not compatible with Macos amrv8 #23185

Merged

3 tasks

This comment has been minimized.

Sign in to view

avoid config

d4ec621

This comment has been minimized.

Sign in to view

Merge branch 'master' into feature/arrow_options_refactor

c71fe29

This comment has been minimized.

Sign in to view

fix

c1a716d

conan-center-bot removed the Failed label Mar 22, 2024

This comment has been minimized.

Sign in to view

danimtb marked this pull request as ready for review March 22, 2024 12:20

This comment has been minimized.

Sign in to view

uilianries reviewed Apr 1, 2024

View reviewed changes

recipes/arrow/all/conanfile.py Outdated Show resolved Hide resolved

Update recipes/arrow/all/conanfile.py

c9d0621

This comment has been minimized.

Sign in to view

ericLemanissier approved these changes Apr 2, 2024

View reviewed changes

danimtb mentioned this pull request Apr 3, 2024

[Packaging][Conan] Clarification on "auto"option values in Conan recipe for Arrow C++ Library apache/arrow#40979

Closed

ericLemanissierBot mentioned this pull request Apr 4, 2024

arrow: simplify recipe #23363

Closed

3 tasks

remove warnings

b25d3f5

ericLemanissier approved these changes Apr 9, 2024

View reviewed changes

RubenRBS approved these changes Apr 9, 2024

View reviewed changes

valgur reviewed Apr 9, 2024

View reviewed changes

conan-center-bot merged commit aab7658 into conan-io:master Apr 9, 2024
73 checks passed

This was referenced Apr 9, 2024

[package] arrow/15.0.0: user options are not respected #22506

Closed

[package] arrow/14.0.1: Fails to compile with arrow/*:parquet=True due to missing Thrift dependency #21627

Closed

wgtmac mentioned this pull request Apr 10, 2024

GH-35804: [CI][Packaging][Conan] Synchronize upstream conan apache/arrow#39729

Merged

dvirtz added a commit to dvirtz/vscode-parquet-viewer that referenced this pull request Apr 12, 2024

build: go back to conanfile.txt

c4b6fba

no need to bypass recipe bugs fixed in conan-io/conan-center-index#23163

dvirtz mentioned this pull request Apr 12, 2024

fix: look for prebuilds in the root path dvirtz/vscode-parquet-viewer#123

Merged

dvirtz added a commit to dvirtz/vscode-parquet-viewer that referenced this pull request Apr 12, 2024

build: go back to conanfile.txt

0a16aec

no need to bypass recipe bugs fixed in conan-io/conan-center-index#23163

dvirtz added a commit to dvirtz/vscode-parquet-viewer that referenced this pull request Apr 12, 2024

build: go back to conanfile.txt

273aaf9

no need to bypass recipe bugs fixed in conan-io/conan-center-index#23163

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arrow: Refactor auto options #23163

Arrow: Refactor auto options #23163

danimtb commented Mar 18, 2024 •

edited

ericLemanissierBot commented Mar 18, 2024 •

edited

This comment has been minimized.

valgur left a comment

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.

danimtb commented Mar 21, 2024

This comment has been minimized.

This comment has been minimized.

danimtb commented Mar 22, 2024

This comment has been minimized.

This comment has been minimized.

danimtb commented Apr 2, 2024

danimtb commented Apr 3, 2024

kou commented Apr 5, 2024

danimtb commented Apr 5, 2024

kou commented Apr 5, 2024

danimtb commented Apr 9, 2024

conan-center-bot commented Apr 9, 2024

valgur Apr 9, 2024

valgur Apr 9, 2024

valgur Apr 9, 2024

wgtmac commented Apr 10, 2024

wgtmac commented Apr 11, 2024

	if self.options.get_safe("simd_level") != None or \
	if self.options.get_safe("simd_level") is not None or \

	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")

Arrow: Refactor auto options #23163

Arrow: Refactor auto options #23163

Conversation

danimtb commented Mar 18, 2024 • edited

ericLemanissierBot commented Mar 18, 2024 • edited

This comment has been minimized.

valgur left a comment

Choose a reason for hiding this comment

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.

danimtb commented Mar 21, 2024

This comment has been minimized.

This comment has been minimized.

danimtb commented Mar 22, 2024

This comment has been minimized.

This comment has been minimized.

danimtb commented Apr 2, 2024

danimtb commented Apr 3, 2024

kou commented Apr 5, 2024

danimtb commented Apr 5, 2024

kou commented Apr 5, 2024

danimtb commented Apr 9, 2024

conan-center-bot commented Apr 9, 2024

Conan v1 pipeline ✔️

Conan v2 pipeline ✔️

valgur Apr 9, 2024

Choose a reason for hiding this comment

valgur Apr 9, 2024

Choose a reason for hiding this comment

valgur Apr 9, 2024

Choose a reason for hiding this comment

wgtmac commented Apr 10, 2024

wgtmac commented Apr 11, 2024

danimtb commented Mar 18, 2024 •

edited

ericLemanissierBot commented Mar 18, 2024 •

edited