[GitHub] orc pull request #192: Cleanup cmake scripts

[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

[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`


---

[GitHub] orc pull request #192: Cleanup cmake scripts

[jcrist]

GitHub user jcrist opened a pull request:

https://github.com/apache/orc/pull/192

Cleanup cmake scripts

This was an attempt to fix using local versions of thirdparty libraries (as 
added in https://github.com/apache/orc/pull/170) so that installing doesn't 
pull in every file matching not matching `*.so|*.dylib` in the containing `lib` 
folder (as it does now). However, it kind of spiraled out in scope as more 
issues developed. Apologies for dumping this here without prior discussion.

---

A summary of the backwards compatible changes:

- Cleanup handling of `find_package` scripts to:
  - Set consistent variable names, and also match conventions in other 
apache projects
  - Set consistent variable names in both `*_HOME` specified and vendored 
paths
  - Provide error messages when not found, and only search when `*_HOME` is 
specified
  - Allow specifying `*_HOME` paths in other cmake projects using orc as an 
`ExternalProject`. This allows the wrapping project to more easily specify 
location of third party libraries with orc (e.g. `snappy`).

- Add an option to not build the cpp tests, and only install/search for 
gtest if turned on

- Add an option to not build the tools

---

A few potentially more contentious changes:

- Don't install third party libraries, even when vendored. The current 
behavior installs the thirdparty libs even if not vendored, which feels wrong 
to me. It also accidentally copies every file in the `lib` directory that isn't 
a shared library, which is definitely a bug.

  At a minimum, the thirdparty libraries that aren't vendored shouldn't be 
installed, and the accidental copying fixed. I'd argue that no thirdparty 
library should be installed, as it may clobber existing versions of e.g. 
`protobuf.a`. Downstream projects may handle the linking and distribution of 
thirdparty libraries themselves.

  If people disagree, perhaps a flag `INSTALL_THIRDPARTY_LIBS=on` could be 
added, to allow downstream projects to turn this off?

- Don't install `NOTICE` and `LICENSE`. When running `make install` 
currently, these get installed in the root directory, which is not where they 
should go. Since the header files contain license headers already, and other 
apache projects don't install `NOTICE` and `LICENSE` when running `make 
install`, I think this should be fine?


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jcrist/orc cleanup-cmake-scripts

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/orc/pull/192.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #192


commit 28c5b20416359524c971ce7c536d561664aab489
Author: Jim Crist 
Date:   2017-11-20T21:15:15Z

Cleanup cmake

- Add option to build tools (default ON, matching current behavior)
- Add option to build tests (default ON, matching current behavior)
- Cleaned up cmake find-package logic to be more consistent, and match
  conventions in other libraries.
- Don't install third party libraries, even if vendored. This seems to
  match common practice in other libraries. At a minimum third party
  libraries shouldn't be installed if they're not vendored (currently
  they're installed either way).

commit 6630118c0614b5ede27917ab082154a9a7c71f5d
Author: Jim Crist 
Date:   2017-11-20T23:56:53Z

find_package required errors if not found

commit 3dcd68e900a3ea165f3df426dfa154d53c2f906f
Author: Jim Crist 
Date:   2017-11-21T21:28:46Z

Don't install LICENSE or NOTICE

- The header files contain the license headers
- Running make install installs LICENSE and NOTICE into the root
directory, which is not where they should go.

Instead, opt for not installing either, and rely on header licenses to
be sufficient.




---