You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@orc.apache.org by "wgtmac (via GitHub)" <gi...@apache.org> on 2024/04/15 10:04:36 UTC

[PR] ORC-1689: [C++] Generate CMake config file [orc]

wgtmac opened a new pull request, #1889:
URL: https://github.com/apache/orc/pull/1889

   ### What changes were proposed in this pull request?
   Generate cmake config file and install it.
   
   ### Why are the changes needed?
   It is good for downstream projects to find ORC library.
   
   ### How was this patch tested?
   Test it locally.
   
   ### Was this patch authored or co-authored using generative AI tooling?
   No.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1569856376


##########
c++/src/CMakeLists.txt:
##########
@@ -202,18 +202,29 @@ endif(BUILD_ENABLE_AVX512)
 add_library (orc STATIC ${SOURCE_FILES})
 
 target_link_libraries (orc
-  orc::protobuf
-  orc::zlib
-  orc::snappy
-  orc::lz4
-  orc::zstd
-  ${LIBHDFSPP_LIBRARIES}
+  INTERFACE
+    $<INSTALL_INTERFACE:protobuf::libprotobuf>
+    $<INSTALL_INTERFACE:ZLIB::ZLIB>
+    $<INSTALL_INTERFACE:Snappy::snappy>
+    $<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:LZ4::lz4_static>>

Review Comment:
   Adding `TARGET_NAME_IF_EXISTS` generator expression looks like a bad idea as it does not complain a missing call to find_package any more.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2068945756

   cc @dongjoon-hyun @stiga-huang 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1569814941


##########
c++/src/CMakeLists.txt:
##########
@@ -202,18 +202,29 @@ endif(BUILD_ENABLE_AVX512)
 add_library (orc STATIC ${SOURCE_FILES})
 
 target_link_libraries (orc
-  orc::protobuf
-  orc::zlib
-  orc::snappy
-  orc::lz4
-  orc::zstd
-  ${LIBHDFSPP_LIBRARIES}
+  INTERFACE
+    $<INSTALL_INTERFACE:protobuf::libprotobuf>

Review Comment:
   It seems that downstream clients are required to add
   ```
   find_package(Protobuf REQUIRED)
   find_package(ZLIB REQUIRED)
   find_package(Snappy REQUIRED)
   ```
   before the line
   ```
   find_package(orc REQUIRED CONFIG)
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2080756803

   I have just merged it. Thanks all!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2068729308

   @h-vetinari Could you help check if this breaks conda? I saw there are some patches https://github.com/conda-forge/orc-feedstock/tree/main/recipe/patches. Is it possible to add `ORC_PACKAGE_KIND=conda` with some code change to remove the patches?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2071079098

   > @h-vetinari @xhochy Could you help check if this breaks conda?
   
   Thanks for the ping - happy to provide feedback and to try things out!
   
   CC also @nehaljwani, who has been taking care of orc in conda-forge for much longer than I have.
   
   > I saw there are some patches https://github.com/conda-forge/orc-feedstock/tree/main/recipe/patches. Is it possible to add `ORC_PACKAGE_KIND=conda` with some code change to remove the patches?
   
   I just went through the patches in the feedstock (after rebasing them on current `main` here):
   * [`0001-Deactivate-aggressive-failures-on-warnings.patch`](https://github.com/conda-forge/orc-feedstock/blob/8586762a73116fec8053ac0682f9a26e47c62955/recipe/patches/0001-Deactivate-aggressive-failures-on-warnings.patch) - we don't want `-Wall -Werror` by default, but this patch comes a time before https://github.com/apache/orc/commit/b7fd75ce616cb0a6368fb48b61eccb6685254e64 existed, so we can probably just use that option
   * [`0002-Don-t-force-orc-to-be-a-static-library-let-end-user-.patch`](https://github.com/conda-forge/orc-feedstock/blob/8586762a73116fec8053ac0682f9a26e47c62955/recipe/patches/0002-Don-t-force-orc-to-be-a-static-library-let-end-user-.patch) - this is really important to us (we need shared libs). ORC should not hardcode that the library must be static, but respect `-DBUILD_SHARED_LIBS=ON`
   * [`0003-CMake-Add-more-hints-for-libraries-on-Windows.patch`](https://github.com/conda-forge/orc-feedstock/blob/8586762a73116fec8053ac0682f9a26e47c62955/recipe/patches/0003-CMake-Add-more-hints-for-libraries-on-Windows.patch) - this could very likely be absorbed into this PR as follows:
     ```diff
     diff --git a/cmake_modules/FindLZ4.cmake b/cmake_modules/FindLZ4.cmake
     index b1557f496..8061dda38 100644
     --- a/cmake_modules/FindLZ4.cmake
     +++ b/cmake_modules/FindLZ4.cmake
     @@ -33,7 +33,7 @@ find_path (LZ4_INCLUDE_DIR lz4.h HINTS
        NO_DEFAULT_PATH
        PATH_SUFFIXES "include")
     
     -find_library (LZ4_LIBRARY NAMES lz4 HINTS
     +find_library (LZ4_LIBRARY NAMES lz4 liblz4 HINTS
        ${_lz4_path}
        PATH_SUFFIXES "lib" "lib64")
     ```
   * [`0004-don-t-force-lib-destination.patch`](https://github.com/conda-forge/orc-feedstock/blob/8586762a73116fec8053ac0682f9a26e47c62955/recipe/patches/0004-don-t-force-lib-destination.patch) - could be adopted here
     ```diff
     diff --git a/c++/src/CMakeLists.txt b/c++/src/CMakeLists.txt
     index 3612c1c8f..3ebabe10d 100644
     --- a/c++/src/CMakeLists.txt
     +++ b/c++/src/CMakeLists.txt
     @@ -234,4 +234,4 @@ endif ()
     
      add_dependencies(orc orc-format_ep)
     
     -install(TARGETS orc DESTINATION lib)
     +install(TARGETS orc DESTINATION "${CMAKE_INSTALL_LIBDIR}")
     ``` 
   * [`0005-Check-for-protobuf-config-based-module.patch`](https://github.com/conda-forge/orc-feedstock/blob/8586762a73116fec8053ac0682f9a26e47c62955/recipe/patches/0005-Check-for-protobuf-config-based-module.patch) - already done in #1529
   
   So aside from patch Nr. 2 (if ORC insists to hardcode the `STATIC` bit), it should be possible to avoid the need for `ORC_PACKAGE_KIND=conda` in the first place, AFAICT.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1574421813


##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -132,6 +154,9 @@ else ()
     BUILD_BYPRODUCTS "${SNAPPY_STATIC_LIB}")
 
   add_built_library (snappy_ep orc_snappy ${SNAPPY_STATIC_LIB} ${SNAPPY_INCLUDE_DIR})
+
+  list (APPEND ORC_VENDOR_DEPENDENCIES "vendored_snappy:${SNAPPY_STATIC_LIB_NAME}")
+  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:vendored_snappy>")

Review Comment:
   How about putting this target to `orc::` namespace for safety?
   
   ```suggestion
     list (APPEND ORC_VENDOR_DEPENDENCIES "orc::vendored_snappy:${SNAPPY_STATIC_LIB_NAME}")
     list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:orc::vendored_snappy>")
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1581837056


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# This config sets the following variables in your project::
+#
+#   orc_VERSION - version of the found ORC
+#   orc_FOUND - true if ORC found on the system
+#
+# This config sets the following targets in your project::

Review Comment:
   ```suggestion
   # This config sets the following targets in your project:
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1581837034


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# This config sets the following variables in your project::

Review Comment:
   ```suggestion
   # This config sets the following variables in your project:
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2062899040

   Yes! But we need to find dependencies such as `protobuf::libprotobuf` in `orcConfig.cmake` something like:
   
   ```diff
   diff --git a/c++/orcConfig.cmake.in b/c++/orcConfig.cmake.in
   index 561ea3d7..a6b16f8c 100644
   --- a/c++/orcConfig.cmake.in
   +++ b/c++/orcConfig.cmake.in
   @@ -17,6 +17,13 @@
    
    @PACKAGE_INIT@
    
   +set(ORC_SYSTEM_DEPENDENCIES "@ORC_SYSTEM_DEPENDENCIES@")
   +
   +include(CMakeFindDependencyMacro)
   +foreach(dependency ${ORC_SYSTEM_DEPENDENCIES})
   +  find_dependency(${dependency})
   +endforeach()
   +
    include("${CMAKE_CURRENT_LIST_DIR}/orcTargets.cmake")
    
    check_required_components(orc)
   diff --git a/c++/src/CMakeLists.txt b/c++/src/CMakeLists.txt
   index 958da1af..05cd34c1 100644
   --- a/c++/src/CMakeLists.txt
   +++ b/c++/src/CMakeLists.txt
   @@ -203,13 +203,7 @@ add_library (orc STATIC ${SOURCE_FILES})
    
    target_link_libraries (orc
      INTERFACE
   -    $<INSTALL_INTERFACE:protobuf::libprotobuf>
   -    $<INSTALL_INTERFACE:ZLIB::ZLIB>
   -    $<INSTALL_INTERFACE:Snappy::snappy>
   -    $<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:LZ4::lz4_static>>
   -    $<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:LZ4::lz4_shared>>
   -    $<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:zstd::libzstd_static>>
   -    $<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:zstd::libzstd_shared>>
   +    ${ORC_INSTALL_INTERFACE_TARGETS}
      PUBLIC
        $<BUILD_INTERFACE:orc::protobuf>
        $<BUILD_INTERFACE:orc::zlib>
   diff --git a/cmake_modules/ThirdpartyToolchain.cmake b/cmake_modules/ThirdpartyToolchain.cmake
   index c3d414e8..3b65db36 100644
   --- a/cmake_modules/ThirdpartyToolchain.cmake
   +++ b/cmake_modules/ThirdpartyToolchain.cmake
   @@ -15,6 +15,9 @@
    # specific language governing permissions and limitations
    # under the License.
    
   +set(ORC_SYSTEM_DEPENDENCIES)
   +set(ORC_INSTALL_INTERFACE_TARGETS)
   +
    set(ORC_FORMAT_VERSION "1.0.0")
    set(LZ4_VERSION "1.9.3")
    set(SNAPPY_VERSION "1.1.7")
   @@ -106,9 +109,13 @@ ExternalProject_Add (orc-format_ep
    # Snappy
    if (ORC_PACKAGE_KIND STREQUAL "conan")
      find_package (Snappy REQUIRED CONFIG)
   +  list (APPEND ORC_SYSTEM_DEPENDENCIES Snappy)
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:Snappy::snappy>")
      add_resolved_library (orc_snappy ${Snappy_LIBRARIES} ${Snappy_INCLUDE_DIR})
    elseif (NOT "${SNAPPY_HOME}" STREQUAL "")
      find_package (Snappy REQUIRED)
   +  list (APPEND ORC_SYSTEM_DEPENDENCIES Snappy)
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:Snappy::snappy>")
      if (ORC_PREFER_STATIC_SNAPPY AND ${SNAPPY_STATIC_LIB})
        add_resolved_library (orc_snappy ${SNAPPY_STATIC_LIB} ${SNAPPY_INCLUDE_DIR})
      else ()
   @@ -141,9 +148,13 @@ add_library (orc::snappy ALIAS orc_snappy)
    
    if (ORC_PACKAGE_KIND STREQUAL "conan")
      find_package (ZLIB REQUIRED CONFIG)
   +  list (APPEND ORC_SYSTEM_DEPENDENCIES ZLIB)
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:ZLIB::ZLIB>")
      add_resolved_library (orc_zlib ${ZLIB_LIBRARIES} ${ZLIB_INCLUDE_DIR})
    elseif (NOT "${ZLIB_HOME}" STREQUAL "")
      find_package (ZLIB REQUIRED)
   +  list (APPEND ORC_SYSTEM_DEPENDENCIES ZLIB)
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:ZLIB::ZLIB>")
      if (ORC_PREFER_STATIC_ZLIB AND ${ZLIB_STATIC_LIB})
        add_resolved_library (orc_zlib ${ZLIB_STATIC_LIB} ${ZLIB_INCLUDE_DIR})
      else ()
   @@ -184,9 +195,17 @@ add_library (orc::zlib ALIAS orc_zlib)
    
    if (ORC_PACKAGE_KIND STREQUAL "conan")
      find_package (ZSTD REQUIRED CONFIG)
   +  list (APPEND ORC_SYSTEM_DEPENDENCIES ZSTD)
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:zstd::libzstd_static>>")
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:zstd::libzstd_shared>>")
      add_resolved_library (orc_zstd ${zstd_LIBRARIES} ${zstd_INCLUDE_DIR})
    elseif (NOT "${ZSTD_HOME}" STREQUAL "")
      find_package (ZSTD REQUIRED)
   +  # Zstandard uses "zstd" as its CMake package name. We don't use our
   +  # FindZSTD.cmake for find_package(orc).
   +  list (APPEND ORC_SYSTEM_DEPENDENCIES zstd)
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:zstd::libzstd_static>>")
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:zstd::libzstd_shared>>")
      if (ORC_PREFER_STATIC_ZSTD AND ${ZSTD_STATIC_LIB})
        add_resolved_library (orc_zstd ${ZSTD_STATIC_LIB} ${ZSTD_INCLUDE_DIR})
      else ()
   @@ -233,9 +252,17 @@ add_library (orc::zstd ALIAS orc_zstd)
    # LZ4
    if (ORC_PACKAGE_KIND STREQUAL "conan")
      find_package (LZ4 REQUIRED CONFIG)
   +  list (APPEND ORC_SYSTEM_DEPENDENCIES LZ4)
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:LZ4::lz4_static>>")
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:LZ4::lz4_shared>>")
      add_resolved_library (orc_lz4 ${lz4_LIBRARIES} ${lz4_INCLUDE_DIR})
    elseif (NOT "${LZ4_HOME}" STREQUAL "")
      find_package (LZ4 REQUIRED)
   +  # LZ4 uses "lz4" as its CMake package name. We don't use our
   +  # FindLZ4.cmake for find_package(orc).
   +  list (APPEND ORC_SYSTEM_DEPENDENCIES lz4)
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:LZ4::lz4_static>>")
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:$<TARGET_NAME_IF_EXISTS:LZ4::lz4_shared>>")
      if (ORC_PREFER_STATIC_LZ4 AND ${LZ4_STATIC_LIB})
        add_resolved_library (orc_lz4 ${LZ4_STATIC_LIB} ${LZ4_INCLUDE_DIR})
      else ()
   @@ -379,9 +406,13 @@ endif ()
    
    if (ORC_PACKAGE_KIND STREQUAL "conan")
      find_package (Protobuf REQUIRED CONFIG)
   +  list (APPEND ORC_SYSTEM_DEPENDENCIES Protobuf)
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:protobuf::libprotobuf>")
      add_resolved_library (orc_protobuf ${protobuf_LIBRARIES} ${protobuf_INCLUDE_DIR})
    elseif (NOT "${PROTOBUF_HOME}" STREQUAL "")
      find_package (Protobuf REQUIRED)
   +  list (APPEND ORC_SYSTEM_DEPENDENCIES Protobuf)
   +  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:protobuf::libprotobuf>")
    
      if (ORC_PREFER_STATIC_PROTOBUF AND ${PROTOBUF_STATIC_LIB})
        add_resolved_library (orc_protobuf ${PROTOBUF_STATIC_LIB} ${PROTOBUF_INCLUDE_DIR})
   ```
   
   Note that the above diff doesn't work for dependencies that are installed in custom location such as `$HOME/local` because we use `cmake_modules/FindXXX.cmake` on build but we don't use it on `find_package(orc)`. They may find different dependencies.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1576016456


##########
c++/CMakeLists.txt:
##########
@@ -21,3 +21,17 @@ add_subdirectory(src)
 if (BUILD_CPP_TESTS)
   add_subdirectory(test)
 endif ()
+
+# Generate cmake package configuration files
+include(CMakePackageConfigHelpers)
+configure_package_config_file(
+  orcConfig.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/orcConfig.cmake
+  INSTALL_DESTINATION ${ORC_INSTALL_CMAKE_DIR})
+write_basic_package_version_file(
+  ${CMAKE_CURRENT_BINARY_DIR}/orcConfigVersion.cmake
+  VERSION ${ORC_VERSION}
+  COMPATIBILITY SameMajorVersion)
+install(FILES
+  ${CMAKE_CURRENT_BINARY_DIR}/orcConfig.cmake

Review Comment:
   Just curious, why do we install **orcConfig.cmake** twice?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2061617539

   Does the file /tmp/orc/lib/cmake/orc/orcTargets.cmake looks right? @kou 
   ```
   # Generated by CMake
   
   if("${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION}" LESS 2.8)
      message(FATAL_ERROR "CMake >= 2.8.0 required")
   endif()
   if(CMAKE_VERSION VERSION_LESS "2.8.3")
      message(FATAL_ERROR "CMake >= 2.8.3 required")
   endif()
   cmake_policy(PUSH)
   cmake_policy(VERSION 2.8.3...3.25)
   #----------------------------------------------------------------
   # Generated CMake target import file.
   #----------------------------------------------------------------
   
   # Commands may need to know the format version.
   set(CMAKE_IMPORT_FILE_VERSION 1)
   
   # Protect against multiple inclusion, which would fail when already imported targets are added once more.
   set(_cmake_targets_defined "")
   set(_cmake_targets_not_defined "")
   set(_cmake_expected_targets "")
   foreach(_cmake_expected_target IN ITEMS orc::orc)
     list(APPEND _cmake_expected_targets "${_cmake_expected_target}")
     if(TARGET "${_cmake_expected_target}")
       list(APPEND _cmake_targets_defined "${_cmake_expected_target}")
     else()
       list(APPEND _cmake_targets_not_defined "${_cmake_expected_target}")
     endif()
   endforeach()
   unset(_cmake_expected_target)
   if(_cmake_targets_defined STREQUAL _cmake_expected_targets)
     unset(_cmake_targets_defined)
     unset(_cmake_targets_not_defined)
     unset(_cmake_expected_targets)
     unset(CMAKE_IMPORT_FILE_VERSION)
     cmake_policy(POP)
     return()
   endif()
   if(NOT _cmake_targets_defined STREQUAL "")
     string(REPLACE ";" ", " _cmake_targets_defined_text "${_cmake_targets_defined}")
     string(REPLACE ";" ", " _cmake_targets_not_defined_text "${_cmake_targets_not_defined}")
     message(FATAL_ERROR "Some (but not all) targets in this export set were already defined.\nTargets Defined: ${_cmake_targets_defined_text}\nTargets not yet defined: ${_cmake_targets_not_defined_text}\n")
   endif()
   unset(_cmake_targets_defined)
   unset(_cmake_targets_not_defined)
   unset(_cmake_expected_targets)
   
   
   # Compute the installation prefix relative to this file.
   get_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
   get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
   get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
   get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
   if(_IMPORT_PREFIX STREQUAL "/")
     set(_IMPORT_PREFIX "")
   endif()
   
   # Create imported target orc::orc
   add_library(orc::orc STATIC IMPORTED)
   
   set_target_properties(orc::orc PROPERTIES
     INTERFACE_COMPILE_DEFINITIONS "ENABLE_METRICS=0"
     INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
     INTERFACE_LINK_LIBRARIES "protobuf::libprotobuf;ZLIB::ZLIB;Snappy::snappy;\$<TARGET_NAME_IF_EXISTS:LZ4::lz4_static>;\$<TARGET_NAME_IF_EXISTS:LZ4::lz4_shared>;\$<TARGET_NAME_IF_EXISTS:zstd::libzstd_static>;\$<TARGET_NAME_IF_EXISTS:zstd::libzstd_shared>"
   )
   
   if(CMAKE_VERSION VERSION_LESS 2.8.12)
     message(FATAL_ERROR "This file relies on consumers using CMake 2.8.12 or greater.")
   endif()
   
   # Load information for each installed configuration.
   file(GLOB _cmake_config_files "${CMAKE_CURRENT_LIST_DIR}/orcTargets-*.cmake")
   foreach(_cmake_config_file IN LISTS _cmake_config_files)
     include("${_cmake_config_file}")
   endforeach()
   unset(_cmake_config_file)
   unset(_cmake_config_files)
   
   # Cleanup temporary variables.
   set(_IMPORT_PREFIX)
   
   # Loop over all imported files and verify that they actually exist
   foreach(_cmake_target IN LISTS _cmake_import_check_targets)
     foreach(_cmake_file IN LISTS "_cmake_import_check_files_for_${_cmake_target}")
       if(NOT EXISTS "${_cmake_file}")
         message(FATAL_ERROR "The imported target \"${_cmake_target}\" references the file
      \"${_cmake_file}\"
   but this file does not exist.  Possible reasons include:
   * The file was deleted, renamed, or moved to another location.
   * An install or uninstall procedure did not complete successfully.
   * The installation package was faulty and contained
      \"${CMAKE_CURRENT_LIST_FILE}\"
   but not all the files it references.
   ")
       endif()
     endforeach()
     unset(_cmake_file)
     unset("_cmake_import_check_files_for_${_cmake_target}")
   endforeach()
   unset(_cmake_target)
   unset(_cmake_import_check_targets)
   
   # This file does not depend on other imported targets which have
   # been exported from the same project but in a separate export set.
   
   # Commands beyond this point should not need to know the version.
   set(CMAKE_IMPORT_FILE_VERSION)
   cmake_policy(POP)
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1576906529


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# This config sets the following variables in your project::
+#
+#   orc_VERSION - version of the found ORC
+#   orc_FOUND - true if ORC found on the system
+#
+# This config sets the following targets in your project::
+#
+#   orc::orc - for linked as static library
+#
+# For backward compatibility, this config also sets the following variables:
+#
+#   ORC_STATIC_LIB - static library of the found ORC
+#   ORC_INCLUDE_DIR - include directory of the found ORC
+#   ORC_INCLUDE_DIRs - same as ORC_INCLUDE_DIRS above

Review Comment:
   ```suggestion
   #   ORC_INCLUDE_DIRS - same as ORC_INCLUDE_DIR above
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2062900675

   Yes, I am already trying to add find_dependency to the configf file. Thanks for the detail help!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2058151723

   It makes sense.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "stiga-huang (via GitHub)" <gi...@apache.org>.
stiga-huang commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1581136738


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@PACKAGE_INIT@
+
+set(ORC_VENDOR_DEPENDENCIES "@ORC_VENDOR_DEPENDENCIES@")
+set(ORC_SYSTEM_DEPENDENCIES "@ORC_SYSTEM_DEPENDENCIES@")
+
+if(DEFINED CMAKE_MODULE_PATH)
+  set(ORC_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH})
+else()
+  unset(ORC_CMAKE_MODULE_PATH_OLD)
+endif()
+set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}")
+
+include(CMakeFindDependencyMacro)
+foreach(dependency ${ORC_SYSTEM_DEPENDENCIES})
+  find_dependency(${dependency})

Review Comment:
   Thanks for fixing these issues! Just verified, the build of Impala succeeds now.
   
   It seems ORC can still build without these fixes. Just curious, is it possible to add tests for this file, or it can only be tested by external systems?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac closed pull request #1889: ORC-1689: [C++] Generate CMake config file
URL: https://github.com/apache/orc/pull/1889


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1577092183


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@PACKAGE_INIT@
+
+set(ORC_VENDOR_DEPENDENCIES "@ORC_VENDOR_DEPENDENCIES@")
+set(ORC_SYSTEM_DEPENDENCIES "@ORC_SYSTEM_DEPENDENCIES@")
+
+if(DEFINED CMAKE_MODULE_PATH)
+  set(ORC_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH})
+else()
+  unset(ORC_CMAKE_MODULE_PATH_OLD)
+endif()
+set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}")
+
+include(CMakeFindDependencyMacro)
+foreach(dependency ${ORC_SYSTEM_DEPENDENCIES})
+  find_dependency(${dependency})

Review Comment:
   I guess you ran into this issue: https://github.com/apache/arrow/issues/41331#issuecomment-2071278787. In short, currently FindProtobuf.cmake will try to find config file first even you have set PROTOBUF_HOME. Let me fix this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "stiga-huang (via GitHub)" <gi...@apache.org>.
stiga-huang commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1576993049


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@PACKAGE_INIT@
+
+set(ORC_VENDOR_DEPENDENCIES "@ORC_VENDOR_DEPENDENCIES@")
+set(ORC_SYSTEM_DEPENDENCIES "@ORC_SYSTEM_DEPENDENCIES@")
+
+if(DEFINED CMAKE_MODULE_PATH)
+  set(ORC_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH})
+else()
+  unset(ORC_CMAKE_MODULE_PATH_OLD)
+endif()
+set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}")
+
+include(CMakeFindDependencyMacro)
+foreach(dependency ${ORC_SYSTEM_DEPENDENCIES})
+  find_dependency(${dependency})

Review Comment:
   Thanks for the pointer! I moved forward by adding variables like SNAPPY_HOME, ZLIB_HOME, PROTOBUF_HOME. But got a strange error on protobuf though the headers and libs are all found:
   ```
   -- SNAPPY_HOME: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/snappy-1.1.8/
   -- Found the Snappy header: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/snappy-1.1.8/include/snappy.h
   -- Found the Snappy library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/snappy-1.1.8/lib/libsnappy.so
   -- Found the Snappy static library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/snappy-1.1.8/lib/libsnappy.a
   -- ZLIB_HOME: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/zlib-1.2.13/
   -- Found the ZLIB header: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/cloudflarezlib-9e601a3f37/include/zlib.h
   -- Found the ZLIB library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/cloudflarezlib-9e601a3f37/lib/libz.so
   -- Found the ZLIB static library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/cloudflarezlib-9e601a3f37/lib/libz.a
   -- ZSTD_HOME: 
   -- Found the zstd header: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/zstd-1.5.2/include/zstd.h
   -- Found the zstd library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/zstd-1.5.2/lib/libzstd.so
   -- Found the zstd static library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/zstd-1.5.2/lib/libzstd.a
   -- LZ4_HOME: 
   -- Found the LZ4 header: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/lz4-1.9.3/include/lz4.h
   -- Found the LZ4 library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/lz4-1.9.3/lib/liblz4.so
   -- Found the LZ4 static library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/lz4-1.9.3/lib/liblz4.a
   -- PROTOBUF_HOME: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/protobuf-3.14.0/
   -- Could NOT find Protobuf (missing: Protobuf_DIR)
   -- Found the Protobuf headers: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/protobuf-3.14.0/include
   -- Found the Protobuf library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/protobuf-3.14.0/lib/libprotobuf.so
   -- Found the Protoc library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/protobuf-3.14.0/lib/libprotoc.so
   -- Found the Protoc executable: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/protobuf-3.14.0/bin/protoc
   -- Found the Protobuf static library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/protobuf-3.14.0/lib/libprotobuf.a
   -- Found the Protoc static library: /home/quanlong/workspace/Impala/toolchain/toolchain-packages-gcc10.4.0/protobuf-3.14.0/lib/libprotoc.a
   CMake Error at CMakeLists.txt:366 (find_package):
     Found package configuration file:
   
       /home/quanlong/workspace/orc/build/install/lib/cmake/orc/orcConfig.cmake
   
     but it set orc_FOUND to FALSE so package "orc" is considered to be NOT
     FOUND.  Reason given by package:
   
     orc could not be found because dependency Protobuf could not be found.
   ```
   
   Impala builds on pre-built toolchain binaries including `liborc.a` so it doesn't need ORC to find these libraries since no need to build ORC again. I modified `orcConfig.cmake` to set `ORC_SYSTEM_DEPENDENCIES` empty and was able to build Impala:
   ```
   -set(ORC_SYSTEM_DEPENDENCIES "Snappy;ZLIB;ZSTD;LZ4;Protobuf")
   +set(ORC_SYSTEM_DEPENDENCIES "")
   ```
   Note that the regular process to build Impala is
   
   - Build native-toolchain which builds ORC here: https://github.com/cloudera/native-toolchain/blob/78a3723ad8251fbd786c276bd0f6b75a6b5bae74/source/orc/build.sh#L44-L47
     The artifacts are uploaded to S3 and share with all developers
   - Build Impala based on the pre-built toolchain



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "stiga-huang (via GitHub)" <gi...@apache.org>.
stiga-huang commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1581597508


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# This config sets the following variables in your project::
+#
+#   orc_VERSION - version of the found ORC
+#   orc_FOUND - true if ORC found on the system
+#
+# This config sets the following targets in your project::

