Header files installing to root

[gchenfc]

Action items:

• @varunagrawal do you have any ideas about this? If not, this is not urgent or anything but just puzzling.
• @arutkowski could you try

  message("CMAKE INSTALL INCLUDE DIR is set to:    ${CMAKE_INSTALL_INCLUDEDIR}")

  at the bottom of GTDynamics/gtdynamics/CMakeLists.txt ?

---

@arutkowski is having a strange issue on his machine where cmake is installing the .h files into /gtdynamics/... (root) instead of ${CMAKE_PREFIX_PATH}/include/gtdynamics/.... Interestingly, the .so file is properly installed into ${CMAKE_PREFIX_PATH}/lib/libgtdynamics.so. Adam worked around it by manually copying the files into the correct directory, but this is not ideal.

Upon reading more carefully, I think maybe this is related to the cmake builtin CMAKE_INSTALL_INCLUDEDIR used e.g. here:

GTDynamics/gtdynamics/CMakeLists.txt

Line 43 in 2636582

install(FILES ${GLOB_RESULT} DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}/${SOURCE_SUBDIR})

CMAKE_INSTALL_INCLUDEDIR was introduced in cmake 3.14 I think, though IIRC @arutkowski had 3.17 so it should have been there.
Nevertheless, maybe we should do cmake_minimum_required(VERSION 3.14), or just hard-code the includes install path as include/ instead of using CMAKE_INSTALL_INCLUDEDIR.

---

Finally I suspect the use of CMAKE_INSTALL_INCLUDEDIR may be inconsistent with

GTDynamics/gtdynamics/CMakeLists.txt

Lines 67 to 76 in 2636582

	# Add includes for source directories 'BEFORE' any other include
	# paths so that the compiler uses GTDynamics headers in our source directory instead
	# of any previously installed GTDynamics headers.
	target_include_directories(gtdynamics BEFORE PUBLIC
	# main gtdynamics includes:
	$<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}>
	$<INSTALL_INTERFACE:include/>
	# config.h
	$<BUILD_INTERFACE:${CMAKE_BINARY_DIR}>
	)

which does hardcode the include/ folder install path.

[varunagrawal]

Hardcoding include/ is not a good idea since that can work differently for different machines. I know for a fact that include locations are different on linux vs macos. I would like to resolve that inconsistency directly and remove all hardcoding since that is shoving in assumptions onto cmake which then causes headaches down the line.

Right now, I was going to suggest what @gchenfc said, which is print out the value of CMAKE_INSTALL_INCLUDEDIR.
Also for this kind of issue, I need more information such as what is their OS, cmake version, what options are they setting etc. The more info the better.

[varunagrawal]

Okay after going through the CMake documentation and really understanding what's going on here, here's the explanation of the 2 snippets:

1. The install command is just telling CMake to add the header files to the specified install location. The install prefix is automatically added, so the final destination becomes ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}/${SOURCE_SUBDIR} which in the case of factors will evaluate to /usr/local/include/gtdynamics/factors. This is the command that actually does the installation for the headers since all headers are in subdirectories.
2. The target_include_directories specifies properties for the build target, in this case gtdynamics. In the build phase, it sets the include directory to ${CMAKE_SOURCE_DIR} so that #include "gtdynamics/X.h" works as expected when compiling. In the install phase however it sets the include directory to <install-prefix>/include, not for building but for specifying where to look when any downstream applications try to link to gtdynamics. So $<INSTALL_INTERFACE:include/> is in fact correct and should not be changed.

My strongest guess is that @arutkowski is using some different setup which doesn't have GNUInstallDirs available, so we'll have to add include(GNUInstallDirs) to the cmake before any use of CMAKE_INSTALL_INCLUDEDIR. Of course, this is conjecture without knowing the true details of the underlying system.

[dellaert]

I think always setting the same, non /use/local prefix should help. Does that work for you, Gerry?

[gchenfc]

I think always setting the same, non /use/local prefix should help. Does that work for you, Gerry?

