Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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
"$")
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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
"$")
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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
"$")
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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
"$")
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
"$")
```
--
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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
-$
-$
-$
-$>
-$>
-$>
-$>
+${ORC_INSTALL_INTERFACE_TARGETS}
PUBLIC
$
$
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
"$")
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
"$")
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
"$")
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
"$")
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
"$>")
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$>")
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
"$>")
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$>")
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
"$>")
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$>")
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
"$>")
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$>")
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
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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
+$
+$
+$
+$>
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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
+$
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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;\$;\$;\$;\$"
)
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] ORC-1689: [C++] Generate CMake config file [orc]
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-unsubscr...@orc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