Review Comment:
   nit: also have duplicated colons here



##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@PACKAGE_INIT@
+
+set(ORC_VENDOR_DEPENDENCIES "@ORC_VENDOR_DEPENDENCIES@")
+set(ORC_SYSTEM_DEPENDENCIES "@ORC_SYSTEM_DEPENDENCIES@")
+
+if(DEFINED CMAKE_MODULE_PATH)
+  set(ORC_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH})
+else()
+  unset(ORC_CMAKE_MODULE_PATH_OLD)
+endif()
+set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}")
+
+include(CMakeFindDependencyMacro)
+foreach(dependency ${ORC_SYSTEM_DEPENDENCIES})
+  find_dependency(${dependency})

Review Comment:
   It's ok to add the tests in a separate PR.



##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# This config sets the following variables in your project::

Review Comment:
   nit: there are duplicated colons at the end "::"



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2062900752

   FYI: Apache Arrow bundles their custom `FindXXX.cmake` and use them if they are needed: https://github.com/apache/arrow/blob/73386cb806a429875f1846b69e21beccfd339b21/cpp/cmake_modules/ThirdpartyToolchain.cmake#L236-L250


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1566550570


##########
c++/CMakeLists.txt:
##########
@@ -15,14 +15,26 @@
 # specific language governing permissions and limitations
 # under the License.
 
-include_directories (
-  ${CMAKE_CURRENT_BINARY_DIR}/include
-  "include"
-  )
-
 add_subdirectory(include)
 add_subdirectory(src)
 
 if (BUILD_CPP_TESTS)
   add_subdirectory(test)
 endif ()
+
+# Generate cmake package configuration files
+include(GNUInstallDirs)
+set(INCLUDE_INSTALL_DIR ${CMAKE_INSTALL_INCLUDEDIR}
+  CACHE PATH "Location of header files")

Review Comment:
   In general, we don't need to define our version of `CMAKE_INSTALL_INCLUDEDIR`.
   We can just use `CMAKE_INSTALL_INCLUDEDIR`. Most packaging systems including RPM specifies `CMAKE_INSTALL_INCLUDEDIR` automatically.



##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,29 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# This config sets the following variables in your project
+#
+#   orc_FOUND - true if orc is found on the system
+#   orc_VERSION - version of the found Orc
+#   orc_INCLUDE_DIR - where to find the Orc headers
+
+@PACKAGE_INIT@
+
+set(orc_VERSION "@ORC_VERSION@")

Review Comment:
   We don't need this because `write_basic_package_version_file()` does this automatically.



##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,29 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# This config sets the following variables in your project
+#
+#   orc_FOUND - true if orc is found on the system
+#   orc_VERSION - version of the found Orc
+#   orc_INCLUDE_DIR - where to find the Orc headers
+
+@PACKAGE_INIT@
+
+set(orc_VERSION "@ORC_VERSION@")
+set_and_check(orc_INCLUDE_DIR "@PACKAGE_INCLUDE_INSTALL_DIR@")

Review Comment:
   We don't need this. We should provide `orc::orc` target instead.
   
   If we provide `orc::orc` target, users can link to `liborc.*` by `target_link_libraries(user_target orc::orc)`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2056446786

   Could you help review this? I'm not sure if we need to generate orcTarget.cmake which I think is unnecesarry at the moment. @kou 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "ffacs (via GitHub)" <gi...@apache.org>.
ffacs commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1565794582


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,29 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# This config sets the following variables in your project::
+#
+#   ORC_FOUND - true if Orc found on the system
+#   ORC_VERSION - version of the found Orc
+#   ORC_INCLUDE_DIR - where to find the Orc headers
+
+@PACKAGE_INIT@
+
+set(ORC_VERSION "@ORC_VERSION@")

Review Comment:
   @PACKAGE_ORC_VERSION@ ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1574440859


##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -132,6 +154,9 @@ else ()
     BUILD_BYPRODUCTS "${SNAPPY_STATIC_LIB}")
 
   add_built_library (snappy_ep orc_snappy ${SNAPPY_STATIC_LIB} ${SNAPPY_INCLUDE_DIR})
+
+  list (APPEND ORC_VENDOR_DEPENDENCIES "vendored_snappy:${SNAPPY_STATIC_LIB_NAME}")
+  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:vendored_snappy>")

