Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why do we need FindEIGEN3.cmake ? #119

Closed
joxoby opened this issue Oct 17, 2020 · 13 comments
Closed

Why do we need FindEIGEN3.cmake ? #119

joxoby opened this issue Oct 17, 2020 · 13 comments

Comments

@joxoby
Copy link
Contributor

joxoby commented Oct 17, 2020

I'm having a hard time understanding why do we need FindEIGEN3.cmake

Can't we rely on the standard CMake lookup process and just do find_package(Eigen3 CONFIG) ?

Eigen3 comes with an Eigen3Config.cmake file.

By the way, I'm asking this question because I'm trying to add ignition-math as a Conan package: conan-io/conan-center-index#3215

@mxgrey
Copy link
Contributor

mxgrey commented Oct 19, 2020

If I recall correctly, it was only (relatively) recently that Eigen3 started providing a cmake config-file. The official debian package for Eigen3 on one of the earlier platforms that we needed to support did not provide the config-file.

I don't remember which platform was missing the config-file, but if we no longer support that platform, then we can consider getting rid of the FindEIGEN3.cmake find-module.

@traversaro
Copy link
Contributor

I don't remember which platform was missing the config-file, but if we no longer support that platform, then we can consider getting rid of the FindEIGEN3.cmake find-module.

I guess the latest Ubuntu LTS without an Eigen3Config.cmake was Ubuntu 14.04 Trusty, see ros/cmake_modules#25 . However, if we drop the FindEIGEN3.cmake , where will be the pkg-config-related function ign_pkg_config_entry called?

@joxoby
Copy link
Contributor Author

joxoby commented Oct 19, 2020

@traversaro why do we need any pkg-config functionality?

@traversaro
Copy link
Contributor

traversaro commented Oct 19, 2020

All the ignition libraries generate .pc files, and so they need to know the pkg-config info of their dependencies, so they can generate correctly the Requires: line of the generated .pc . I personally do not use this functionality, but this is what is implemented now, unless I recall incorrectly.

@joxoby
Copy link
Contributor Author

joxoby commented Oct 19, 2020

I see. I wonder who is using the pkg-config feature or why do we support it.

@mxgrey
Copy link
Contributor

mxgrey commented Oct 19, 2020

Great point about pkg-config. The ign_find_package(~) command has some arguments to provide it manually, but it really is simpler to keep it all in one place.

Is there any problem with the FindEIGEN.cmake file that would motivate us to remove it? If it's causing collisions with other find-modules, then we could consider renaming it to something like FindIgnEigen.cmake.

@mxgrey
Copy link
Contributor

mxgrey commented Oct 19, 2020

I see. I wonder who is using the pkg-config feature or why do we support it.

We definitely have users who prefer pkg-config over cmake because pkg-config is considered more portable between different build systems, and not all of our users like to use cmake.

@joxoby
Copy link
Contributor Author

joxoby commented Oct 26, 2020

Is there any problem with the FindEIGEN.cmake file that would motivate us to remove it? If it's causing collisions with other find-modules, then we could consider renaming it to something like FindIgnEigen.cmake.

See: conan-io/conan-center-index#3268 (comment)

@mxgrey
Copy link
Contributor

mxgrey commented Oct 27, 2020

I don't know what that reviewer's concern is. Packaging and distributing cmake find-modules for non-cmake dependencies is considered good practice.

The only issue I can think of is if our find-modules conflict with another package's find-modules and someone wants to use both our package and the other package as dependencies for the same project, and the find-modules exhibit critically different behaviors.

@joxoby
Copy link
Contributor Author

joxoby commented Oct 27, 2020

I think that the question here is "Why are these FindXXX.cmake files part of ignition-cmake and not part of ignition-<lib>?"

The reviewer's concern is that when packaging ignition-<lib>, FindXXX.cmake can fail, in which case one would have to patch and repackage ignition-cmake.

ignition-cmake seems perfectly suitable to store macros that are used across all the different libraries. However, I'm not convinced that it should contain .cmake scripts to find libraries that are specific to each ignition project.

@mxgrey
Copy link
Contributor

mxgrey commented Oct 27, 2020

The reviewer's concern is that when packaging ignition-, FindXXX.cmake can fail

This is what I'm lost on. What does it mean for FindXXX.cmake to fail? Does it mean calling find_package(XXX) from cmake fails to find package XXX or is there some other meaning of failure that I'm not thinking of?

If that's what's meant by fail, then that's perfectly normal. find-modules are designed to fail gracefully when the desired package is not available. Cmake itself is distributed with lots of find-modules for packages that users might not actually have, like boost or protobuf.

I wonder if there may be some confusion here between the nature of a find-module and a config-file. Config-files are meant exclusively for packages that are definitely installed, but that's not what we're distributing here. We're distributing find-modules which are redistributable cmake modules whose job is to sniff around for the presence of a package and declare whether the package is available or not.

I can agree that there's a fair debate to be had about whether the "best" place for these modules would be ignition-cmake or if it would be the respective ignition libraries that depend on them. The decision to put them all in ignition-cmake was based on primarily these factors:

  1. It's easier to maintain all the heavy lifting related to cmake in one repo. The downstream packages don't need to do anything special related to cmake except declare their dependencies and name their libraries.
  2. If multiple downstream packages share a dependency, we only need to have one copy of the find-module for it, here in this repo.
  3. We only need to worry about putting one path into CMAKE_MODULE_PATH. If every downstream package hosted its own find-modules, that variable could get bloated quickly. This isn't a terrible problem, but I felt it would be cleaner this way.

The only downside I can think of for putting the find-modules in ignition-cmake is that we need to bump the minor version of this package any time a downstream package takes on a new dependency. This happens rarely enough that it's never been a bother for us.

@mxgrey
Copy link
Contributor

mxgrey commented Oct 27, 2020

tl;dr: find-modules are just another type of cmake module, just like the cmake modules that provide functions or macros. They don't have any packaging dependencies or requirements besides cmake.

@scpeters
Copy link
Member

this conversation seems to have reached a stopping point. I'll close for now; please reopen if there are still concerns

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

No branches or pull requests

4 participants