Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install fbjniLibraryConfig.cmake and headers correctly #76

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jonringer
Copy link

@jonringer jonringer commented Jun 19, 2022

It's pretty common for the lib directory to be added
to the cmake modules path, but much less so for
share.

This is to better align with dominate cmake pratices.

Motivation

Why?

Met some awkwardness:

CMake Error at CMakeLists.txt:18 (find_package):
  Could not find a package configuration file provided by "fbjni" with any of
  the following names:

    fbjniConfig.cmake
    fbjni-config.cmake

  Add the installation prefix of "fbjni" to CMAKE_PREFIX_PATH or set
  "fbjni_DIR" to a directory containing one of the above files.  If "fbjni"
  provides a separate development package or SDK, be sure it has been
  installed

Although /nix/store/grkksx8cdka4vmii6mv9fj9handf36m8-fbjni-0.3.0 was on my CMAKE_PREFIX_PATH, but cmake looks for ${CMAKE_PREFIX}/lib/cmake, not ${CMAKE_PREFIX}/share/cmake

Summary

What did you change?

Where it installs the fbjniLibraryConfig.cmake to lib

How does the code work?

GNUInstallDirs + CMake

Why did you choose this approach?

Most common pattern that I've seen

Test Plan

How did you test this change?

mkdir build && cd build && mkdir tmp_install  && cmake .. -DCMAKE_INSTALL_PREFIX=$PWD/tmp_install && make install

Adding a downstream project to test further would probably be good

Any change that adds functionality should add a unit test as well.

It's pretty common for the lib directory to be added
to the cmake modules path, but much less so for
share. 

This is to better align with dominate cmake pratices.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 19, 2022
@jonringer
Copy link
Author

another option would be to install in both locations, to avoid breaking changes for individuals

@jonringer jonringer changed the title Install fbjniLibraryConfig.cmake at CMAKE_INSTALL_LIBDIR Install fbjniLibraryConfig.cmake and headers correctly Jul 3, 2022
@jonringer
Copy link
Author

Also, added the headers as part of the install target, so that cmake --build --target install should be all that downstream users need to do for installation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants