CMake Install Destination

[varunagrawal]

Use an older form of CMake install so that it works with older CMake versions as well

[johnwlambert]

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?

[varunagrawal]

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.

[johnwlambert]

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

[varunagrawal]

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.

[johnwlambert]

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.