Correct me if I'm wrong - you're saying that we should always be using -DCMAKE_INSTALL_PREFIX=path where path != /usr/local, right? If so, yes I expect this to work but I think the issue is that cmake on Adam's machine is not automatically prepending CMAKE_INSTALL_PREFIX as expected. I have on my machine (and we also did on Adam's machine) to always use -DCMAKE_PREFIX_PATH=/home/gerry/, but for some reason on Adam's machine, even with that option, gtdynamics is installing the header files into /installs instead of /home/adam/installs. Since Adam's machine IS properly installing the .so file into /home/adam/lib (i.e. it's properly installing the linked library but not the include header files), then therefore I hypothesized maybe it's related to the use of CMAKE_INSTALL_INCLUDEDIR. I should have explained this more clearly/succinctly in the original issue text.

---

My strongest guess is that @arutkowski is using some different setup which doesn't have GNUInstallDirs available, so we'll have to add include(GNUInstallDirs) to the cmake before any use of CMAKE_INSTALL_INCLUDEDIR. Of course, this is conjecture without knowing the true details of the underlying system.

Yes I think this may be it??? We maybe need include(GNUInstallDirs) ? @arutkowski could you try adding that include e.g. near the beginning of the root CMakeLists next to the add_compile_options(-faligned-new) line, then clearing build directory and re-installing gtdynamics? Basically you can just try make install withOUT sudo and if it succeeds, then we're good and can PR that addition. If it fails (permissions error), then that's not the solution (and your previous install should not be broken)

---

@varunagrawal I think there may be a misunderstanding due to my imprecise wording (I'm familiar with install/target_include_directories and generator expressions). I'm trying to say that

$<INSTALL_INTERFACE:include/> 

should be

$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>

or something.
Since $<INSTALL_INTERFACE:include/> makes it so hypothetically if someone has a non-standard CMAKE_INTSALL_INCLUDEDIR, then it'll install gtdynamics header files into the non-standard locations but then downstream applications will be including the wrong location for header files since they'll be using include/ (well actually I guess we haven't packaged gtdynamics into a find_package-compatible module yet so I think it doesn't make a difference yet, but anyway).

Well anyway this is a minor note that isn't that important atm, the main question is why Adam's machine is not using CMAKE_PREFIX_PATH.

[varunagrawal]

To answer that question, I need details about Adam's machine and setup.

[arutkowski]

Indeed, adding include(GNUInstallDirs) to the root CMakeLists seemed to do the trick; the header files now go in the proper place. However, I had to set GTDYNAMICS_BUILD_EXAMPLES to OFF to work around this error after executing make install:

make[2]: *** No rule to make target '/home/adam/installs/lib/libgtdynamics.so', needed by 'examples/example_forward_dynamics/example_forward_dynamics'.  Stop.
CMakeFiles/Makefile2:5791: recipe for target 'examples/example_forward_dynamics/CMakeFiles/example_forward_dynamics.dir/all' failed
make[1]: *** [examples/example_forward_dynamics/CMakeFiles/example_forward_dynamics.dir/all] Error 2
Makefile:145: recipe for target 'all' failed
make: *** [all] Error 2

Following the suggestion from @gchenfc to add message("CMAKE INSTALL INCLUDE DIR is set to: ${CMAKE_INSTALL_INCLUDEDIR}") to the bottom of GTDynamics/gtdynamics/CMakeLists.txt, before I added the GNUInstallDirs, the output looked like this:

CMAKE INSTALL INCLUDE DIR is set to:

and after adding the GNUInstallDirs (before the line add_compile_options(-faligned-new)), it looks like this:

CMAKE INSTALL INCLUDE DIR is set to: include

For reference, here's some basic info on my machine:
cmake version: 3.18.4
OS: Kubuntu 18.04.5 64-bit

Although, at some point while trying to get this to work, cmake got updated to 3.22.1, and I don't remember doing that myself (I wonder if Homebrew had something to do with that, as I seem to have to reinstall it after every time I clear the build directory).

Also, I followed the directions for installing gtdynamics as described in the readme, but I used an absolute path for CMAKE_INSTALL_PREFIX. Both gtsam and gtdynamics are installed to /home/adam/installs.

[gchenfc]

Sorry for the late reply, glad that include(GNUInstallDirs) worked and looks like yetong made a PR.

I'm not able to reproduce the examples issue, is it possible it just needs a make clean or rm the build directory due to side effect of adding include(GNUInstallDirs) partway through or something?

[varunagrawal]

Should we close this as resolved?

[arutkowski]

I think it is safe to close this issue. I didn't follow up on checking whether or not I could build the examples. It's not a high priority for me. If I notice that problem again, I suggest that I could make a separate issue for that.