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 <jiminy.cr...@gmail.com>
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 <jiminy.cr...@gmail.com>
Date: 2017-11-20T23:56:53Z
find_package required errors if not found
commit 3dcd68e900a3ea165f3df426dfa154d53c2f906f
Author: Jim Crist <jiminy.cr...@gmail.com>
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.
----
---
