Mailing-List: contact dev-help@orc.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@orc.apache.org
From: jcrist <git@git.apache.org>
To: dev@orc.apache.org
Reply-To: dev@orc.apache.org
Message-ID: <git-pr-192-orc@git.apache.org>
Subject: [GitHub] orc pull request #192: Cleanup cmake scripts
Content-Type: text/plain
Date: Tue, 21 Nov 2017 21:58:25 +0000 (UTC)
archived-at: Tue, 21 Nov 2017 21:58:27 -0000

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.crist@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.crist@gmail.com>
Date:   2017-11-20T23:56:53Z

    find_package required errors if not found

commit 3dcd68e900a3ea165f3df426dfa154d53c2f906f
Author: Jim Crist <jiminy.crist@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.

----


---