Update target_include_directories to make geometry-central installable

[jdumas]

See this SO thread for a description of the issue.

[nmwsharp]

Hi! I just merged another PR addressing some of the same issues. #238

From my understanding this should be sorted out now. Please let me know if not! Otherwise I will close this PR next time I come back to it.

[jdumas]

Ugh. I don't like the changes in #238. Hardcoding the include directories for happly/nanort/etc. to relative paths will prevent any downstream users from using geometry-central via add_subdirectory() but using an external version of those dependencies.

[nmwsharp]

Ah thanks for the feedback @jdumas. I partly waited to cut a new version in case there were complaints :) It's hard to make everyone happy with build/install conventions but that doesn't mean there's not room for improvement.

Is there an easy fix that would make it possible?

My gut instinct is that I'm not sure we need/want to support users dropping-in other other versions of these dependencies, since they are small, internal to geometry-central, and do not appear in the public API. In fact, nanoflann and nanort are now fully private. happly is not private but perhaps it should be. If the user wants to use a different version in their own code, they can include it in their own build system. Does that correspond to your use-case?

(I speak only about small & header-only deps, Eigen is a different story of course!).

[jdumas]

No even if the targets are private it's still a problem because of ODR issues. The only way to make it completely safe is to wrap all the symbols of these private libraries in your own namespace to avoid symbol collisions and make you you capture all the symbols.

And of course need to you hide any implementation detail behind PIMPL idioms and not leaks any header, but you've done that part already.

[nmwsharp]

Ah gotcha on the ODR issues. How about we do move them to anonymous namespaces? It sounds like the right thing to do (even if it doesn't totally resolve your issue, not sure if it does).

Here's a PR doing that: #243

This is only for nanoflann & nanort, not happly. But since I'm also the maintainer of happly and it is getting almost no changes I'm not so worried about it.

[jdumas]

That's a bit of an ugly hack, but I guess this could work. This is still not great for any consumer of geometry-central though. Now if a memory issue or another security vulnerability is found is nanoflann or nanort, rather than updating a centralized version e.g. in a vcpkg registry or at the root of the consuming project, this requires patching geometry-central. With the previous approach one could simply override the dependency from the consuming project if the API was compatible. It works fine for a small project, but it scales poorly when you need to manage 100+ dependencies that all do some variation of this.

But since I'm also the maintainer of happly and it is getting almost no changes I'm not so worried about it.

In a sense, that makes it even worse. I'm still waiting for nmwsharp/happly#41 to be merged for example. If you won't merge the change, now I have to maintain a fork of geometry-central AND happly which is work that could be avoided easily. Even if I wanted to use the upstream version of happly, this line forces me to consume the exact same version used in geometry-central's submodule in my larger project. This makes it harder to pin happly's version centrally (e.g. using vcpkg or CPM) and decouple it from geometry-central's version.

Finally, this line with Eigen's include is also not needed. Include directories should be provided as a target property. Using a CMake variable for this if very brittle and can lead to discrepancies between the Eigen3::Eigen target property and the EIGEN3_INCLUDE_DIR variable (e.g. variables are scoped to folder/functions, while the target is scoped globally).

I think the old approach worked better all around. I'm not proposing to revert #238 altogether, simply to replace this block:

geometry-central/src/CMakeLists.txt

Lines 181 to 190 in 6c3779e

	target_include_directories(geometry-central
	PUBLIC
	$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../include>
	$<BUILD_INTERFACE:${EIGEN3_INCLUDE_DIR}>
	$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../deps/happly>
	$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
	PRIVATE
	"${CMAKE_CURRENT_SOURCE_DIR}/../deps/nanoflann/include"
	"${CMAKE_CURRENT_SOURCE_DIR}/../deps/nanort/include"
	)

with the proper per-target version:

include(GNUInstallDirs)
target_include_directories(geometry-central
    $<BUILD_INTERFACE:../include> # use relative path
    $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
)
target_link_libraries(geometry-central
  PUBLIC
    Eigen3::Eigen
    happly
  PRIVATE
    nanoflann
    nanort
)

After all, you do define the targets properly here, but now your code doesn't use them at all.

I'm happy to propose a PR in that direction, while also making sure we stay compatible with installed versions of geometry-central for the use-case #238 had in mind.

[nmwsharp]

@jdumas putting aside for a moment my own spirit quest to compartmentalize dependencies, would the proposed change in #244 address your need?

(I appreciate your answer---in short my personal feeling is that I'm not really sure we should support consumers exchanging versions of the library's internal dependencies; who knows what could break, and if they want to do that they should fork the library and take responsibility for a fork. But I also recognize there are many different needs here. If there's a simple fix that resolves the situation we should take it!)