[GitHub] orc issue #192: ORC-266: [C++] Cleanup cmake scripts

2017-11-22 Thread jcrist
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?


---


[jira] [Created] (ORC-266) [C++] A few issues with cmake scripts and non-vendored third party libraries

2017-11-22 Thread Jim Crist (JIRA)
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: Cleanup cmake scripts

2017-11-22 Thread majetideepak
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

2017-11-22 Thread majetideepak
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`


---