Pull up common YARP-related lines to root CMakeLists.txt

roboticslab-uc3m / questions-and-answers

A place for general debate and question&answer

https://robots.uc3m.es/developer-manual/appendix/repository-index.html

2 stars 0 forks source link

Pull up common YARP-related lines to root CMakeLists.txt #54

Closed PeterBowman closed 6 years ago

PeterBowman commented 6 years ago

Rationale:

https://github.com/roboticslab-uc3m/developer-manual/issues/18#issuecomment-375742784

Another CMake guideline proposal: set find_package(<pkg> REQUIRED) in root CMakeLists.txt for hard dependencies. Only a few repos in our org will meet this criteria: YARP in yarp-devices, OpenRAVE+Boost in openrave-yarp-plugins. See https://github.com/roboticslab-uc3m/yarp-devices/commit/2fdc802cb7eecd061af11829a3ade5e93c56b224.

https://github.com/roboticslab-uc3m/developer-manual/issues/18#issuecomment-375808909

Only a few repos in our org will meet this criteria

In spite of that, we can assume that most repos just need YARP for whatever purpose they exist for. When working on https://github.com/roboticslab-uc3m/questions-and-answers/issues/45, we observed that certain components inside share/ and libraries/YarpPlugins/ may require having set common installation variables (via yarp_configure_external_installation()). Thus, pulling YARP-related commands to root CMakeLists.txt will avoid code duplication and other potential issues.

This reasoning may be extended to cover other hard dependencies, e.g. OpenRAVE+Boost in openrave-yarp-plugins.

PeterBowman commented 6 years ago

Example: https://github.com/roboticslab-uc3m/yarp-devices/commit/2fdc802cb7eecd061af11829a3ade5e93c56b224.

PeterBowman commented 6 years ago

From https://github.com/roboticslab-uc3m/developer-manual/issues/18#issuecomment-379301520:

Also, there are cases in which CMake variables defined by Config.cmake or Find.cmake must be accessible in distinct folders. In order to avoid redundant find_package() calls, those could be arranged in one place as well (@jgvictores proposes cmake/<pkg>Dependencies.cmake). This resembles https://github.com/roboticslab-uc3m/questions-and-answers/issues/54, but covers soft dependencies, too.

Question: put all dependencies (soft & hard) together?

jgvictores commented 6 years ago

Question: put all dependencies (soft & hard) together?

IMHO, yes. Simple # cmake comment lines would be enough to serve as delimiters.

PeterBowman commented 6 years ago

Question: put all dependencies (soft & hard) together?

Use case: different versions requested for the same dependency. See:

jgvictores commented 6 years ago

Unordered thoughts:

I guess we should strive to use the same versions everywhere.
Not sure if bumping up or down is better, probably depends on use case.
The problem in the described issue was ROS enfocing its older version even if having an installed newer version.

To sum up, IMHO:

Keep the reasonable shared low version dependency near root.
If a specific optional component needs a very updated version of something say, difficult to install, let it be, in its own directory.

I do understand these directives are very rule-of-thumb and we are going to have to end up checking case by case, but I really can't come up with a straightforward solution that requires no analysis of the specific use case.

PeterBowman commented 6 years ago

Related: #7, #14. Regarding the latter, I'd state somewhere (README file at repo's root, or perhaps best practices if common to all repos?) what is the required minimum YARP (and other hard dependency) version. Specific components may expand on this matter in their own README file (see #29).

PeterBowman commented 6 years ago

Problem: YARP 3.0 is not compatible with YARP 2, thus find_package(YARP 2.x.x) will fail. This is currently causing errors in CI builds, but we can hack them around by checking out previous YARP releases during git clone.

PeterBowman commented 6 years ago

YARP 3.0 introduces components. However, only some of them might be required by specific downstream libs/apps. For example, if only libA needs YARP_math, raise a warning and disable it if not found (see https://github.com/roboticslab-uc3m/questions-and-answers/issues/18). By contrast, the following would not be desired in root CMakeLists.txt:

find_package(YARP 3.0 REQUIRED COMPONENTS OS dev math)

I hope to have this solved upstream by introducing support for optional components, see https://github.com/robotology/yarp/pull/1741.

PeterBowman commented 6 years ago

Problem: YARP 3.0 is not compatible with YARP 2, thus find_package(YARP 2.x.x) will fail. This is currently causing errors in CI builds, but we can hack them around by checking out previous YARP releases during git clone.

See https://github.com/roboticslab-uc3m/questions-and-answers/issues/65.

PeterBowman commented 6 years ago

I'd state somewhere (README file at repo's root, or perhaps best practices if common to all repos?) what is the required minimum YARP (and other hard dependency) version.

Regarding CMake, YCM and YARP, see current supported releases at https://github.com/roboticslab-uc3m/yarp-devices/issues/111#issuecomment-398427897. Also, I started reflecting these values at each repo's install guides (example).

PeterBowman commented 6 years ago

Question: put all dependencies (soft & hard) together?

Example: https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/8a328641807d9ac5523c3aab7f357b2346d03d6a. As you can see, I'm moving even those find_package() calls hidden behind an if() guard, too (i.e. ROBOTICSLAB_YARP_DEVICES and GTestSources).