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

Add relocatable CMakeConfig files. #652

Merged
merged 12 commits into from
Jun 23, 2020
Merged

Conversation

Nicogene
Copy link
Member

@Nicogene Nicogene commented Apr 6, 2020

This PR introduce a deep refactor on how the Config.cmake are generated inside icub-main.

With this PR all the libraries are handled as components so:
find_package(ICUB COMPONENTS iKin) and find_package(iKin) are equivalent.

If no components are specify here is the list of the components automatically added:

-- Found ICUB 1.16.0 (Components: iCubDev ctrlLib skinDynLib iKin iDyn learningMachine perceptiveModels actionPrimitives optimization)

(depends also if you have compiled some of them that are optional like optimization, perceptiveModels etc)

Here is the content of ${ICUB_LIBRARIES}: ICUB::iCubDev;ICUB::ctrlLib;ICUB::skinDynLib;ICUB::iKin;ICUB::iDyn;ICUB::learningMachine;ICUB::perceptiveModels;ICUB::actionPrimitives;ICUB::optimization

For now the embobj libs( ethResources, canLoaderLib, ethLoaderLib) have not been ported to COMPONENTS due to :robotology/ycm-cmake-modules#340

If fixes #651

@pattacini
Copy link
Member

Exporting ICUB_LIBRARIES is definitely needed in order to avoid breaking current code.

@traversaro
Copy link
Member

I think that given the constraint that we need still to support ICUB_LIBRARIES exported variables, and we also need to support the non-namespaced exported targets (as they are quite used in downstream projects, see https://github.com/search?l=CMake&q=org%3Arobotology+ctrlLib&type=Code), I guess that for now we cannot remove the use of the list of targetes built within the project (that is contained in the global property ICUB_TARGETS).

For this reason, I think that for now it is a better idea to modify the existing icub_export_library macro with the logic that we need, instead of removing it and duplicate the code related to installation for each library.

Once we have the list of targets again in ICUB_TARGETS, I think we can modify the install_basic_package_files function to use a specific CONFIG_TEMPLATE CMake, so that we can put some additional logic in the CMake config file. In particular, we can start with the CMake config template automatically created by install_basic_package_files . There we can define the ICUB_LIBRARIES variable to contain all the targets defined in ICUB_TARGETS, and also add non-namespaced targets that permit to link the corresponding imported targets (i.e. add the target ctrlLib that links ICUB::ctrLib). If we want to eventually just switch to use ICUB::ctrlLib, we can also mark the non-namespaced targets as DEPRECATED targets, if the used CMake version is recent enough: https://cmake.org/cmake/help/v3.17/release/3.17.html#properties .

@Nicogene Nicogene force-pushed the feat/relocableCMakeConfigFiles branch from 48c1b3a to 76836d7 Compare April 16, 2020 07:55
@CLAassistant
Copy link

CLAassistant commented Apr 20, 2020

CLA assistant check
All committers have signed the CLA.

@Nicogene
Copy link
Member Author

In the latter two commits I made some changes in order to handle the libraries through the COMPONENTS, as it is done in YARP.

Here is the default components:

                                 iCubDev
                                 ctrlLib
                                 skinDynLib
                                 iKin
                                 iDyn
                                 icubmod

I managed to create a ICUBConfig.cmake containing the targets and ICUB_LIBRARIES.

It is super draft, probably there are some copy-paste errors, or some missing parts.

What do you think about it ?

@drdanz
Copy link
Member

drdanz commented Jun 19, 2020

What about installing iKin, iDyn and any other libraries that could eventually be detached from the iCub-main repository one day, as separate package, and have iCubConfig.cmake find them using find_dependency? In this way, you can start searching them either with find_package(ICUB) or with find_package(iKin)`, and if they are moved to a separate repository the compatibility will not be broken

@Nicogene
Copy link
Member Author

What about installing iKin, iDyn and any other libraries that could eventually be detached from the iCub-main repository one day, as separate package, and have iCubConfig.cmake find them using find_dependency? In this way, you can start searching them either with find_package(ICUB) or with find_package(iKin)`, and if they are moved to a separate repository the compatibility will not be broken

In theory porting it to the COMPONENTS should do the trick I am right?

@Nicogene Nicogene force-pushed the feat/relocableCMakeConfigFiles branch 2 times, most recently from dc28d73 to bb81f9a Compare June 22, 2020 15:00
@Nicogene
Copy link
Member Author

Nicogene commented Jun 23, 2020

FYI I have updated the body of the PR with the latest changes-> #652 (comment)

At this moment I have the issue that the link to <lib> is not working, while ICUB::<lib> is working.

@Nicogene Nicogene force-pushed the feat/relocableCMakeConfigFiles branch 2 times, most recently from c9da3fd to b063775 Compare June 23, 2020 12:27
@Nicogene Nicogene marked this pull request as ready for review June 23, 2020 12:27
@Nicogene
Copy link
Member Author

Nicogene commented Jun 23, 2020

The PR is ready to be review/merged, I successfully compiled the robotology-superbuild with almost all profiles enabled.

These changes are fully back-compatible

@Nicogene Nicogene requested a review from traversaro June 23, 2020 12:30
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@pattacini pattacini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

Only a quick request for clarification from my side.

src/libraries/actionPrimitives/CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments.

@Nicogene Nicogene force-pushed the feat/relocableCMakeConfigFiles branch from b063775 to 7035200 Compare June 23, 2020 14:40
@Nicogene Nicogene requested a review from traversaro June 23, 2020 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants