ethz-asl / gtsam_catkin

A catkin wrapper for GTSAM
12 stars 17 forks source link

Side-effects related to libpointmatcher #24

Closed danieldugas closed 6 years ago

danieldugas commented 6 years ago

Note: This is a really strange one and I'm still looking at possible ways I might have done something wrong. Still, after this much testing It seems like an issue might be in order.

Environment:

Behavior:

Steps taken:

test_proj/CMakeLists.txt:

cmake_minimum_required(VERSION 2.8.3)
project(test_proj)

add_definitions(-std=c++11)

find_package(catkin_simple 0.1.0 REQUIRED)
catkin_simple(ALL_DEPS_REQUIRED)

cs_add_executable(test_proj_exe test_proj.cpp)

cs_install()
cs_export()

test_proj/package.xml:

<?xml version="1.0"?>

<package format="2">
  <name>test_proj</name>
  <version>0.0.1</version>
  <description>WIP</description>

  <author email="dugasd@ethz.ch">Daniel Dugas</author>
  <maintainer email="dugasd@ethz.ch">Daniel Dugas</maintainer>

  <license>BSD</license>

  <buildtool_depend>catkin</buildtool_depend>
  <buildtool_depend>catkin_simple</buildtool_depend>
  <depend>gtsam_catkin</depend>

</package>

test_proj/test_proj.cpp:

#include <gtsam/nonlinear/ISAM2.h>

int main(int argc, char **argv) {
}

after cloning gtsam_catkin and catkin_simple to the workspace, the same error is shown: fatal error: gtsam/nonlinear/ISAM2.h: No such file or directory

Compilation is then successful. I also applied the same fix to my own project and also immediately observed successful compilation. Any thoughts?

danieldugas commented 6 years ago

I've reproduced this error on another 16.04 machine.

test_proj.zip

HannesSommer commented 6 years ago

Thanks @danieldugas for the deep digging. The gtsam_catkin's CMakeLists.txt & gtsam-extra.cmake were incomplete such that they didn't export the ${CATKIN_DEVEL_PREFIX}/include as include folder despite this being the place where it gets installed to. The libpointmatcher "sideeffect" was to rightfully export this include folder because it is also installing to it.

By lucky coincidence @eggerk recently solved this with the already merged PR #26 (as a cleanup component). Hence, current master should behave nicer in this respect. However, it also comes with a new catkin dependency (https://github.com/ethz-asl/metis_catkin).

danieldugas commented 6 years ago

Thanks @HannesSommer , it is indeed fixed!

danieldugas commented 6 years ago

I indeed had looked at the feature/install branch during my digging, but noticing the extra dependency and unsure as to which direction the branch was headed, I opted to remain on master. Cheers!

HannesSommer commented 6 years ago

Great! Thanks for feeding back :).