Review Comment:
   Ah, how about using `|` instead of `:` as a separator?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1574452384


##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -132,6 +154,9 @@ else ()
     BUILD_BYPRODUCTS "${SNAPPY_STATIC_LIB}")
 
   add_built_library (snappy_ep orc_snappy ${SNAPPY_STATIC_LIB} ${SNAPPY_INCLUDE_DIR})
+
+  list (APPEND ORC_VENDOR_DEPENDENCIES "vendored_snappy:${SNAPPY_STATIC_LIB_NAME}")
+  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:vendored_snappy>")

Review Comment:
   Fixed. Thanks for your help!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2068726019

   @deshanxiao Could you help check if this breaks vcpkg? Is it possible to add `ORC_PACKAGE_KIND=vcpkg` to remove the patch file https://github.com/microsoft/vcpkg/blob/9224b3bbd8df24999d85720b1d005dd6f969ade0/ports/orc/fix-cmake.patch?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2069529985

   Hi @wgtmac I checked vcpkg and the previous patch can not apply any more:
   
   ```
   -- Downloading https://github.com/wgtmac/orc/archive/ORC-1689.tar.gz -> wgtmac-orc-ORC-1689.tar.gz...
   -- Extracting source /home/deshanxiao/vcpkg/downloads/wgtmac-orc-ORC-1689.tar.gz
   -- Applying patch fix-cmake.patch
   CMake Error at scripts/cmake/z_vcpkg_apply_patches.cmake:34 (message):
     Applying patch failed: Checking patch c++/src/CMakeLists.txt..
   ```
   
   This will not break our existing version . Feel free to merge the PR. And I will make a PR to remove the patch. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1574429589


##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -132,6 +154,9 @@ else ()
     BUILD_BYPRODUCTS "${SNAPPY_STATIC_LIB}")
 
   add_built_library (snappy_ep orc_snappy ${SNAPPY_STATIC_LIB} ${SNAPPY_INCLUDE_DIR})
+
+  list (APPEND ORC_VENDOR_DEPENDENCIES "vendored_snappy:${SNAPPY_STATIC_LIB_NAME}")
+  list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:vendored_snappy>")

Review Comment:
   This will break the logic here: https://github.com/apache/orc/blob/2ae87d2da38a91728046d9c2fe36fb8b15db5690/c%2B%2B/orcConfig.cmake.in#L47-L49
   
   I can rename it to orc_vendored_snappy



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1576162617


##########
c++/CMakeLists.txt:
##########
@@ -21,3 +21,17 @@ add_subdirectory(src)
 if (BUILD_CPP_TESTS)
   add_subdirectory(test)
 endif ()
