Plain cmake

[jontje]

Draft PR that turns the library into a plain cmake package, and it aims to solve issue #3.

The main reason for this is to be able to build and use the library regardless if e.g.:

• Ubuntu or Windows
• ROS or ROS2

[gavanderhoorn]

See my ros-industrial/abb_libegm#63 (comment).

The same applies here of course.

[jontje]

@gavanderhoorn, I assume that similar commits concerning travis from ros-industrial/abb_libegm#63 should be applied here as well, correct?

[gavanderhoorn]

Correct.

[jontje]

It seems that I made a wrong assumption in commit 5a25561.

I.e. regarding find_package(Poco ...) I wanted to remove the custom cmake module for finding the package, since it looks like there are cmake config files per default in Ubuntu 18.04 (as far as I can see) and I wanted to use those instead.

@gavanderhoorn, is it possible to change the setup_poco_ppa.sh script to install the Poco libraries from Bionic instead? Do you know if it would cause any issues?

[jontje]

I have created a new branch with some additions that I think would clean-up the CMake files, as well as updated the Poco installation script. I didn't add it to this PR yet, since I have been waiting for some comments, but I think this will have to wait until next year.

[gavanderhoorn]

This one is a little trickier.

The base CMake changes look OK, but the PPA situation requires some more attention.

It seems that I made a wrong assumption in commit 5a25561.

As a high-level comment: try to make commits smaller. Reverting just the find_package(Poco ..) removal now requires manual changes instead of simply reverting a commit.

I.e. regarding find_package(Poco ...) I wanted to remove the custom cmake module for finding the package, since it looks like there are cmake config files per default in Ubuntu 18.04 (as far as I can see) and I wanted to use those instead.

@gavanderhoorn, is it possible to change the setup_poco_ppa.sh script to install the Poco libraries from Bionic instead? Do you know if it would cause any issues?

No, we should not do that. Poco is used by many projects and replacing a system-matched version with one from another distribution is not a good idea (ABI compatibility etc).

If we just want to use the find script, we could always vendor it into abb_librws (read: copy it into the cmake sub dir and note the license and provenance somewhere).

You're primarily interested in that find script because of the support for the version specificer, correct?

I have created a new branch with some additions that I think would clean-up the CMake files, as well as updated the Poco installation script. I didn't add it to this PR yet, since I have been waiting for some comments, but I think this will have to wait until next year.

There are some interesting changes there, but we'll deal with those in a follow-up PR after we merge this one.

Another high-level comment: leave the check for whether CI is running a Xenial job out of the PPA setup script. It's not the place for it.

[jontje]

Thanks for the comments @gavanderhoorn.

As a high-level comment: try to make commits smaller. Reverting just the find_package(Poco ..) removal now requires manual changes instead of simply reverting a commit.

I need to "burn my hand" a few times before I actually learn 😉

No, we should not do that. Poco is used by many projects and replacing a system-matched version with one from another distribution is not a good idea (ABI compatibility etc).

Ah, ok, I see.

If we just want to use the find script, we could always vendor it into abb_librws (read: copy it into the cmake sub dir and note the license and provenance somewhere).

You're primarily interested in that find script because of the support for the version specificer, correct?

My main reason was to remove the dependency on the catkin package cmake_modules's find script, and I guess potential vendors would be either poco or cmake_modules. And this should also mean that the PPA script change I drafted wouldn't be necessary.

There are some interesting changes there, but we'll deal with those in a follow-up PR after we merge this one.

Yes, that would be good. I also have a few pending code changes I would like merge before a follow-up PR.

[gavanderhoorn]

@jontje: if we're copying a FindPoco script, could we not take one which is a bit more compatible with "modern CMake" (ie: defines INTERFACE libraries)?

[jontje]

@jontje: if we're copying a FindPoco script, could we not take one which is a bit more compatible with "modern CMake" (ie: defines INTERFACE libraries)?

I opted for the least effort approach (at least for me), since I knew where to find this script, and I see this as a more or less temporary solution.

But if you know of a nicer script, then we can take that one instead.

[gavanderhoorn]

But if you know of a nicer script, then we can take that one instead.

One from a newer version of CMake? :)

IIRC, @traversaro has mentioned a few times that newer versions export INTERFACE libraries and the appropriate targets.

@traversaro: would you happen to know starting from which version things normalise wrt FindProtobuf?

[traversaro]

IIRC, @traversaro has mentioned a few times that newer versions export INTERFACE libraries and the appropriate targets.

Are we discussing about FindPoco or FindProtobuf ?

@traversaro: would you happen to know starting from which version things normalise wrt FindProtobuf?

The first CMake version that provides protobuf imported targets is CMake 3.9 https://cmake.org/cmake/help/v3.9/module/FindProtobuf.html . Note that however if you link imported targets as INTERFACE or PUBLIC in your public targets, then you need to make sure that your <package>Config.cmake files creates them, tipically by calling find_dependency(Protobuf REQUIRED), but if you are on a system that ships CMake 3.5 you will need to write custom code to make sure that in your <package>Config.cmake you use the correct target-aware FindProtobuf.cmake .

[gavanderhoorn]

Hm. I may have mixed Protobuf and Poco.

Thanks for the info in any case @traversaro.

[jontje]

@gavanderhoorn, I assume that the current solution for finding the Poco package will suffice for now due to the latest comments. I looked around a bit for a Poco script in a newer version of CMake, but I didn't find anything.

[gavanderhoorn]

I've added 33c10fb as we weren't installing the manifest, which makes this package 'invisible' after installation (in a ROS context).

[gavanderhoorn]

One minor comment in-line, other than that: 👍

Apologies for leaving this for so long @jontje :(

[jontje]

Apologies for leaving this for so long @jontje :(

No worries @gavanderhoorn! I know you are busy, and I have had plenty of other tasks to spend my time on 😄

[jontje]

Thanks a lot both @gavanderhoorn and @traversaro!

[gavanderhoorn]

@jontje: you'll also update the readme to document the migration to plain cmake like in abb_libegm?

[jontje]

@jontje: you'll also update the readme to document the migration to plain cmake like in abb_libegm?

Yes, I have already prepared a branch for it.

[gavanderhoorn]

Good :)

If you could prepare a quick PR we can get it merged in quickly.