Skip to content

Conversation

@wxmerkt
Copy link
Member

@wxmerkt wxmerkt commented Oct 25, 2019

This is a WIP to test generating a CMake config (#98). This does not yet export the include directories properly (i.e., ${eigenpy_INCLUDE_DIRS} is not populated). I'd love some feedback on which steps I have missed to include :-)

@olivier-stasse
Copy link
Member

olivier-stasse commented Nov 2, 2019

Hi @wxmerkt , I gave a shot to CMake export + jrl-cmakemodule on dynamic-graph and I have a preliminary version working. It seems to me that the problem comes from the fact you do not install the include in your INSTALL macro (https://github.com/wxmerkt/eigenpy/blob/b518cbefbb0bca5e3aa33045d1e59e8dc6a36ac1/CMakeLists.txt#L130)
You should have something:

INSTALL(TARGETS ${PROJECT_NAME}
  EXPORT ${TARGETS_EXPORT_NAME}
  PUBLIC_HEADER 
  INCLUDES DESTINATION include
...

and then for target_include_directories the INTERFACE option specifies the directory to include,
and it seems to me that:

$<INSTALL_INTERFACE:include>

will create the entry

${_IMPORT_PREFIX}/include

in your eigenpyTargets.cmake file
while your current value

$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>

is probably ignored as far as I understood as it is used to build the interface.
But what I suspect is that your current CMakeLists.txt is not installing the header.
In addition you might have to specify PUBLIC_HEADER:

SET (PUBLIC_HEADER ${{PROJECT_NAME}_HEADERS})

Hope it helps.

@wxmerkt
Copy link
Member Author

wxmerkt commented Nov 4, 2019

Hi @olivier-stasse,
Thank you very much for looking into this and the detailed instructions. I think we need some additional steps to export PKG_CONFIG_APPEND_CFLAGS and other cflags dependencies into the target, i.e.:

  • Boost::python
  • Eigen
  • Python include directories

This minimal example works, but obviously all flags should be set by eigenpy::eigenpy/correctly exported:

project(test_test CXX)

find_package(eigenpy REQUIRED)

# TODO: This should not be necessary
find_package(PythonLibs REQUIRED)
include_directories(${PYTHON_INCLUDE_DIRS})

# TODO: This should not be necessary
find_package(Boost REQUIRED COMPONENTS python numpy)

# TODO: This should not be necessary
find_package(Eigen3 REQUIRED)

add_library(${PROJECT_NAME} test.cpp)
# TODO: This should only link against `eigenpy::eigenpy`
target_link_libraries(${PROJECT_NAME} PUBLIC eigenpy::eigenpy Boost::python Boost::numpy Eigen3::Eigen)

There is a ton of macros in cmake-modules that only use pkg-config and would need to be fixed. It may be easier to do a clean modern CMake from scratch.

Thanks,
Wolfgang

@wxmerkt wxmerkt changed the base branch from master to devel November 4, 2019 04:41
@jmirabel
Copy link
Contributor

jmirabel commented Nov 4, 2019

There is a ton of macros in cmake-modules that only use pkg-config and would need to be fixed. It may be easier to do a clean modern CMake from scratch.

I agree, although it requires some work.

@nim65s
Copy link
Contributor

nim65s commented Nov 4, 2019

Hi,

I pretty much think this is my job to do this kind of stuff ; so if you can wait a week or two, I can handle this.

@jcarpent
Copy link
Contributor

jcarpent commented Nov 4, 2019

I also agree with @wxmerkt and @jmirabel and it is related to discussions in stack-of-tasks/pinocchio#919
From my side, I can take the charge of moving Pinocchio, EigenPy and HPP-FCL towards this new features.

@olivier-stasse
Copy link
Member

This was for me the occasion to have a look to the impact to use modern CMake.
I agree that this will have a profund impact on the jrl-cmakemodules to the point it
does not make sense to start from the current one.
The new approach is suppose to make pkg-config useless.
As far as I can see, the main impact is that all the macros handling the PKG_CONFIG information should be switch to proper Targets.cmake files.
We have to check dependency by dependency what make sense.
Now given the number of packages, I am not sure that everything should fall down on the shoulder of Guilhem (thanks Justin for your proposal)

Another question: should we continue with submodule or use something else ?
I do not see real alternative (catkin is not suppose to survive ROS1)

@jmirabel
Copy link
Contributor

jmirabel commented Nov 4, 2019

Pure CMake based and pkg-config based configuration have both their downsides. The cons of CMake is that you cannot use CMake files outside of CMake.

Commiting to CMake is fine for me. The submodule can be useful to configure the generate of the cmake config, target and version files. I also think it is a good idea to continue generating pc files on linux and mac.

@jcarpent
Copy link
Contributor

jcarpent commented Nov 4, 2019

@jmirabel I do not think we should continue this discussion but rather on the cmake modules ;) But I agree, we should mainteed the generation of the pkg-config files.

… MoveIt

- MoveIt cannot make a new release right now with our bugfix due to people constraints
- We cannot roll back due to project constraints
- This adds a cmake config that internally uses the pkg-config. This will be replaced in the future with one with proper CMake targets
@wxmerkt wxmerkt changed the title Add CMake config generation Add CMake config using pkg-config internally Nov 6, 2019
@wxmerkt
Copy link
Member Author

wxmerkt commented Nov 6, 2019

This is now changed to install a cmake config that internally uses pkg-config. This is a temporary solution to fix the MoveIt builds (per discussion at IROS) and provide backward compatibility with a previously released version.

When the targets-export-version in jrl-cmake supports dependencies etc. that are currently handled via pkg-config macros, we will overwrite the cmake config with an automatically generated one (via SETUP_PROJECT_PACKAGE_FINALIZE).

@wxmerkt wxmerkt marked this pull request as ready for review November 6, 2019 14:38
@jcarpent
Copy link
Contributor

jcarpent commented Nov 6, 2019

@wxmerkt Do you need a release?

@jcarpent jcarpent merged commit cdea5a8 into stack-of-tasks:devel Nov 6, 2019
@wxmerkt
Copy link
Member Author

wxmerkt commented Nov 7, 2019

@jcarpent: Yes, please! :-)

@jcarpent
Copy link
Contributor

jcarpent commented Nov 7, 2019

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants