Enable basic warnings + include correct headers + remove Travis CI

[audrow]

This PR is a continuation of https://github.com/ros2/urdfdom/pull/22 and enables -Wall, -Wextra, -Wpedantic. It should also fix #147.

[audrow]

• Linux
• Linux-aarch64
• macOS
• Windows

[scpeters]

the travis-ci build can't find tinyxml_vendor

[clalancette]

the travis-ci build can't find tinyxml_vendor

Yeah, travis isn't going to work on the ros2 branch because of this. My suggestion is to just remove travis for this branch; we'll get coverage via the Rpr job anyway.

[scpeters]

My suggestion is to just remove travis for this branch

👍

[traversaro]

the travis-ci build can't find tinyxml_vendor

Yeah, travis isn't going to work on the ros2 branch because of this. My suggestion is to just remove travis for this branch; we'll get coverage via the Rpr job anyway.

What is the strategy regarding the GitHub releases for this repo? If future 2.* release of this repo will be tagged/released on the ros2 branch, then depending on "ROS2 as a software distribution" details such as looking tinyxml_vendor and console_bridge_vendor will mean that it will not be possible to package this repo for vcpkg, conda-forge or any "normal" Linux distribution (see https://repology.org/project/urdfdom/versions). Indeed, actually conda-forge already contains a patch to remove exactly those lines: https://github.com/conda-forge/urdfdom-feedstock/blob/master/recipe/remove_vendor.patch .

cc @wolfv @Tobias-Fischer

[clalancette]

What is the strategy regarding the GitHub releases for this repo?

It's a good question, and one that I've been pondering as well. In short, I don't know. The original goal here was to bring https://github.com/ros2/urdfdom back into this repository so we can have less divergence.

As it stands, any PR against the ros2 branch here is going to fail travis because of those extra vendor calls. On the other hand, we need to have the find_package(*_vendor) lines for this to work in the ROS 2 distributions.

Two ways to resolve this conflict that come to mind:

1. Do what we are currently doing, and make downstreams who want to consume this package patch out those lines. It's not really nice to the downstreams, but it is a simple patch.
2. Remove the find_package(*_vendor) here, and switch urdfdom to be itself vendored in the ROS 2 distributions. Then the ROS 2 distributions would have a urdfdom_vendor CMakeLists.txt, which could have the find_package(*_vendor) package in them, which I think would do the same thing.

There may be other ways to handle it that I'm not thinking of.

Regardless, I don't think this should hold up this particular PR. My suggestion to move forward here (and with #146) is to remove travis for now. Once we decide on a strategy going forward, we can reenable travis if we want.

[wolfv]

you could also have a cmake option 'USE_ROS_VENDOR'?

[clalancette]

you could also have a cmake option 'USE_ROS_VENDOR'?

Yeah, that's another way to do it. In order to play nicely in the ROS 2 builds, it would have to be enabled by default, and then downstreams could do -DUSE_ROS_VENDOR=OFF.

[traversaro]

The USE_ROS_VENDOR seems to be a nice compromise, and it could be set to OFF in the Travis build to ensure that we don't break the non-ROS2 build.

[traversaro]

fyi I guess that @j-rivero as the Debian urdfdom maintainer could be interested in this discussion.

[traversaro]

Then the ROS 2 distributions would have a urdfdom_vendor CMakeLists.txt, which could have the find_package(*_vendor) package in them, which I think would do the same thing.

While I think USE_ROS_VENDOR CMake option is by far the best compromise, but just to clarify: I do not think it is the same thing to start adding distribution-specific change to a new version of package (such as urdfdom) that is already packaged in many existing and used package managers, as opposed to add distribution-specific changes to packages (such as the vast majority of packages in ROS2) that are not not packaged in almost any package manager.

[clalancette]

While I think USE_ROS_VENDOR CMake option is by far the best compromise, but just to clarify: I do not think it is the same thing to start adding distribution-specific change to a new version of package (such as urdfdom) that is already packaged in many existing and used package managers, as opposed to add distribution-specific changes to packages (such as the vast majority of packages in ROS2) that are not not packaged in almost any package manager.

Just to be clear; this situation already exists today. The only difference is that it is now more visible since I merged the ros2 code into here.

And to be further clear; if we went the route of having a urdfdom_vendor package in ROS 2, then we wouldn't have any calls to find_package(console_vendor) or find_package(tinyxml_vendor) in here at all. At that point, we could consider deprecating the ros2 branch in this repository completely and always just using master. Then downstreams would not have to do anything at all to use the sources; they would just build outside of a ROS environment.

I personally like that idea the best, though I'm not yet 100% convinced it will work in practice.

[traversaro]

Just to be clear; this situation already exists today. The only difference is that it is now more visible since I merged the ros2 code into here.

I totally agree, but to clarify what I meant: as a downstream packager of urdfdom, from my point of view the ros2/urdfdom fork or the ros2 branches are fork/branches like any other, so the problematic situation does not exists yet. The problem will arise as soon as a new release will be tagged on this repo: at that point, if the release is tagged from the ros2 branch, then the problem will "exists" . Until then, indeed there is no problem at all.

[traversaro]

Actually I just noticed that tags such as https://github.com/ros/urdfdom/tree/2.3.3 are now present in the repo, so I would say that the problem emerged as those tags were added to this repo.

[clalancette]

@sloretz had another idea, which is to just switch those calls to find_package(console_bridge_vendor) to find_package(console_bridge_vendor QUIET). That way, we use the vendored library in the places we want to (ROS 2 builds), but it still picks up the system version if that is not available. @traversaro @wolfv what do you think about that?

[traversaro]

@sloretz had another idea, which is to just switch those calls to find_package(console_bridge_vendor) to find_package(console_bridge_vendor QUIET). That way, we use the vendored library in the places we want to (ROS 2 builds), but it still picks up the system version if that is not available. @traversaro @wolfv what do you think about that?

Both this and the USE_ROS_VENDOR solution are fine for me, with this one I am a bit afraid that on the ROS2 side it would be more complicate to detect if for some reason console_bridge_vendor or tinyxml_vendor are not installed, as you would not get a clean error, but clearly that is a trade-off that is up to the maintainers evaluate.

[clalancette]

To move forward with this PR, my suggestion is to just remove .travis.yml for now. Once this and #146 are in, I'll do a follow-up to make the vendor packages essentially optional, and restore travis at that time.

@audrow Can you remove travis.yml, and respond to @scpeters review here?

[audrow]

@clalancette, will do.

[audrow]

I ran tests for urdf as well as urdfdom, because I wanted to make sure that our changes to the CMakeLists file didn't break packages using urdfdom.

• Linux
• Linux-aarch64
• macOS
• Windows

[audrow]

@clalancette or @scpeters, could you merge this PR in? I don't have permissions to.

Also, I believe that ros2/urdfdom can be archived now.

[clalancette]

Woohoo, green CI! I'll wait for final approval from @scpeters before merging.