Enable basic warnings + include correct headers + remove Travis CI #148

audrow · 2020-10-27T19:15:09Z

This PR is a continuation of ros2#22 and enables -Wall, -Wextra, -Wpedantic. It should also fix #147.

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

This reverts commit 9a4d55d.

This reverts commit 542110f.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

audrow · 2020-10-27T21:18:14Z

Linux
Linux-aarch64
macOS
Windows

clalancette

This works for me, but I'd like @scpeters and/or @sloretz take a look at it before we merge.

scpeters · 2020-10-28T17:46:28Z

urdf_parser/CMakeLists.txt

 add_library(urdfdom_world SHARED
  src/pose.cpp
  src/model.cpp
  src/link.cpp
  src/joint.cpp
  src/world.cpp)
-target_include_directories(urdfdom_world PUBLIC
+target_include_directories(urdfdom_world SYSTEM PUBLIC


this SYSTEM keyword is applied not only to TinyXML_INCLUDE_DIRS, but also to $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include> and $<INSTALL_INTERFACE:include>. is that ok? would it be better to split into separate calls to target_include_directories, one with SYSTEM and one without?

I don't know enough about how this works

this SYSTEM keyword is applied not only to TinyXML_INCLUDE_DIRS, but also to $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include> and $<INSTALL_INTERFACE:include>. is that ok? would it be better to split into separate calls to target_include_directories, one with SYSTEM and one without?

I think that would work. @audrow can you give that a shot?

scpeters · 2020-10-28T17:47:53Z

the travis-ci build can't find tinyxml_vendor

clalancette · 2020-10-28T18:01:13Z

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 · 2020-10-28T18:14:43Z

My suggestion is to just remove travis for this branch

👍

traversaro · 2020-10-29T14:16:04Z

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 · 2020-10-29T15:07:03Z

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:

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.
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 · 2020-10-29T15:08:36Z

you could also have a cmake option 'USE_ROS_VENDOR'?

clalancette · 2020-10-29T15:11:28Z

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 · 2020-10-29T15:22:28Z

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 · 2020-10-29T15:25:23Z

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

traversaro · 2020-10-29T15:30:28Z

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 · 2020-10-29T15:47:34Z

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 · 2020-10-29T15:54:33Z

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 · 2020-10-29T15:57:03Z

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 · 2020-10-29T17:23:30Z

@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 · 2020-10-29T20:57:29Z

@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 · 2020-11-02T13:16:21Z

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 · 2020-11-02T17:53:44Z

@clalancette, will do.

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

clalancette

Looks good with green CI.

audrow · 2020-11-03T19:31:07Z

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 · 2020-11-03T19:36:41Z

@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 · 2020-11-03T20:26:43Z

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

audrow and others added 7 commits October 15, 2020 11:56

Enable basic warnings

7da6841

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

Handle input arguments and return in main function

15fea17

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

Set default C compiler

26ab3ee

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

Add extern "C" around console_bridge/console.h include

c2c5b67

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

Revert "Add extern "C" around console_bridge/console.h include"

c0501fd

This reverts commit 9a4d55d.

Revert "Set default C compiler"

c67038a

This reverts commit 542110f.

Use SYSTEM for the include directories in urdfdom.

40188c7

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

audrow mentioned this pull request Oct 27, 2020

Enable basic warnings ros2/urdfdom#22

Closed

audrow changed the title ~~Enable basic warnings~~ Enable basic warnings + include correct headers Oct 27, 2020

Add comment describing SYSTEM keyword

2ec2654

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

clalancette approved these changes Oct 27, 2020

View reviewed changes

scpeters reviewed Oct 28, 2020

View reviewed changes

audrow added 2 commits November 3, 2020 09:23

Remove travis.ci yaml and build script

701cf22

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

Only use SYSTEM with TinyXML includes

8e89a0c

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

clalancette approved these changes Nov 3, 2020

View reviewed changes

audrow changed the title ~~Enable basic warnings + include correct headers~~ Enable basic warnings + include correct headers + remove Travis CI Nov 3, 2020

audrow requested a review from scpeters November 3, 2020 22:23

scpeters approved these changes Nov 4, 2020

View reviewed changes

scpeters merged commit 09462aa into ros:ros2 Nov 4, 2020

clalancette mentioned this pull request Nov 4, 2020

Merge the master branch onto the ros2 branch #146

Closed

clalancette deleted the audrow/enable-warnings-urdfdom branch November 5, 2020 16:56

sloretz mentioned this pull request Dec 29, 2020

Using wrong headers #147

Closed

traversaro mentioned this pull request Jun 16, 2021

Conflicting version requirements urdfdom RoboStack/ros-noetic#138

Closed

traversaro mentioned this pull request May 5, 2023

Build Status link in README.md is broken #182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable basic warnings + include correct headers + remove Travis CI #148

Enable basic warnings + include correct headers + remove Travis CI #148

audrow commented Oct 27, 2020

audrow commented Oct 27, 2020

clalancette left a comment

scpeters Oct 28, 2020

clalancette Oct 28, 2020

scpeters commented Oct 28, 2020

clalancette commented Oct 28, 2020

scpeters commented Oct 28, 2020

traversaro commented Oct 29, 2020

clalancette commented Oct 29, 2020

wolfv commented Oct 29, 2020

clalancette commented Oct 29, 2020

traversaro commented Oct 29, 2020 •

edited

Loading

traversaro commented Oct 29, 2020

traversaro commented Oct 29, 2020

clalancette commented Oct 29, 2020

traversaro commented Oct 29, 2020 •

edited

Loading

traversaro commented Oct 29, 2020

clalancette commented Oct 29, 2020

traversaro commented Oct 29, 2020

clalancette commented Nov 2, 2020

audrow commented Nov 2, 2020

clalancette left a comment

audrow commented Nov 3, 2020

audrow commented Nov 3, 2020

clalancette commented Nov 3, 2020

Enable basic warnings + include correct headers + remove Travis CI #148

Enable basic warnings + include correct headers + remove Travis CI #148

Conversation

audrow commented Oct 27, 2020

audrow commented Oct 27, 2020

clalancette left a comment

Choose a reason for hiding this comment

scpeters Oct 28, 2020

Choose a reason for hiding this comment

clalancette Oct 28, 2020

Choose a reason for hiding this comment

scpeters commented Oct 28, 2020

clalancette commented Oct 28, 2020

scpeters commented Oct 28, 2020

traversaro commented Oct 29, 2020

clalancette commented Oct 29, 2020

wolfv commented Oct 29, 2020

clalancette commented Oct 29, 2020

traversaro commented Oct 29, 2020 • edited Loading

traversaro commented Oct 29, 2020

traversaro commented Oct 29, 2020

clalancette commented Oct 29, 2020

traversaro commented Oct 29, 2020 • edited Loading

traversaro commented Oct 29, 2020

clalancette commented Oct 29, 2020

traversaro commented Oct 29, 2020

clalancette commented Nov 2, 2020

audrow commented Nov 2, 2020

clalancette left a comment

Choose a reason for hiding this comment

audrow commented Nov 3, 2020

audrow commented Nov 3, 2020

clalancette commented Nov 3, 2020

traversaro commented Oct 29, 2020 •

edited

Loading

traversaro commented Oct 29, 2020 •

edited

Loading