+
+# Generate cmake package configuration files
+include(CMakePackageConfigHelpers)
+configure_package_config_file(
+  orcConfig.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/orcConfig.cmake
+  INSTALL_DESTINATION ${ORC_INSTALL_CMAKE_DIR})
+write_basic_package_version_file(
+  ${CMAKE_CURRENT_BINARY_DIR}/orcConfigVersion.cmake
+  VERSION ${ORC_VERSION}
+  COMPATIBILITY SameMajorVersion)
+install(FILES
+  ${CMAKE_CURRENT_BINARY_DIR}/orcConfig.cmake

Review Comment:
   Oh, You are right. `configure_package_config_file` does not execute installation.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "stiga-huang (via GitHub)" <gi...@apache.org>.
stiga-huang commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1575763100


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@PACKAGE_INIT@
+
+set(ORC_VENDOR_DEPENDENCIES "@ORC_VENDOR_DEPENDENCIES@")
+set(ORC_SYSTEM_DEPENDENCIES "@ORC_SYSTEM_DEPENDENCIES@")
+
+if(DEFINED CMAKE_MODULE_PATH)
+  set(ORC_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH})
+else()
+  unset(ORC_CMAKE_MODULE_PATH_OLD)
+endif()
+set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}")
+
+include(CMakeFindDependencyMacro)
+foreach(dependency ${ORC_SYSTEM_DEPENDENCIES})
+  find_dependency(${dependency})

Review Comment:
   I might not understand this correctly. If this file is expected to be used by downstream projects, can we add an option to skip this? Impala finds the dependencies by itself and don't need the ORC lib to do so. Got an error like this in a quick try:
   ```
   CMake Error at /home/quanlong/workspace/orc/build/install/lib/cmake/orc/FindSnappy.cmake:70 (message):
     Could not find Snappy in system search paths.
   Call Stack (most recent call first):
     toolchain/toolchain-packages-gcc10.4.0/cmake-3.22.2/share/cmake-3.22/Modules/CMakeFindDependencyMacro.cmake:47 (find_package)
     /home/quanlong/workspace/orc/build/install/lib/cmake/orc/orcConfig.cmake:56 (find_dependency)
     CMakeLists.txt:362 (find_package)
   ```



##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+

Review Comment:
   I might not understand this correctly. If this file is expected to be used by downstream projects, can we add some comments about the result variables? E.g. https://github.com/Kitware/CMake/blob/d41252c91fd9d7c8fb95b7202adca278524780da/Modules/FindBoost.cmake#L42-L95
   or
   https://github.com/apache/impala/blob/9b05a205fec397fa1e19ae467b1cc406ca43d948/cmake_modules/FindOrc.cmake#L21-L24



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1577174723


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@PACKAGE_INIT@
+
+set(ORC_VENDOR_DEPENDENCIES "@ORC_VENDOR_DEPENDENCIES@")
+set(ORC_SYSTEM_DEPENDENCIES "@ORC_SYSTEM_DEPENDENCIES@")
+
+if(DEFINED CMAKE_MODULE_PATH)
+  set(ORC_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH})
+else()
+  unset(ORC_CMAKE_MODULE_PATH_OLD)
+endif()
+set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}")
+
+include(CMakeFindDependencyMacro)
+foreach(dependency ${ORC_SYSTEM_DEPENDENCIES})
+  find_dependency(${dependency})

Review Comment:
   I have also added XXX_ROOT env and variables, which aligns with what currently Impala does.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2058079951

   I did try to generate the target file but was having a hard time to deal with the third-party libraries and make it relocatable. Let me learn it deeper and try again. Thanks @kou! 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2062902382

   > FYI: Apache Arrow bundles their custom `FindXXX.cmake` and use them if they are needed: https://github.com/apache/arrow/blob/73386cb806a429875f1846b69e21beccfd339b21/cpp/cmake_modules/ThirdpartyToolchain.cmake#L236-L250
   
   I saw that. What I tried is to install FindXXX.cmake from Apache ORC but they are called before find_dependency in the config file, so I thought it is not a good idea to use them.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1565922199


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,29 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# This config sets the following variables in your project::
+#
+#   ORC_FOUND - true if Orc found on the system
+#   ORC_VERSION - version of the found Orc
+#   ORC_INCLUDE_DIR - where to find the Orc headers
+
+@PACKAGE_INIT@
+
+set(ORC_VERSION "@ORC_VERSION@")

Review Comment:
   No, this does not compile.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2058095278

   > FYI: Apache Arrow creates `libarrow_bundled_dependencies.a` that contains all `*.a` built by `externalproject_add()`/`fetchcontent`:
   > 
   > * https://github.com/apache/arrow/blob/b98763ad079f20e36d82268f9e7cf0db49fdd461/cpp/cmake_modules/BuildUtils.cmake#L74-L172
   > * https://github.com/apache/arrow/blob/b98763ad079f20e36d82268f9e7cf0db49fdd461/cpp/src/arrow/CMakeLists.txt#L924-L942
   > 
   > We can use only `libarrow_bundled_dependencies.a` for `libarrow.a`: https://github.com/apache/arrow/blob/b98763ad079f20e36d82268f9e7cf0db49fdd461/cpp/src/arrow/ArrowConfig.cmake.in#L103-L119
   
   Yes, I looked at that and it can be a separate improvement to Apache ORC because Apache Arrow does not use third-party libraries built by Apache ORC.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2068694134

   Finally I make it work for all modes (vendored, providing XXX_HOME, or conan) and this is ready for review. Would you mind taking a look at this again? @kou 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2071248675

   Thanks for the check and feedback! @h-vetinari 
   
   For the 2nd patch, unfortunately it requires some additional change (not just removing `STATIC` keyword). The only patch on the Conan side since Apache ORC 2.0.0 is doing the similar thing: https://github.com/conan-io/conan-center-index/blob/master/recipes/orc/all/conanfile.py#L135-L137. I will try to support building shared libraries natively but for now let's still patch this.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1575770010


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@PACKAGE_INIT@
+
+set(ORC_VENDOR_DEPENDENCIES "@ORC_VENDOR_DEPENDENCIES@")
+set(ORC_SYSTEM_DEPENDENCIES "@ORC_SYSTEM_DEPENDENCIES@")
+
+if(DEFINED CMAKE_MODULE_PATH)
+  set(ORC_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH})
+else()
+  unset(ORC_CMAKE_MODULE_PATH_OLD)
+endif()
+set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}")
+
+include(CMakeFindDependencyMacro)
+foreach(dependency ${ORC_SYSTEM_DEPENDENCIES})
+  find_dependency(${dependency})

