Skip to content
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: fix recipe when arrow/*:parquet=True #24044

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

robomics
Copy link
Contributor

@robomics robomics commented May 20, 2024

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.

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.


@conan-center-bot

This comment has been minimized.

@valgur
Copy link
Contributor

valgur commented May 21, 2024

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
Copy link
Contributor Author

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

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@robomics robomics closed this May 22, 2024
@robomics robomics reopened this May 22, 2024
@conan-center-bot

This comment has been minimized.

@robomics
Copy link
Contributor Author

robomics commented May 23, 2024

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

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

@ghost ghost mentioned this pull request May 26, 2024
3 tasks
@robomics robomics closed this Jun 12, 2024
@robomics robomics reopened this Jun 12, 2024
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@robomics
Copy link
Contributor Author

uhm... It looks like the thrift package for v0.20.0 is missing (at least for Linux GCC).
Would you mind looking into this @AbrilRBS?

@conan-center-bot conan-center-bot added Failed Missing dependencies Build failed due missing dependencies in Conan Center labels Aug 14, 2024
@conan-center-bot

This comment has been minimized.

@perseoGI
Copy link
Contributor

@robomics building missing binaries now...

@conan-center-bot conan-center-bot removed the Missing dependencies Build failed due missing dependencies in Conan Center label Sep 8, 2024
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@robomics
Copy link
Contributor Author

Thanks @perseoGI!
CI seems to be green now.

Can you please review this (and also #24934, #24935, and #24936 if appropriate)?

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 22 (952d5c9e9a06f06eb1143f000eede64a7419f5f3):

  • arrow/16.1.0:
    Built 18 packages out of 22 (All logs)

  • arrow/12.0.0:
    Built 18 packages out of 22 (All logs)

  • arrow/15.0.0:
    Built 18 packages out of 22 (All logs)

  • arrow/14.0.1:
    Built 18 packages out of 22 (All logs)

  • arrow/14.0.2:
    Built 18 packages out of 22 (All logs)

  • arrow/11.0.0:
    Built 18 packages out of 22 (All logs)

  • arrow/10.0.0:
    Built 18 packages out of 22 (All logs)

  • arrow/17.0.0:
    Built 18 packages out of 22 (All logs)

  • arrow/13.0.0:
    Built 18 packages out of 22 (All logs)

  • arrow/7.0.0:
    All packages built successfully! (All logs)

  • arrow/16.0.0:
    Built 18 packages out of 22 (All logs)

  • arrow/12.0.1:
    Built 18 packages out of 22 (All logs)

  • arrow/10.0.1:
    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/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 24 (952d5c9e9a06f06eb1143f000eede64a7419f5f3):

  • arrow/16.0.0:
    All packages built successfully! (All logs)

  • arrow/17.0.0:
    All packages built successfully! (All logs)

  • arrow/12.0.0:
    All packages built successfully! (All logs)

  • arrow/12.0.1:
    All packages built successfully! (All logs)

  • arrow/16.1.0:
    All packages built successfully! (All logs)

  • arrow/11.0.0:
    All packages built successfully! (All logs)

  • arrow/15.0.0:
    All packages built successfully! (All logs)

  • arrow/14.0.1:
    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/10.0.0:
    All packages built successfully! (All logs)

  • arrow/10.0.1:
    All packages built successfully! (All logs)

  • arrow/14.0.2:
    All packages built successfully! (All logs)

  • arrow/7.0.0:
    Built 6 packages out of 10 (All logs)

  • arrow/8.0.1:
    All packages built successfully! (All logs)

  • arrow/8.0.0:
    All packages built successfully! (All logs)

@perseoGI
Copy link
Contributor

Thanks @perseoGI! CI seems to be green now.

Can you please review this (and also #24934, #24935, and #24936 if appropriate)?

Could you refresh a lit my memory?
Those PRs are pointing to this one but in this PR we are not aiming to bump xsimd

@robomics
Copy link
Contributor Author

@perseoGI I am bumping xsimd here and in the PRs linked in my other post to address the conflict mentioned in #24382 (review).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] arrow 13.0.0 not building for msvc debug profile
5 participants