(#7329) Support Gandiva in Arrow 2.0.0 #7331

kexianda · 2021-09-18T12:18:51Z

fix protobuf, re2 and llvm dependencies
fix compile error since the llvm api changed.

Specify library name and version: arrow/2.0.0

In arrow 2.0.0, CCI does not support gandiva component since gandiva depends on llvm. Since CCI has llvm recipe now.
Turn on gandiva component support, and fix some compiling errors.

Tested on
MacOS / Clang 12
Debian / GCC 9.3

I've read the guidelines for contributing.
I've followed the PEP8 style guides for Python code in the recipes.
I've used the latest Conan client version.
I've tried at least one configuration locally with the
conan-center hook activated.

CLAassistant · 2021-09-18T12:18:54Z

All committers have signed the CLA.

ghost · 2021-09-18T12:54:19Z

I detected other pull requests that are modifying arrow/all recipe:

arrow: fix typo #7675

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

jgsogo

The comment about version ranges is important for CCI.

Thanks!

jgsogo · 2021-09-27T09:40:59Z

recipes/arrow/all/conanfile.py

@@ -232,7 +232,7 @@ def requirements(self):
        if self.options.with_json:
            self.requires("rapidjson/1.1.0")
        if self._with_llvm():
-            raise ConanInvalidConfiguration("CCI has no llvm recipe (yet)")
+            self.requires("llvm-core/[>=11.1.0]")


Do not use version ranges in CCI. It might change packageID and consumers will complain about missing packages.

Thanks a lot. Fixed now. now it is "llvm-core/12.0.0"

recipes/arrow/all/conanfile.py

kexianda · 2021-10-09T07:39:09Z

The comment about version ranges is important for CCI.

Thanks!

Thanks a lot. Fixed now.

jgsogo · 2021-10-20T10:25:37Z

recipes/arrow/all/conanfile.py

        if self._with_llvm():
-            self.cpp_info.components["libarrow"].requires.append("llvm::llvm")
+            self.cpp_info.components["libarrow"].requires.append("llvm-core::llvm-core")


Duplicated, right?

Duplicated, right?

yes. Thanks. Fixed.

jgsogo · 2021-10-20T10:28:55Z

recipes/arrow/all/patches/2.0.0-0006-gandiva-llvm-re2.patch

+-  get_target_property(RE2_INCLUDE_DIR RE2::re2 INTERFACE_INCLUDE_DIRECTORIES)
+  # get_target_property(RE2_INCLUDE_DIR RE2::re2 INTERFACE_INCLUDE_DIRECTORIES)


I would keep existing logic, getting the include directories from the target will always work, the variable RE2_INCLUDE_DIR might not be available depending on the generator used.

Suggested change

- get_target_property(RE2_INCLUDE_DIR RE2::re2 INTERFACE_INCLUDE_DIRECTORIES)

+ # get_target_property(RE2_INCLUDE_DIR RE2::re2 INTERFACE_INCLUDE_DIRECTORIES)

- get_target_property(RE2_INCLUDE_DIR RE2::re2 INTERFACE_INCLUDE_DIRECTORIES)

+ get_target_property(RE2_INCLUDE_DIR re2::re2 INTERFACE_INCLUDE_DIRECTORIES)

I would keep existing logic, getting the include directories from the target will always work, the variable RE2_INCLUDE_DIR might not be available depending on the generator used.

make sense. Fixed.

jgsogo · 2021-10-20T10:30:42Z

recipes/arrow/all/patches/2.0.0-0006-gandiva-llvm-re2.patch

+find_package(llvm-core REQUIRED)
+if ( NOT ${llvm-core_VERSION} VERSION_LESS "10.0.0")
+  set(GANDIVA_CXX_STANDARD 14)
+endif()


This can be added to the recipe: if llvm is >=11, then check that the compiler supports c++14 and also set GANDIVA_CXX_STANDARD=14 in the build() method

This can be added to the recipe: if llvm is >=11, then check that the compiler supports c++14 and also set GANDIVA_CXX_STANDARD=14 in the build() method

GANDIVA_CXX_STANDARD also depends on 'CMAKE_CXX_STANDARD'.
And the original CMAKE script has handle this correctly.
So I just revert my redundant change here.

jgsogo · 2021-10-20T10:32:08Z

recipes/arrow/all/patches/2.0.0-0006-gandiva-llvm-re2.patch

+-              $<TARGET_PROPERTY:LLVM::LLVM_INTERFACE,INTERFACE_INCLUDE_DIRECTORIES>
+              lvm-core_INCLUDE_DIRECTORIES


Same here, why not keep using the targets?

Suggested change

- $<TARGET_PROPERTY:LLVM::LLVM_INTERFACE,INTERFACE_INCLUDE_DIRECTORIES>

+ lvm-core_INCLUDE_DIRECTORIES

- $<TARGET_PROPERTY:LLVM::LLVM_INTERFACE,INTERFACE_INCLUDE_DIRECTORIES>

+ $<TARGET_PROPERTY:llvm-core::llvm-core,INTERFACE_INCLUDE_DIRECTORIES>

Same here, why not keep using the targets?

'INTERFACE_INCLUDE_DIRECTORIES' is not the property of target llvm-core::llvm-core.
the target name is llvm-core::core. Fixed now

jgsogo · 2021-10-20T10:32:30Z

recipes/arrow/all/patches/2.0.0-0006-gandiva-llvm-re2.patch

+-                 $<TARGET_PROPERTY:LLVM::LLVM_INTERFACE,INTERFACE_INCLUDE_DIRECTORIES>
+                 llvm-core_INCLUDE_DIRS


Suggested change

- $<TARGET_PROPERTY:LLVM::LLVM_INTERFACE,INTERFACE_INCLUDE_DIRECTORIES>

+ llvm-core_INCLUDE_DIRS

- $<TARGET_PROPERTY:LLVM::LLVM_INTERFACE,INTERFACE_INCLUDE_DIRECTORIES>

+ $<TARGET_PROPERTY:llvm-core::llvm-core,INTERFACE_INCLUDE_DIRECTORIES>

kexianda · 2021-10-26T03:03:47Z

The comment about version ranges is important for CCI.

Thanks!

fixed

uilianries

LGTM

* fix re2 and llvm dependencies * fix compile error since the llvm api changed.

conan-center-bot · 2021-12-03T17:55:35Z

All green in build 8 (8968551c1847c778c60a097b1a6bd961c102134c):

arrow/2.0.0@:
All packages built successfully! (All logs)
arrow/1.0.0@:
All packages built successfully! (All logs)

prince-chrismc · 2022-01-07T18:32:46Z

@jgsogo your requested changes have been applied! please review when you have the chance!

* (conan-io#7329) Support Gandiva in Arrow 2.0.0 * fix re2 and llvm dependencies * fix compile error since the llvm api changed. * minor: use generator expr

…update-from-conan-io * 'master' of github.com:conan-io/conan-center-index: (480 commits) (conan-io#8840) wslay: modernize (conan-io#8830) Deprecate diligentgraphics-spirv-tools (conan-io#8829) Deprecate diligentgraphics-spirv-headers (conan-io#8828) Deprecate diligentgraphics-vulkan-headers (conan-io#8819) nanoflann: add 1.4.2 + modernize (conan-io#8814) coin-lemon: restore .names["pkg_config"] (conan-io#8797) Let contributors know to avoid rebasing (conan-io#8809) libalsa: restore .names["pkg_config"] + add ALSA_CONFIG_DIR to runenv_info (conan-io#8807) pkgconf: restore .names["pkg_config"] (conan-io#8741) spirv-headers: fix pkg_config name + modernize (conan-io#8732) oniguruma: modernize (conan-io#8684) libtiff: modernize (conan-io#8632) cpython: Add PATH variable only if env_vars option is enabled. (conan-io#7331) (conan-io#7329) Support Gandiva in Arrow 2.0.0 (conan-io#8598) add scdoc documentation tool (conan-io#8796) backport-cpp: modernize (conan-io#8793) cpp-ipc: add version 1.2.0 (conan-io#8778) variant-lite: modernize (conan-io#8777) string-view-lite: modernize (conan-io#8775) scope-lite: modernize ...

This comment has been minimized.

Sign in to view

conan-center-bot added the No Beta user label Sep 18, 2021

ghost mentioned this pull request Sep 18, 2021

[service] Potentially conflicting recipes #3571

Closed

conan-center-bot removed the No Beta user label Sep 21, 2021

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Sep 21, 2021

ghost mentioned this pull request Sep 25, 2021

Add Runtime SIMD Level for Apache Arrow #6571

Merged

4 tasks

jgsogo suggested changes Sep 27, 2021

View reviewed changes

kexianda force-pushed the gandiva branch from f5b773e to 6a2d2aa Compare October 9, 2021 07:04

This comment has been minimized.

Sign in to view

ghost mentioned this pull request Oct 13, 2021

arrow: fix typo #7675

Merged

4 tasks

kexianda force-pushed the gandiva branch from 6a2d2aa to d885411 Compare October 20, 2021 03:54

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Oct 20, 2021

jgsogo reviewed Oct 20, 2021

View reviewed changes

conan-center-bot requested a review from SSE4 October 20, 2021 12:00

kexianda force-pushed the gandiva branch from d885411 to dd31451 Compare October 22, 2021 01:17

This comment has been minimized.

Sign in to view

kexianda force-pushed the gandiva branch from dd31451 to c1668ab Compare October 28, 2021 06:18

This comment has been minimized.

Sign in to view

SSE4 requested a review from uilianries October 29, 2021 05:37

uilianries previously approved these changes Nov 1, 2021

View reviewed changes

prince-chrismc mentioned this pull request Nov 1, 2021

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

(conan-io#7329) Support Gandiva in Arrow 2.0.0

3cf42fa

* fix re2 and llvm dependencies * fix compile error since the llvm api changed.

kexianda dismissed uilianries’s stale review via 3cf42fa November 4, 2021 05:05

kexianda force-pushed the gandiva branch from c1668ab to 3cf42fa Compare November 4, 2021 05:05

This comment has been minimized.

Sign in to view

conan-center-bot requested a review from uilianries November 9, 2021 12:03

conan-center-bot requested a review from jgsogo November 19, 2021 12:00

conan-center-bot requested review from danimtb and czoido December 1, 2021 12:00

minor: use generator expr

8968551

prince-chrismc approved these changes Dec 4, 2021

View reviewed changes

uilianries approved these changes Dec 9, 2021

View reviewed changes

danimtb approved these changes Dec 13, 2021

View reviewed changes

conan-center-bot requested a review from memsharded December 27, 2021 12:00

SSE4 approved these changes Jan 7, 2022

View reviewed changes

jgsogo approved these changes Jan 12, 2022

View reviewed changes

conan-center-bot merged commit a4eebcf into conan-io:master Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#7329) Support Gandiva in Arrow 2.0.0 #7331

(#7329) Support Gandiva in Arrow 2.0.0 #7331

kexianda commented Sep 18, 2021 •

edited

Loading

CLAassistant commented Sep 18, 2021 •

edited

Loading

This comment has been minimized.

ghost commented Sep 18, 2021 •

edited by ghost

Loading

This comment has been minimized.

jgsogo left a comment

jgsogo Sep 27, 2021

kexianda Oct 9, 2021

kexianda commented Oct 9, 2021

This comment has been minimized.

This comment has been minimized.

jgsogo Oct 20, 2021

kexianda Oct 22, 2021

jgsogo Oct 20, 2021

kexianda Oct 22, 2021

jgsogo Oct 20, 2021

kexianda Oct 22, 2021

jgsogo Oct 20, 2021

kexianda Oct 22, 2021 •

edited

Loading

jgsogo Oct 20, 2021

This comment has been minimized.

kexianda commented Oct 26, 2021

This comment has been minimized.

uilianries left a comment

This comment has been minimized.

conan-center-bot commented Dec 3, 2021

prince-chrismc commented Jan 7, 2022

		- get_target_property(RE2_INCLUDE_DIR RE2::re2 INTERFACE_INCLUDE_DIRECTORIES)
		+ # get_target_property(RE2_INCLUDE_DIR RE2::re2 INTERFACE_INCLUDE_DIRECTORIES)

		- $<TARGET_PROPERTY:LLVM::LLVM_INTERFACE,INTERFACE_INCLUDE_DIRECTORIES>
		+ lvm-core_INCLUDE_DIRECTORIES

		- $<TARGET_PROPERTY:LLVM::LLVM_INTERFACE,INTERFACE_INCLUDE_DIRECTORIES>
		+ llvm-core_INCLUDE_DIRS

(#7329) Support Gandiva in Arrow 2.0.0 #7331

(#7329) Support Gandiva in Arrow 2.0.0 #7331

Conversation

kexianda commented Sep 18, 2021 • edited Loading

CLAassistant commented Sep 18, 2021 • edited Loading

This comment has been minimized.

ghost commented Sep 18, 2021 • edited by ghost Loading

This comment has been minimized.

jgsogo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kexianda commented Oct 9, 2021

This comment has been minimized.

This comment has been minimized.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kexianda Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

kexianda commented Oct 26, 2021

This comment has been minimized.

uilianries left a comment

Choose a reason for hiding this comment

This comment has been minimized.

conan-center-bot commented Dec 3, 2021

prince-chrismc commented Jan 7, 2022

kexianda commented Sep 18, 2021 •

edited

Loading

CLAassistant commented Sep 18, 2021 •

edited

Loading

ghost commented Sep 18, 2021 •

edited by ghost

Loading

kexianda Oct 22, 2021 •

edited

Loading