arrow: fix recipe when arrow/*:parquet=True #24044

robomics · 2024-05-20T19:28:37Z

Specify library name and version: arrow/all

When the arrow package is built with arrow/*:parquet=True, arrow/*:with_thrift=True is also required.
Thrift is found by Arrow by calling FindThriftAlt.cmake, which uses some custom logic to define thrift::thrift when the project is compiled on Windows.

This causes the transitive_header=True specified in Thrift's recipe to be ignored.

conan-center-index/recipes/thrift/all/conanfile.py

Lines 69 to 70 in b4b8a8d

    
           def requirements(self): 
        
               self.requires("boost/1.84.0", transitive_headers=True)

This PR patches FindThriftAlt.cmake so that find_package(Thrift) is always called.

Fixes #20082 (note that without the patch I am unable to build the package using MSVC regardless of the build_type).

While debugging the above issue I also realized that ARROW_WITH_RE2 was not explicitly set by the recipe, which on my machine causes compilation to fail for v11.0.0. This PR also addresses this issue.

Finally, I am bumping thrift, xsimd, and zstdto avoid version conflicts with other packages on cci.

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.

valgur · 2024-05-21T07:35:10Z

Thanks! Please also set parquet=True in default_options as well. It was only left disabled due to build issues in the last PR and this one should probably fix these.

robomics · 2024-05-21T08:47:38Z

Another thing I noticed, is that arrow always uses a vendored version of mimalloc: https://github.com/apache/arrow/blob/b2e8c33c86c819b167a1cbca834da3c9047a9350/cpp/cmake_modules/ThirdpartyToolchain.cmake#L2197-L2204

Should I drop mimalloc from the requirement list (but leave the option to toggle mimalloc on or off)?

robomics · 2024-05-23T07:48:28Z

@RubenRBS I think this is ready for review :).

The conan v2 pipeline failure seems to be due to the same issue described in #24011.

robomics · 2024-10-04T10:16:46Z

@perseoGI CI is green! :)

AbrilRBS

Some comments from a review alongside @perseoGI

AbrilRBS · 2024-10-15T09:53:52Z

recipes/arrow/all/conanfile.py

@@ -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 are more checks in ThirdpartyToolchain.cmake that might be worth to reflect in the recipe, instead of relying on autodiscovery

That file is over 5000 lines long :D. I do not have the bandwidth to read through it to see what's going on.
If you have some specific checks in mind?

Otherwise I suggest we leave this to be addressed in another PR.

This is the only one that stood out to me, so I'm happy with keeping that on a follow-up PR, yes :)

AbrilRBS · 2024-10-15T09:54:50Z

recipes/arrow/all/conanfile.py

@@ -101,7 +101,7 @@ class ArrowConan(ConanFile):
        "with_glog": False,
        "with_grpc": False,
        "with_json": False,
-        "with_thrift": False,
+        "with_thrift": True,


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

Yes! with_thrift needs to be True in order to build Arrow with Parquet support (which is now enabled by default based on #24044 (comment)).

AbrilRBS · 2024-10-15T09:55:28Z

recipes/arrow/all/conanfile.py

@@ -351,6 +360,7 @@ def generate(self):
        tc.variables["ARROW_WITH_THRIFT"] = bool(self.options.with_thrift)
        tc.variables["Thrift_SOURCE"] = "SYSTEM"
        if self.options.with_thrift:
+            tc.variables["ARROW_THRIFT"] = True


As far as I can see, this variable was not set, but maybe we would need to set ARROW_WITH_THRIFT instead? Am I missing something? Do you have any insight? Thanks!

I am not super familiar with Arrow's code base, so I am not 100% sure on what's the difference between ARROW_WITH_THRIFT and ARROW_THRIFT.

A quick search on Arrow's codebase suggests that ARROW_THRIFT is only referenced in

$ 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 ARROW_WITH_THRIFT and ARROW_THRIFT is ok (will push a commit to do so ASAP).

AbrilRBS

Didn't mean to approve just yet!

Co-authored-by: Abril Rincón Blanco <[email protected]> Co-authored-by: PerseoGI <[email protected]>

conan-center-bot · 2024-10-29T16:46:23Z

Conan v1 pipeline ✔️

Warning

Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement.

All green in build 28 (b776db92e460f2c179a2140de91a76eb55e4d3d3):

arrow/15.0.0:
Built 18 packages out of 22 (All logs)
arrow/16.0.0:
Built 18 packages out of 22 (All logs)
arrow/16.1.0:
Built 18 packages out of 22 (All logs)
arrow/17.0.0:
Built 18 packages out of 22 (All logs)
arrow/11.0.0:
Built 18 packages out of 22 (All logs)
arrow/7.0.0:
All packages built successfully! (All logs)
arrow/10.0.1:
Built 18 packages out of 22 (All logs)
arrow/12.0.0:
Built 18 packages out of 22 (All logs)
arrow/14.0.2:
Built 18 packages out of 22 (All logs)
arrow/8.0.0:
All packages built successfully! (All logs)
arrow/14.0.0:
Built 18 packages out of 22 (All logs)
arrow/12.0.1:
Built 18 packages out of 22 (All logs)
arrow/13.0.0:
Built 18 packages out of 22 (All logs)
arrow/10.0.0:
Built 18 packages out of 22 (All logs)
arrow/14.0.1:
Built 18 packages out of 22 (All logs)
arrow/8.0.1:
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 30 (b776db92e460f2c179a2140de91a76eb55e4d3d3):

arrow/17.0.0:
All packages built successfully! (All logs)
arrow/14.0.1:
All packages built successfully! (All logs)
arrow/16.1.0:
All packages built successfully! (All logs)
arrow/16.0.0:
All packages built successfully! (All logs)
arrow/14.0.0:
All packages built successfully! (All logs)
arrow/13.0.0:
All packages built successfully! (All logs)
arrow/15.0.0:
All packages built successfully! (All logs)
arrow/14.0.2:
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/8.0.0:
All packages built successfully! (All logs)
arrow/12.0.1:
All packages built successfully! (All logs)
arrow/7.0.0:
Built 6 packages out of 10 (All logs)
arrow/10.0.0:
All packages built successfully! (All logs)
arrow/12.0.0:
All packages built successfully! (All logs)
arrow/8.0.1:
All packages built successfully! (All logs)

robomics · 2024-10-29T20:53:59Z

Things are looking good now.
Can you please have a look @AbrilRBS, @perseoGI?

AbrilRBS

This now looks good on my end, will ask @perseoGI to re-review for tomorrow so we can finally get this merged, thanks a lot for your patience @robomics and for taking the time to address our reviews, we appreciate it :)

AbrilRBS · 2024-10-29T22:51:33Z

recipes/arrow/all/conanfile.py

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


This is the only one that stood out to me, so I'm happy with keeping that on a follow-up PR, yes :)

AbrilRBS · 2024-10-29T22:51:49Z

recipes/arrow/all/conanfile.py

@@ -351,6 +360,7 @@ def generate(self):
        tc.variables["ARROW_WITH_THRIFT"] = bool(self.options.with_thrift)
        tc.variables["Thrift_SOURCE"] = "SYSTEM"
        if self.options.with_thrift:
+            tc.variables["ARROW_THRIFT"] = True


perseoGI

Finally! This is merging! Thank you for your patience!!!

robomics · 2024-10-30T10:49:37Z

Awesome!
Thank you both for your comments and help throughout the review :)

* Fix recipe when arrow/*:parquet=True * Add patch description and type annotations * Set parquet=True * Do not look for static analyzers * Bugfix * Bugfix * Refactor patches * Backport fixes from upstream * Patch v17.0.0 * Woops. Forgot to apply patch to v16.1.0 * Bump deps * Bugfix * Add patch_source * Add check to ensure with_boost=True when with_thrift=True * Fix typos * Cleanups * Set ARROW_THRIFT Co-authored-by: Abril Rincón Blanco <[email protected]> Co-authored-by: PerseoGI <[email protected]> * Bugfix * Bugfix --------- Co-authored-by: Abril Rincón Blanco <[email protected]> Co-authored-by: PerseoGI <[email protected]>

robomics added 2 commits May 20, 2024 21:11

Fix recipe when arrow/*:parquet=True

dc958d2

Add patch description and type annotations

a8f9fde

This was referenced May 20, 2024

[service] Potentially conflicting recipes #3571

Closed

arrow: add version 16.1.0 #24011

Merged

conan-center-bot added the Failed label May 21, 2024

This comment has been minimized.

Sign in to view

Set parquet=True

7fd393b

This comment has been minimized.

Sign in to view

robomics added 2 commits May 21, 2024 15:52

Do not look for static analyzers

15a8629

Bugfix

c578dfb

This comment has been minimized.

Sign in to view

Bugfix

64a8976

This comment has been minimized.

Sign in to view

Refactor patches

cba2cd1

conan-center-bot added the Unexpected Error label May 21, 2024

This comment has been minimized.

Sign in to view

robomics closed this May 21, 2024

robomics reopened this May 21, 2024

conan-center-bot removed the Unexpected Error label May 21, 2024

This comment has been minimized.

Sign in to view

Backport fixes from upstream

c46189d

robomics closed this May 22, 2024

robomics reopened this May 22, 2024

This comment has been minimized.

Sign in to view

ghost mentioned this pull request May 26, 2024

arrow: fix target names #24106

Merged

3 tasks

robomics closed this Jun 12, 2024

robomics reopened this Jun 12, 2024

robomics added 2 commits October 3, 2024 12:10

Add check to ensure with_boost=True when with_thrift=True

0059278

Fix typos

90ef254

robomics dismissed perseoGI’s stale review via 90ef254 October 3, 2024 10:11

This comment has been minimized.

Sign in to view

Cleanups

6408788

AbrilRBS approved these changes Oct 15, 2024

View reviewed changes

AbrilRBS requested changes Oct 15, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

Set ARROW_THRIFT

184ae5c

Co-authored-by: Abril Rincón Blanco <[email protected]> Co-authored-by: PerseoGI <[email protected]>

This comment has been minimized.

Sign in to view

robomics requested a review from AbrilRBS October 22, 2024 11:34

conan-center-bot requested a review from franramirez688 October 24, 2024 11:38

robomics closed this Oct 29, 2024

robomics reopened this Oct 29, 2024

This comment has been minimized.

Sign in to view

Bugfix

20124f8

conan-center-bot added the Failed label Oct 29, 2024

This comment has been minimized.

Sign in to view

Bugfix

b776db9

conan-center-bot removed the Failed label Oct 29, 2024

AbrilRBS approved these changes Oct 29, 2024

View reviewed changes

perseoGI approved these changes Oct 30, 2024

View reviewed changes

conan-center-bot merged commit e4ffd89 into conan-io:master Oct 30, 2024
69 checks passed

robomics deleted the fix/arrow branch October 30, 2024 10:49

robomics mentioned this pull request Nov 1, 2024

arrow: add version 18.0.0 #25770

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

arrow: fix recipe when arrow/*:parquet=True #24044

arrow: fix recipe when arrow/*:parquet=True #24044

robomics commented May 20, 2024 •

edited

Loading

This comment has been minimized.

valgur commented May 21, 2024

robomics commented May 21, 2024

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.

robomics commented May 23, 2024 •

edited

Loading

This comment has been minimized.

robomics commented Oct 4, 2024

AbrilRBS left a comment

AbrilRBS Oct 15, 2024

robomics Oct 22, 2024

AbrilRBS Oct 29, 2024

AbrilRBS Oct 15, 2024

robomics Oct 22, 2024 •

edited

Loading

AbrilRBS Oct 15, 2024

robomics Oct 22, 2024

AbrilRBS Oct 29, 2024

AbrilRBS left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

conan-center-bot commented Oct 29, 2024

robomics commented Oct 29, 2024

AbrilRBS left a comment

AbrilRBS Oct 29, 2024

AbrilRBS Oct 29, 2024

perseoGI left a comment

robomics commented Oct 30, 2024

	def requirements(self):
	self.requires("boost/1.84.0", transitive_headers=True)

arrow: fix recipe when arrow/*:parquet=True #24044

arrow: fix recipe when arrow/*:parquet=True #24044

Conversation

robomics commented May 20, 2024 • edited Loading

This comment has been minimized.

valgur commented May 21, 2024

robomics commented May 21, 2024

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.

robomics commented May 23, 2024 • edited Loading

This comment has been minimized.

robomics commented Oct 4, 2024

AbrilRBS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robomics Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

conan-center-bot commented Oct 29, 2024

Conan v1 pipeline ✔️

Conan v2 pipeline ✔️

robomics commented Oct 29, 2024

AbrilRBS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perseoGI left a comment

Choose a reason for hiding this comment

robomics commented Oct 30, 2024

robomics commented May 20, 2024 •

edited

Loading

robomics commented May 23, 2024 •

edited

Loading

robomics Oct 22, 2024 •

edited

Loading