-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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-12175: [C++] Fix CMake packages #13892
Conversation
|
38ee685
to
40e2e9d
Compare
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit example-python-minimal-build-ubuntu-venv homebrew-cpp verify-rc-source-python-linux-conda-latest-amd64 wheel-macos-big-sur-cp310-arm64 wheel-manylinux2014-cp310-amd64 wheel-windows-cp310-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit verify-rc-source-python-linux-conda-latest-amd64 wheel-windows-cp310-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit conda-linux-gcc-py310-cpu |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit conda-linux-gcc-py310-cpu |
This comment was marked as outdated.
This comment was marked as outdated.
ARROW-9171 is also fixed. Our CMake packages are broken. For example, find_package(Parquet) doesn't work without specifying CMAKE_MODULE_PATH. find_package(${PACKAGE}) searches ${PREFIX}/${PACKAGE}/${PACKAGE}Config.cmake or ${PREFIX}/${PACKAGE}/Find${PACKAGE}.cmake. But our .cmake files are always installed ${PREFIX}/arrow/. So find_package(Parquet) can't find ${PREFIX}/arrow/FindParquet.cmake because "/arrow/" isn't "/${PACKAGE}". This change fixes this by installing ${PACKAGE}Config.cmake to ${PREFIX}/${PACKAGE}/ instead of ${PREFIX}/arrow/. This also removes all Find${PACKAGE}.cmake. We only provides ${PACKAGE}Config.cmake. Our Find${PACKAGE}.cmake can find ${PACKAGE} by CMake, pkg-config or manual .so/.h search. But we don't need to support pkg-config nor manual .so/.h search. We can use ${PACKAGE}Config.cmake to support CMake package search. So this removes all Find${PACKAGE}.cmake. This also introduces namespace to our CMake targets. For example, arrow_shared is exported as Arrow::arrow_shared and parquet_static is exported as Parquet::parquet_static. But no namespace targets such as arrow_shared and parquet_static are still also exported for keeping backward compatibility. But this requires CMake 3.11 or later for users because we can't use add_library(IMPORTED) is available since CMake 3.11. (Plasma::plasma-store-server target is also added for plasma-store-server executable.) FYI: We can resolve this problem by using COMPONENTS feature of find_package(). For example, find_package(Arrow COMPONENTS Parquet) is used instead of find_package(Parquet). With COMPONENTS, ${PACKAGE} is always "Arrow". So we can still install our .cmake files to ${PREFIX}/arrow/. But this approach breaks backward compatibility. So I choose ${PREFIX}/${PACKAGE}/*.cmake approach.
… isn't provided This is a follow-up of ARROW-12175 / apache#13892 . We introduced `${PACKAGE}::` namespace to all exported CMake targets such as `Arrow::arrow_shared` and `Arrow::arrow_static` but we also provides no namespaced CMake targets such as `arrow_share` and `arrow_static` as aliases of namespaced CMake targets. But the backward compatibility feature isn't worked for `_shared`.
Benchmark runs are scheduled for baseline = b43c6f6 and contender = 93b63e8. 93b63e8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
['Python', 'R'] benchmarks have high level of regressions. |
… isn't provided (#14003) This is a follow-up of ARROW-12175 / #13892 . We introduced `${PACKAGE}::` namespace to all exported CMake targets such as `Arrow::arrow_shared` and `Arrow::arrow_static`, but we also provided no-namespaced CMake targets such as `arrow_shared` and `arrow_static` as aliases of namespaced CMake targets. However, the logic to provide `arrow_shared` was buggy. Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
ARROW-9171 and ARROW-17231 are also fixed. Our CMake packages are broken. For example, `find_package(Parquet)` doesn't work without specifying `CMAKE_MODULE_PATH`. `find_package(${PACKAGE})` searches `${PREFIX}/${PACKAGE}/${PACKAGE}Config.cmake` or `${PREFIX}/${PACKAGE}/Find${PACKAGE}.cmake`. But our .cmake files are always installed `${PREFIX}/arrow/`. So `find_package(Parquet)` can't find `${PREFIX}/arrow/FindParquet.cmake` because "`/arrow/`" isn't "`/${PACKAGE}`". This change fixes this by installing `${PACKAGE}Config.cmake` to `${PREFIX}/${PACKAGE}/` instead of `${PREFIX}/arrow/`. This also removes all `Find${PACKAGE}.cmake`. We only provides `${PACKAGE}Config.cmake`. Our `Find${PACKAGE}.cmake` can find `${PACKAGE}` by CMake, pkg-config or manual .so/.h search. But we don't need to support pkg-config nor manual .so/.h search. We can use `${PACKAGE}Config.cmake` to support CMake package search. So this removes all `Find${PACKAGE}.cmake`. This also introduces namespace to our CMake targets. For example, `arrow_shared` is exported as `Arrow::arrow_shared` and `parquet_static` is exported as `Parquet::parquet_static`. But no namespace targets such as `arrow_shared` and `parquet_static` are still also exported for keeping backward compatibility. But this requires CMake 3.18 or later for users because `add_library(ALIAS)` for non-global `IMPORTED` library is available since CMake 3.18. (`Plasma::plasma-store-server` target is also added for `plasma-store-server` executable.) FYI: We can resolve this problem by using `COMPONENTS` feature of `find_package()`. For example, `find_package(Arrow COMPONENTS Parquet)` is used instead of `find_package(Parquet)`. With `COMPONENTS`, `${PACKAGE}` is always "Arrow". So we can still install our .cmake files to `${PREFIX}/arrow/`. But this approach breaks backward compatibility. So I choose `${PREFIX}/${PACKAGE}/*.cmake` approach. Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
… isn't provided (apache#14003) This is a follow-up of ARROW-12175 / apache#13892 . We introduced `${PACKAGE}::` namespace to all exported CMake targets such as `Arrow::arrow_shared` and `Arrow::arrow_static`, but we also provided no-namespaced CMake targets such as `arrow_shared` and `arrow_static` as aliases of namespaced CMake targets. However, the logic to provide `arrow_shared` was buggy. Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
message(STATUS "Arrow version: ${ARROW_VERSION} (${ARROW_FIND_APPROACH})") | ||
message(STATUS "Arrow SO and ABI version: ${ARROW_SO_VERSION}") | ||
message(STATUS "Arrow full SO version: ${ARROW_FULL_SO_VERSION}") | ||
message(STATUS "Found the Arrow core shared library: ${ARROW_SHARED_LIB}") | ||
message(STATUS "Found the Arrow core import library: ${ARROW_IMPORT_LIB}") | ||
message(STATUS "Found the Arrow core static library: ${ARROW_STATIC_LIB}") |
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 I understand correctly, this file is now replaced by cmake's find_package(Arrow)
now using the ArrowConfig.cmake
file that gets installed in the cmake directory (when installing Arrow C++).
But so before, the above printed some details about Arrow being found (and which version, the path, ..). Is there a way to let find_package(Arrow)
still print this? (would adding that to ArrowConfig.cmake.in
do this?)
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.
Yes. We need to add message(STATUS ...)
to ArrowConfig.cmake.in
for it. cmake --debug-print
prints more details but it will not show these information.
But *Config.cmake
doesn't use message(STATUS ...)
in general. If we want to add message(STATUS ...)
, it's better that we check Arrow_FIND_QUIETLY
like the following:
if(NOT Arrow_FIND_QUIETLY)
message(STATUS ...)
endif()
Arrow_FIND_QUIETLY
is TRUE
only when an user specify QUIET
explicitly like find_package(Arrow QUIET)
.
See also; https://cmake.org/cmake/help/latest/command/find_package.html#package-file-interface-variables
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.
I've created https://issues.apache.org/jira/browse/ARROW-17632 to follow this up, feel free to update the ticket if I missed anything
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.
@@ -59,7 +61,6 @@ endif() | |||
# | |||
|
|||
find_package(Arrow REQUIRED) | |||
include(ArrowOptions) |
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.
For my own understanding, the reason this line is no longer needed, is because this will also be included by find_package(Arrow)
already? (since ArrowConfig.cmake
has a line include("${CMAKE_CURRENT_LIST_DIR}/ArrowOptions.cmake")
)
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.
Right.
…nd during build (#14059) This PR aims to add back `message(STATUS ...)` statements that printed some details about Arrow being found, its version, and the paths. These were refactored away as part of #13892. Authored-by: Dhruv Vats <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…ckage (#14097) This is a follow up to update our build documentation from the changes on #13892 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
ARROW-9171 and ARROW-17231 are also fixed. Our CMake packages are broken. For example, `find_package(Parquet)` doesn't work without specifying `CMAKE_MODULE_PATH`. `find_package(${PACKAGE})` searches `${PREFIX}/${PACKAGE}/${PACKAGE}Config.cmake` or `${PREFIX}/${PACKAGE}/Find${PACKAGE}.cmake`. But our .cmake files are always installed `${PREFIX}/arrow/`. So `find_package(Parquet)` can't find `${PREFIX}/arrow/FindParquet.cmake` because "`/arrow/`" isn't "`/${PACKAGE}`". This change fixes this by installing `${PACKAGE}Config.cmake` to `${PREFIX}/${PACKAGE}/` instead of `${PREFIX}/arrow/`. This also removes all `Find${PACKAGE}.cmake`. We only provides `${PACKAGE}Config.cmake`. Our `Find${PACKAGE}.cmake` can find `${PACKAGE}` by CMake, pkg-config or manual .so/.h search. But we don't need to support pkg-config nor manual .so/.h search. We can use `${PACKAGE}Config.cmake` to support CMake package search. So this removes all `Find${PACKAGE}.cmake`. This also introduces namespace to our CMake targets. For example, `arrow_shared` is exported as `Arrow::arrow_shared` and `parquet_static` is exported as `Parquet::parquet_static`. But no namespace targets such as `arrow_shared` and `parquet_static` are still also exported for keeping backward compatibility. But this requires CMake 3.18 or later for users because `add_library(ALIAS)` for non-global `IMPORTED` library is available since CMake 3.18. (`Plasma::plasma-store-server` target is also added for `plasma-store-server` executable.) FYI: We can resolve this problem by using `COMPONENTS` feature of `find_package()`. For example, `find_package(Arrow COMPONENTS Parquet)` is used instead of `find_package(Parquet)`. With `COMPONENTS`, `${PACKAGE}` is always "Arrow". So we can still install our .cmake files to `${PREFIX}/arrow/`. But this approach breaks backward compatibility. So I choose `${PREFIX}/${PACKAGE}/*.cmake` approach. Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
… isn't provided (apache#14003) This is a follow-up of ARROW-12175 / apache#13892 . We introduced `${PACKAGE}::` namespace to all exported CMake targets such as `Arrow::arrow_shared` and `Arrow::arrow_static`, but we also provided no-namespaced CMake targets such as `arrow_shared` and `arrow_static` as aliases of namespaced CMake targets. However, the logic to provide `arrow_shared` was buggy. Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…nd during build (apache#14059) This PR aims to add back `message(STATUS ...)` statements that printed some details about Arrow being found, its version, and the paths. These were refactored away as part of apache#13892. Authored-by: Dhruv Vats <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…ckage (apache#14097) This is a follow up to update our build documentation from the changes on apache#13892 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
We need clang 11 or later for Gandiva. https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=37098&view=logs&j=4c86bc1b-1091-5192-4404-c74dfaad23e7&t=41795ef0-6501-5db4-3ad4-33c0cf085626&l=1001 [6/94] Generating decimal_ops.bc FAILED: src/gandiva/precompiled/decimal_ops.bc D:/bld/arrow-cpp-ext_1665469353844/work/cpp/build/src/gandiva/precompiled/decimal_ops.bc cmd.exe /C "cd /D D:\bld\arrow-cpp-ext_1665469353844\work\cpp\build\src\gandiva\precompiled && D:\bld\arrow-cpp-ext_1665469353844\_h_env\Library\bin\clang.exe -std=c++17 -fms-compatibility -fms-compatibility-version=19.20 -DGANDIVA_IR -DNDEBUG -DARROW_STATIC -DGANDIVA_STATIC -fno-use-cxa-atexit -emit-llvm -O3 -c D:/bld/arrow-cpp-ext_1665469353844/work/cpp/src/gandiva/precompiled/decimal_ops.cc -o D:/bld/arrow-cpp-ext_1665469353844/work/cpp/build/src/gandiva/precompiled/decimal_ops.bc -ID:/bld/arrow-cpp-ext_1665469353844/work/cpp/src -ID:/bld/arrow-cpp-ext_1665469353844/work/cpp/build/src -ID:/bld/arrow-cpp-ext_1665469353844/_h_env/Library/include" In file included from D:/bld/arrow-cpp-ext_1665469353844/work/cpp/src/gandiva/precompiled/decimal_ops.cc:20: In file included from D:/bld/arrow-cpp-ext_1665469353844/work/cpp/src\gandiva/precompiled/decimal_ops.h:20: In file included from C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\cstdint:9: C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\yvals_core.h:571:2: error: STL1000: Unexpected compiler version, expected Clang 11.0.0 or newer. #error STL1000: Unexpected compiler version, expected Clang 11.0.0 or newer. ^ This was added by me in apache#13892 but I can't remember why I pinned to 10...
…gs (#14376) We need clang 11 or later for Gandiva. https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=37098&view=logs&j=4c86bc1b-1091-5192-4404-c74dfaad23e7&t=41795ef0-6501-5db4-3ad4-33c0cf085626&l=1001 [6/94] Generating decimal_ops.bc FAILED: src/gandiva/precompiled/decimal_ops.bc D:/bld/arrow-cpp-ext_1665469353844/work/cpp/build/src/gandiva/precompiled/decimal_ops.bc cmd.exe /C "cd /D D:\bld\arrow-cpp-ext_1665469353844\work\cpp\build\src\gandiva\precompiled && D:\bld\arrow-cpp-ext_1665469353844\_h_env\Library\bin\clang.exe -std=c++17 -fms-compatibility -fms-compatibility-version=19.20 -DGANDIVA_IR -DNDEBUG -DARROW_STATIC -DGANDIVA_STATIC -fno-use-cxa-atexit -emit-llvm -O3 -c D:/bld/arrow-cpp-ext_1665469353844/work/cpp/src/gandiva/precompiled/decimal_ops.cc -o D:/bld/arrow-cpp-ext_1665469353844/work/cpp/build/src/gandiva/precompiled/decimal_ops.bc -ID:/bld/arrow-cpp-ext_1665469353844/work/cpp/src -ID:/bld/arrow-cpp-ext_1665469353844/work/cpp/build/src -ID:/bld/arrow-cpp-ext_1665469353844/_h_env/Library/include" In file included from D:/bld/arrow-cpp-ext_1665469353844/work/cpp/src/gandiva/precompiled/decimal_ops.cc:20: In file included from D:/bld/arrow-cpp-ext_1665469353844/work/cpp/src\gandiva/precompiled/decimal_ops.h:20: In file included from C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\cstdint:9: C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\yvals_core.h:571:2: error: STL1000: Unexpected compiler version, expected Clang 11.0.0 or newer. #error STL1000: Unexpected compiler version, expected Clang 11.0.0 or newer. ^ This was added by me in #13892 but I can't remember why I pinned to 10... Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
ARROW-9171 and ARROW-17231 are also fixed. Our CMake packages are broken. For example, `find_package(Parquet)` doesn't work without specifying `CMAKE_MODULE_PATH`. `find_package(${PACKAGE})` searches `${PREFIX}/${PACKAGE}/${PACKAGE}Config.cmake` or `${PREFIX}/${PACKAGE}/Find${PACKAGE}.cmake`. But our .cmake files are always installed `${PREFIX}/arrow/`. So `find_package(Parquet)` can't find `${PREFIX}/arrow/FindParquet.cmake` because "`/arrow/`" isn't "`/${PACKAGE}`". This change fixes this by installing `${PACKAGE}Config.cmake` to `${PREFIX}/${PACKAGE}/` instead of `${PREFIX}/arrow/`. This also removes all `Find${PACKAGE}.cmake`. We only provides `${PACKAGE}Config.cmake`. Our `Find${PACKAGE}.cmake` can find `${PACKAGE}` by CMake, pkg-config or manual .so/.h search. But we don't need to support pkg-config nor manual .so/.h search. We can use `${PACKAGE}Config.cmake` to support CMake package search. So this removes all `Find${PACKAGE}.cmake`. This also introduces namespace to our CMake targets. For example, `arrow_shared` is exported as `Arrow::arrow_shared` and `parquet_static` is exported as `Parquet::parquet_static`. But no namespace targets such as `arrow_shared` and `parquet_static` are still also exported for keeping backward compatibility. But this requires CMake 3.18 or later for users because `add_library(ALIAS)` for non-global `IMPORTED` library is available since CMake 3.18. (`Plasma::plasma-store-server` target is also added for `plasma-store-server` executable.) FYI: We can resolve this problem by using `COMPONENTS` feature of `find_package()`. For example, `find_package(Arrow COMPONENTS Parquet)` is used instead of `find_package(Parquet)`. With `COMPONENTS`, `${PACKAGE}` is always "Arrow". So we can still install our .cmake files to `${PREFIX}/arrow/`. But this approach breaks backward compatibility. So I choose `${PREFIX}/${PACKAGE}/*.cmake` approach. Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
… isn't provided (apache#14003) This is a follow-up of ARROW-12175 / apache#13892 . We introduced `${PACKAGE}::` namespace to all exported CMake targets such as `Arrow::arrow_shared` and `Arrow::arrow_static`, but we also provided no-namespaced CMake targets such as `arrow_shared` and `arrow_static` as aliases of namespaced CMake targets. However, the logic to provide `arrow_shared` was buggy. Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…nd during build (apache#14059) This PR aims to add back `message(STATUS ...)` statements that printed some details about Arrow being found, its version, and the paths. These were refactored away as part of apache#13892. Authored-by: Dhruv Vats <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…ckage (apache#14097) This is a follow up to update our build documentation from the changes on apache#13892 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
ARROW-9171 and ARROW-17231 are also fixed.
Our CMake packages are broken. For example,
find_package(Parquet)
doesn't work without specifying
CMAKE_MODULE_PATH
.find_package(${PACKAGE})
searches${PREFIX}/${PACKAGE}/${PACKAGE}Config.cmake
or${PREFIX}/${PACKAGE}/Find${PACKAGE}.cmake
. But our .cmake files arealways installed
${PREFIX}/arrow/
. Sofind_package(Parquet)
can't find${PREFIX}/arrow/FindParquet.cmake
because "/arrow/
" isn't"
/${PACKAGE}
".This change fixes this by installing
${PACKAGE}Config.cmake
to${PREFIX}/${PACKAGE}/
instead of${PREFIX}/arrow/
.This also removes all
Find${PACKAGE}.cmake
. We only provides${PACKAGE}Config.cmake
. OurFind${PACKAGE}.cmake
can find${PACKAGE}
by CMake, pkg-config or manual .so/.h search. But we don't need to
support pkg-config nor manual .so/.h search. We can use
${PACKAGE}Config.cmake
to support CMake package search. So thisremoves all
Find${PACKAGE}.cmake
.This also introduces namespace to our CMake targets. For example,
arrow_shared
is exported asArrow::arrow_shared
andparquet_static
is exported asParquet::parquet_static
. But nonamespace targets such as
arrow_shared
andparquet_static
arestill also exported for keeping backward compatibility. But this
requires CMake 3.18 or later for users because
add_library(ALIAS)
for non-global
IMPORTED
library is available since CMake 3.18.(
Plasma::plasma-store-server
target is also added forplasma-store-server
executable.)FYI: We can resolve this problem by using
COMPONENTS
feature offind_package()
. For example,find_package(Arrow COMPONENTS Parquet)
is used instead offind_package(Parquet)
. WithCOMPONENTS
,${PACKAGE}
is always "Arrow". So we can still installour .cmake files to
${PREFIX}/arrow/
. But this approach breaksbackward compatibility. So I choose
${PREFIX}/${PACKAGE}/*.cmake
approach.