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.

Sign up for GitHub

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

CMake Install Destination #660

Merged
merged 2 commits into from
Jan 7, 2021
Merged

CMake Install Destination #660

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd9d501
use older form of CMake install
varunagrawal Jan 6, 2021
6309917
add docs for cmake update
varunagrawal Jan 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions wrap/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,15 @@ install(FILES cmake/gtwrapConfig.cmake cmake/PybindWrap.cmake

# Install wrapping scripts as binaries to `CMAKE_INSTALL_PREFIX/bin` so they can
# be invoked for wrapping.
install(PROGRAMS scripts/pybind_wrap.py scripts/matlab_wrap.py TYPE BIN)
# We use DESTINATION (instead of TYPE) so we can support older CMake versions.
install(PROGRAMS scripts/pybind_wrap.py scripts/matlab_wrap.py
DESTINATION ${CMAKE_INSTALL_BINDIR})

# Install pybind11 directory to `CMAKE_INSTALL_PREFIX/lib/pybind11` This will
# allow the gtwrapConfig.cmake file to load it later.
install(DIRECTORY pybind11 TYPE LIB)
# We use DESTINATION (instead of TYPE) so we can support older CMake versions.
install(DIRECTORY pybind11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, Varun.

A quick question to get me up to speed:

  1. Can you remind me why we need the wrap lib and can't use pybind11 out of the box?

Could you also explain a little more here about what the DESTINATION vs. TYPE argument does and why one is preferred?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We use wrap to convert the gtsam.i file into a Pybind11 friendly file that can then be generated into a python package. So it's basically a transpiler for convenience and backwards compatibility.
  2. TYPE is the new form where specifying what type of object it is will let CMake decide where to install it to (e.g. type BIN will install in to /usr/local/bin). DESTINATION on the other hand is an explicit argument specifying where to install the object to, taking that decision away from CMake.

Copy link
Contributor

@johnwlambert johnwlambert Jan 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks, so wrap and the .i file are used so that we don't have to embed code like this into every .h or .cpp file?

PYBIND11_MODULE(example, m) {
    m.doc() = "pybind11 example plugin"; // optional module docstring

    m.def("add", &add, "A function which adds two numbers");
}

Could you add a small bit of documentation explaining why DESTINATION is preferred here and why we are not using BIN?

And can we also add a unit test or change the CI environment so that these kind of breaking changes are caught next time? Maybe this will take a bit of discussion, but along the principles of there should be a unit test that was broken before, and is fixed now with the PR etcc

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes exactly. The wrap package lets us define a simple .i file with specific semantics which then gets wrapped into Python, MATLAB etc. Wrap allows us to support multiple languages simultaneously rather than be Pybind11 specific.
  2. BIN is actually preferred in the new CMake versions, but DESTINATION is the older API. Added the documentation.
  3. This would involve a lot more work than I have the bandwidth for: We'd have to set up a docker container with the specific virtual environment and toolchain, have that be available on the BorgLab docker registry, and then modify the CI to use the new container as our CI environment. Alternatively, we could install an older version of CMake directly, but that's going to increase the CI time (breaking development flow due to longer wait times) and potentially cause inconsistencies with the default environment depending on what other tools are installed (do they depend on the CMake 3.19 API specifically?). I am already spread too thin to take up all of this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Ok, I'm approving for now, but I do feel that an easier fix is just to force users to use cmake 3.19 instead of >3.9. I think people could do the upgrade.

DESTINATION ${CMAKE_INSTALL_LIBDIR})

# ##############################################################################
# Install the Python package
Expand Down