Review Comment:
   No, this is required by ORC because it depends on them. You need to set Snappy_HOME (and other similar 3rd-party dependencies) to make it work.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1575772487


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+

Review Comment:
   Good suggestion! Let me add them.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1581223823


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@PACKAGE_INIT@
+
+set(ORC_VENDOR_DEPENDENCIES "@ORC_VENDOR_DEPENDENCIES@")
+set(ORC_SYSTEM_DEPENDENCIES "@ORC_SYSTEM_DEPENDENCIES@")
+
+if(DEFINED CMAKE_MODULE_PATH)
+  set(ORC_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH})
+else()
+  unset(ORC_CMAKE_MODULE_PATH_OLD)
+endif()
+set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}")
+
+include(CMakeFindDependencyMacro)
+foreach(dependency ${ORC_SYSTEM_DEPENDENCIES})
+  find_dependency(${dependency})

Review Comment:
   This target file will be adopted by Apache Arrow once released. I can also add a test to make sure it works (perhaps with vendored third-party libraries only). Let me think about it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2058085377

   FYI: Apache Arrow creates `libarrow_bundled_dependencies.a` that contains all `*.a` built by `externalproject_add()`/`fetchcontent`:
   * https://github.com/apache/arrow/blob/b98763ad079f20e36d82268f9e7cf0db49fdd461/cpp/cmake_modules/BuildUtils.cmake#L74-L172
   * https://github.com/apache/arrow/blob/b98763ad079f20e36d82268f9e7cf0db49fdd461/cpp/src/arrow/CMakeLists.txt#L924-L942
   
   We can use only `libarrow_bundled_dependencies.a` for `libarrow.a`: https://github.com/apache/arrow/blob/b98763ad079f20e36d82268f9e7cf0db49fdd461/cpp/src/arrow/ArrowConfig.cmake.in#L103-L119


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2062904451

   Apache Arrow uses `FindXXXAlt.cmake` when we need to use `CONFIG` CMake package.
   e.g.: https://github.com/apache/arrow/blob/main/cpp/cmake_modules/FindSnappyAlt.cmake


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1577092183


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@PACKAGE_INIT@
+
+set(ORC_VENDOR_DEPENDENCIES "@ORC_VENDOR_DEPENDENCIES@")
+set(ORC_SYSTEM_DEPENDENCIES "@ORC_SYSTEM_DEPENDENCIES@")
+
+if(DEFINED CMAKE_MODULE_PATH)
+  set(ORC_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH})
+else()
+  unset(ORC_CMAKE_MODULE_PATH_OLD)
+endif()
+set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}")
+
+include(CMakeFindDependencyMacro)
+foreach(dependency ${ORC_SYSTEM_DEPENDENCIES})
+  find_dependency(${dependency})

Review Comment:
   I guess you ran into this issue: https://github.com/apache/arrow/issues/41331#issuecomment-2071278787. In short, currently FindProtobuf.cmake will try to find config file first even you have set PROTOBUF_HOME. Let me fix this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1889:
URL: https://github.com/apache/orc/pull/1889#issuecomment-2071264602

   @stiga-huang 
   
   On the Apache Impala side, we can remove [FindOrc.cmake](https://github.com/apache/impala/blob/master/cmake_modules/FindOrc.cmake) by doing following things:
   - Add the installed location of Apache ORC to CMAKE_PREFIX_PATH
   - `find_package(orc REQUIRED CONFIG)`
   - `target_link_libraries(${PROJECT_NAME} PRIVATE orc::orc)`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1575913361


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+

Review Comment:
   Added. PTAL again.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1576143645


##########
c++/CMakeLists.txt:
##########
@@ -21,3 +21,17 @@ add_subdirectory(src)
 if (BUILD_CPP_TESTS)
   add_subdirectory(test)
 endif ()
+
+# Generate cmake package configuration files
+include(CMakePackageConfigHelpers)
+configure_package_config_file(
+  orcConfig.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/orcConfig.cmake
+  INSTALL_DESTINATION ${ORC_INSTALL_CMAKE_DIR})
+write_basic_package_version_file(
+  ${CMAKE_CURRENT_BINARY_DIR}/orcConfigVersion.cmake
+  VERSION ${ORC_VERSION}
+  COMPATIBILITY SameMajorVersion)
+install(FILES
+  ${CMAKE_CURRENT_BINARY_DIR}/orcConfig.cmake

Review Comment:
   Where else did I install orcConfig.cmake?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1889:
URL: https://github.com/apache/orc/pull/1889#discussion_r1577100842


##########
c++/orcConfig.cmake.in:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@PACKAGE_INIT@
+
+set(ORC_VENDOR_DEPENDENCIES "@ORC_VENDOR_DEPENDENCIES@")
+set(ORC_SYSTEM_DEPENDENCIES "@ORC_SYSTEM_DEPENDENCIES@")
+
+if(DEFINED CMAKE_MODULE_PATH)
+  set(ORC_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH})
+else()
+  unset(ORC_CMAKE_MODULE_PATH_OLD)
+endif()
+set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}")
+
+include(CMakeFindDependencyMacro)
+foreach(dependency ${ORC_SYSTEM_DEPENDENCIES})
+  find_dependency(${dependency})

Review Comment:
   The error message means that it tried to read `Protobuf_FOUND` but we set `PROTOBUF_FOUND`. This is fixed now. Would you mind trying it again?
   
   BTW, we do not build ORC again when find_package(orc) is called. Instead, it simply tries to find linked interface libraries and sets the correct library installed location to them.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org