[GitHub] orc issue #188: ORC-263: Implement column writers of compound types
Github user majetideepak commented on the issue: https://github.com/apache/orc/pull/188 @wgtmac I will look into this PR by the end of this week. Thanks! ---
[GitHub] orc pull request #192: Cleanup cmake scripts
Github user majetideepak commented on a diff in the pull request: https://github.com/apache/orc/pull/192#discussion_r152654881 --- Diff: cmake_modules/ThirdpartyToolchain.cmake --- @@ -10,19 +10,46 @@ # See the License for the specific language governing permissions and # limitations under the License. +set (LZ4_VERSION "1.7.5") +set (SNAPPY_VERSION "1.1.4") +set (ZLIB_VERSION "1.2.11") +set (GTEST_VERSION "1.8.0") +set (PROTOBUF_VERSION "2.6.0") + set (THIRDPARTY_DIR "${CMAKE_BINARY_DIR}/c++/libs/thirdparty") string(TOUPPER ${CMAKE_BUILD_TYPE} UPPERCASE_BUILD_TYPE) +if (DEFINED ENV{SNAPPY_HOME}) + set (SNAPPY_HOME "$ENV{SNAPPY_HOME}") +endif () + +if (DEFINED ENV{ZLIB_HOME}) + set (ZLIB_HOME "$ENV{ZLIB_HOME}") +endif () + +if (DEFINED ENV{LZ4_HOME}) + set (LZ4_HOME "$ENV{LZ4_HOME}") +endif () + +if (DEFINED ENV{PROTOBUF_HOME}) + set (PROTOBUF_HOME "$ENV{PROTOBUF_HOME}") +endif () + +if (DEFINED ENV{GTEST_HOME}) + set (GTEST_HOME "$ENV{GTEST_HOME}") +endif () + # -- # Snappy -set (SNAPPY_HOME "$ENV{SNAPPY_HOME}") -find_package (Snappy) -if (NOT SNAPPY_FOUND) +if (NOT "${SNAPPY_HOME}" STREQUAL "") + find_package (Snappy REQUIRED) + set(SNAPPY_VENDORED FALSE) +else () --- End diff -- I would like the library to be vendored in the case where `SNAPPY_HOME` is set, but `SNAPPY_FOUND` is inferred as `false` ---
[GitHub] orc pull request #192: Cleanup cmake scripts
Github user majetideepak commented on a diff in the pull request: https://github.com/apache/orc/pull/192#discussion_r152657567 --- Diff: cmake_modules/ThirdpartyToolchain.cmake --- @@ -41,32 +68,35 @@ if (NOT SNAPPY_FOUND) LOG_BUILD 1 LOG_INSTALL 1 BUILD_BYPRODUCTS "${SNAPPY_STATIC_LIB}") + + set (SNAPPY_VENDORED TRUE) endif () -include_directories (SYSTEM ${SNAPPY_INCLUDE_DIRS}) + +include_directories (SYSTEM ${SNAPPY_INCLUDE_DIR}) add_library (snappy STATIC IMPORTED) set_target_properties (snappy PROPERTIES IMPORTED_LOCATION ${SNAPPY_STATIC_LIB}) -set (SNAPPY_LIBRARIES snappy) -add_dependencies (snappy snappy_ep) -install(DIRECTORY ${SNAPPY_PREFIX}/lib DESTINATION . --- End diff -- AFAIK `cpack` depends on `install` as well to make a package. So I would vote for your other option of using `INSTALL_THIRDPARTY_LIBS=on`. ---
[GitHub] orc pull request #192: Cleanup cmake scripts
Github user jcrist commented on a diff in the pull request: https://github.com/apache/orc/pull/192#discussion_r152660084 --- Diff: cmake_modules/ThirdpartyToolchain.cmake --- @@ -10,19 +10,46 @@ # See the License for the specific language governing permissions and # limitations under the License. +set (LZ4_VERSION "1.7.5") +set (SNAPPY_VERSION "1.1.4") +set (ZLIB_VERSION "1.2.11") +set (GTEST_VERSION "1.8.0") +set (PROTOBUF_VERSION "2.6.0") + set (THIRDPARTY_DIR "${CMAKE_BINARY_DIR}/c++/libs/thirdparty") string(TOUPPER ${CMAKE_BUILD_TYPE} UPPERCASE_BUILD_TYPE) +if (DEFINED ENV{SNAPPY_HOME}) + set (SNAPPY_HOME "$ENV{SNAPPY_HOME}") +endif () + +if (DEFINED ENV{ZLIB_HOME}) + set (ZLIB_HOME "$ENV{ZLIB_HOME}") +endif () + +if (DEFINED ENV{LZ4_HOME}) + set (LZ4_HOME "$ENV{LZ4_HOME}") +endif () + +if (DEFINED ENV{PROTOBUF_HOME}) + set (PROTOBUF_HOME "$ENV{PROTOBUF_HOME}") +endif () + +if (DEFINED ENV{GTEST_HOME}) + set (GTEST_HOME "$ENV{GTEST_HOME}") +endif () + # -- # Snappy -set (SNAPPY_HOME "$ENV{SNAPPY_HOME}") -find_package (Snappy) -if (NOT SNAPPY_FOUND) +if (NOT "${SNAPPY_HOME}" STREQUAL "") + find_package (Snappy REQUIRED) + set(SNAPPY_VENDORED FALSE) +else () --- End diff -- As I wrote it, we error out if `SNAPPY_HOME` is set but not found (since `REQUIRED` is set). I think this makes more sense - as a user if I specify `SNAPPY_HOME` I'd rather get an error than silently download and build a new version if it fails to find it. ---
[GitHub] orc pull request #192: Cleanup cmake scripts
Github user majetideepak commented on a diff in the pull request: https://github.com/apache/orc/pull/192#discussion_r152662030 --- Diff: cmake_modules/ThirdpartyToolchain.cmake --- @@ -10,19 +10,46 @@ # See the License for the specific language governing permissions and # limitations under the License. +set (LZ4_VERSION "1.7.5") +set (SNAPPY_VERSION "1.1.4") +set (ZLIB_VERSION "1.2.11") +set (GTEST_VERSION "1.8.0") +set (PROTOBUF_VERSION "2.6.0") + set (THIRDPARTY_DIR "${CMAKE_BINARY_DIR}/c++/libs/thirdparty") string(TOUPPER ${CMAKE_BUILD_TYPE} UPPERCASE_BUILD_TYPE) +if (DEFINED ENV{SNAPPY_HOME}) + set (SNAPPY_HOME "$ENV{SNAPPY_HOME}") +endif () + +if (DEFINED ENV{ZLIB_HOME}) + set (ZLIB_HOME "$ENV{ZLIB_HOME}") +endif () + +if (DEFINED ENV{LZ4_HOME}) + set (LZ4_HOME "$ENV{LZ4_HOME}") +endif () + +if (DEFINED ENV{PROTOBUF_HOME}) + set (PROTOBUF_HOME "$ENV{PROTOBUF_HOME}") +endif () + +if (DEFINED ENV{GTEST_HOME}) + set (GTEST_HOME "$ENV{GTEST_HOME}") +endif () + # -- # Snappy -set (SNAPPY_HOME "$ENV{SNAPPY_HOME}") -find_package (Snappy) -if (NOT SNAPPY_FOUND) +if (NOT "${SNAPPY_HOME}" STREQUAL "") + find_package (Snappy REQUIRED) + set(SNAPPY_VENDORED FALSE) +else () --- End diff -- Ok. Makes sense. Thanks. ---
[jira] [Created] (ORC-266) [C++] A few issues with cmake scripts and non-vendored third party libraries
Jim Crist created ORC-266: - Summary: [C++] A few issues with cmake scripts and non-vendored third party libraries Key: ORC-266 URL: https://issues.apache.org/jira/browse/ORC-266 Project: ORC Issue Type: Bug Components: C++ Reporter: Jim Crist Currently there are a few issues with the cmake build system: - Running `make install` (or `make package`) when using any non-vendored third party library results in copying all files not matching `"*.so|*.dylib"` from the `*_HOME/lib` directory into the `lib/` directory. This is because third party libraries are being installed via a filter instead of specific file paths. This is definitely a bug. - There is no easy way to specify third party library locations when using orc as an `ExternalProject` in another cmake project. This is easily fixed by separating the defining of `*_HOME` variables from their use, which allows e.g. `-DSNAPPY_HOME=...` to work. - There is no way to avoid building the tests - There is no way to avoid building the tools - The LICENSE and NOTICE get installed into the root directory, which is not where they should go. Since the header files all contain the license header, and other apache projects don't install the license, the lines installing these files should be removable. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] orc pull request #192: ORC-266: [C++] Cleanup cmake scripts
Github user jcrist commented on a diff in the pull request: https://github.com/apache/orc/pull/192#discussion_r152686927 --- Diff: cmake_modules/ThirdpartyToolchain.cmake --- @@ -41,32 +68,35 @@ if (NOT SNAPPY_FOUND) LOG_BUILD 1 LOG_INSTALL 1 BUILD_BYPRODUCTS "${SNAPPY_STATIC_LIB}") + + set (SNAPPY_VENDORED TRUE) endif () -include_directories (SYSTEM ${SNAPPY_INCLUDE_DIRS}) + +include_directories (SYSTEM ${SNAPPY_INCLUDE_DIR}) add_library (snappy STATIC IMPORTED) set_target_properties (snappy PROPERTIES IMPORTED_LOCATION ${SNAPPY_STATIC_LIB}) -set (SNAPPY_LIBRARIES snappy) -add_dependencies (snappy snappy_ep) -install(DIRECTORY ${SNAPPY_PREFIX}/lib DESTINATION . --- End diff -- Done. Went with `INSTALL_VENDORED_LIBS`, which defaults to on. ---
[GitHub] orc issue #192: ORC-266: [C++] Cleanup cmake scripts
Github user jcrist commented on the issue: https://github.com/apache/orc/pull/192 Opened issue https://issues.apache.org/jira/browse/ORC-266. I think this should be good now? ---
[GitHub] orc issue #192: ORC-266: [C++] Cleanup cmake scripts
Github user majetideepak commented on the issue: https://github.com/apache/orc/pull/192 +1 LGTM. Thanks! I will verify if these changes pass on all our supported OSes and merge this. ---