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-17615: [C++][Packaging] Remove check_required_components from configure_package_config_file #14045

Closed
wants to merge 1 commit into from

Conversation

raulcd
Copy link
Member

@raulcd raulcd commented Sep 5, 2022

The problem is only reproducible when using REQUIRED components on the find_package call:

find_package(Arrow REQUIRED COMPONENTS dataset flight parquet) 

due to the following macro automatically generated from configure_package_config_file

macro(check_required_components _NAME)
  foreach(comp ${${_NAME}_FIND_COMPONENTS})
    if(NOT ${_NAME}_${comp}_FOUND)
      if(${_NAME}_FIND_REQUIRED_${comp})
        set(${_NAME}_FOUND FALSE)
      endif()
    endif()
  endforeach()
endmacro()

A possible solution is to add the NO_CHECK_REQUIRED_COMPONENTS_MACRO

@@ -155,7 +155,8 @@ function(arrow_install_cmake_package PACKAGE_NAME EXPORT_NAME)
set(CONFIG_CMAKE "${PACKAGE_NAME}Config.cmake")
set(BUILT_CONFIG_CMAKE "${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_CMAKE}")
configure_package_config_file("${CONFIG_CMAKE}.in" "${BUILT_CONFIG_CMAKE}"
INSTALL_DESTINATION "${ARROW_CMAKE_DIR}/${PACKAGE_NAME}")
INSTALL_DESTINATION "${ARROW_CMAKE_DIR}/${PACKAGE_NAME}"
NO_CHECK_REQUIRED_COMPONENTS_MACRO)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kou I am not sure if the solution to this issue should be to not add the required components macro or fix CMake to force finding the required components before. Let me know what do you think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem the Appveyor failure is related:

-- Detecting CXX compile features - done
CMake Error at C:/Miniconda38-x64/envs/arrow/Library/lib/cmake/Arrow/ArrowConfig.cmake:178 (check_required_components):
  Unknown CMake command "check_required_components".
Call Stack (most recent call first):
  CMakeLists.txt:63 (find_package)

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

⚠️ Ticket has no components in JIRA, make sure you assign one.

@@ -51,7 +51,7 @@ file into an executable linked with the Arrow C++ shared library:
find_package(Arrow REQUIRED)

add_executable(my_example my_example.cc)
target_link_libraries(my_example PRIVATE arrow_shared)
target_link_libraries(my_example PRIVATE Arrow::arrow_shared)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We recommend Arrow::arrow_shared. So this change is preferred.
FYI: Users can still use arrow_shared with CMake 3.18 or later for backward compatibility.
See also: https://github.com/apache/arrow/blob/master/cpp/src/arrow/ArrowConfig.cmake.in#L110-L121

BTW, we have a Jira issue to update this document: ARROW-17575
If you take over this, I'm very happy!

Here are some notes for the new our CMake packages:

  • Users can use find_package(Arrow), find_package(Parquet) and so on as usual but can't use COMPONENTS like find_package(Arrow COMPONENTS parquet)
    • The old our CMake packages just ignore COMPONENTS.
  • Arrow::arrow_shared is recommended than arrow_shared
  • CMAKE_PREFIX_PATH not PKG_CONFIG_PATH should be used to find Apache Arrow C++ that isn't installed to the system location such as /usr
    • The old our CMake packages also uses PKG_CONFIG_PATH (.pc can be used to find the Arrow CMake package)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kou I've assigned the ticket to me, it would help me learn some of these changes! I am closing this PR as it will be covered as documentation changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!!
If you have a trouble, please ask me!

@raulcd raulcd closed this Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants