Std::shared_ptr in the geometric_shapes public API

Packaging and Release Management Kinetic
1

Recently, FCL 0.5 was released which is compatible with octomap 1.8. MoveIt! switched to FCL 0.5 for kinetic. However, FCL 0.5 also switched to std::shared_ptr instead of boost::shared_ptr, while geometric_shapes::OcTree was still using boost::shared_ptr. Converting between the two perfectly is not possible. Basically, there were three options:

  1. Switch geometric_shapes and moveit to std::shared_ptr for compatibility with FCL 0.5. This violates REP 003 by using C++11 features in the public API and requires all packages using geometric_shapes to enable C++11 support. The advantage here is that if something breaks the compiler will complain loud and clear.

  2. Copy the underlying object into a new std::shared_ptr. This is potentially expensive (OcTrees can be big) and means changes to the original OcTree are not reflected in the copy held by FCL. Personally I wouldn’t consider this as a viable alternative, but for completeness it is listed anyway.

  3. Create a std::shared_ptr managing the same boost::shared_ptr with a special deleter. This breaks the semantics of the accompanying weak_ptr subtly. The advantage here is that only code actually using FCL and Octomaps needs to be updated, but the disadvantage is that if a broken weak_ptr causes problems, there is no compiler diagnostic and the problem will be very difficult to track down.

Option 1 was actually done, but the discussion does not seem over. So the first question is, was this decision right or should we go for another option?

A second question if we stick with option 1: do we export the -std=c++11 flags from geometric_shapes or not?

Exporting the flag has the advantage that other packages not using octomap will probably just work ™. A disadvantage is that compiling with C++11 can introduce subtle errors when linking with non-C++11 code, and it complicates matters for people who want to use a different compatible standard (-std=gnu++11, c++14 or gnu++14 for example). It would also mean that multiple packages might export conflicting standards and suddenly the order of declaring your dependencies becomes important. Personally I would prefer not exporting the flag, even if we stick with option 1.

2

Also, there are apparently issues when linking object files using boost::variant if some are compiled with C++11 and some are not. I’m not sure what causes this (maybe someone else can elaborate?) but it means that removing C++11 from the public API might not be enough to support packages not using C++11.

3

Thanks for the post.

MoveIt moved to C++11 and we are not going to revert this.
If we can simplify a header file (e.g. using delegation constructors ), I don’t see a reason why we should not do that.

The issue we discussed is that OSRF specified all packages in desktop_full will not export a C++11 dependency.
MoveIt is not in there. geometric_shapes on the other hand is.

So the problems are that geometric_shapes currently exports a std::shared_ptr as generated by octomap 1.8 and uses boost::variant for the ShapeMsg type.
The first obviously requires C++11 to compile, the latter has different implementations in C++98 and C++11 and thus variants generated in c++98-compiled code cannot be handed over to code in libgeometric_shapes.so - We will require C++11 to compile the library in any case, as we will have to deal with octomap’s std::shared_ptr.

The question is:
Should we go for a totally hacky and somewhat (nobody knows how much) broken way out to remove the std::shared_ptr from the API and to replace the variant (the second is perfectly possible now that I looked at it)
or
Should we expose the C++11 header dependency?

In the latter case:
Do we force all projects directly and indirectly to explicitly compile with C++11 (this is what gazebo did and it turned out to be quite a number of places where the flag had to be added)
or
do we implicitly set it on find_package(geometric_shapes)?

I proposed to go for the second option in both cases.
You argued that if another module requires C++14 in their public interface (osrf still proposes to follow their example and only expose C++98 compatible code) and also implicitly specifies the standard then it would depend on the order of find_package directives in the CMakeLists.txt whether everything works out or fails because the other module’s headers are compiled with geometric_shape's C++11.

In either case the user can always specify a standard on tis own by adding add_compile_options(-std=xyz) in their CMakeLists.txt after everything has been find_packaged.

As the implicit -std=c++11 can be removed once osrf enforces gcc6 (defaults to a c++11 compatible standard),
I do not see your point.

4

This is specified by REP 003. People from OSRF proposed it, but everyone had a chance to give input and this point in particular was discussed at length (https://github.com/ros-infrastructure/rep/pull/106). Also it can be changed if need be. I think it’s a subtle but important distinction to say “the REP specifies …” rather than “the OSRF specifies …”.

Sorry, I know this is off topic, but it was bothering me :slight_smile:

I won’t rehash the argument here, as you guys discussed it in the issue that started this thread and @de-vri-es and I discussed it previously as well here:

I ended up agreeing with him that exporting the c++11 setting was not a great idea because of how it interacts with gcc6 which is c++14 by default and when it conflicts with a package that wants to use something different, like c++14 or the c++17 preview. The most compelling example of how this goes wrong is if the compiler, e.g. gcc6, choose c++14 by default, then this exported flag will now be forcing the compiler to an older standard. If there were a way to say -std>=c++11, then that would be ok, but I don’t believe there is.

Perhaps a compromise would be to use CMake to check if c++11 is enabled by default by trying to compile a file that uses it and pass -std=c++11 only if it is not. See: CheckCXXSourceCompiles — CMake 3.0.2 Documentation

If only we had made it to CMake 3.1.0 as a minimum requirement we could use the “compiler features” mechanism to detect if the compiler being used supports C++11 by default: c++ - How to detect C++11 support of a compiler with CMake - Stack Overflow

5

For others reference, this thread is a follow up to the discussion on this pull request:

It looks like I doubled with @wjwwood in our replies but this is similar but mostly new.

Although we drive the processof determining the REP targets at OSRF it’s an open community process that’s well circulated to the mailing lists and is open for contributions. Personally I’d love to blaze ahead and jump forward to the new standards. And many others here at OSRF also would love to do that too. However, we know that many in the community value stability and our goal is to best represent what we believe is best for the whole community.

If you review the discussion about how to pick the target platforms we considered many different options. There are good cases for both conservative and aggressive approaches however we eventually converged on the currently defined compromise.

The compromise selected allows many of the new features of the standard to be utilized. But prevents public API changes to the new standard for core packages so as not to make one packages changes force changes to other packages.

Part of being a core package in the ROS ecosystem is understanding that a lot of other developers depend on your APIs and any API change made at the core ripples across all the dependencies. There are many thousands of packages that depend on the API, and if you change the API in a non-backwards compatible manner it will force thousands of maintainers to respond immediately. We picked desktop-full as the target since it’s what most people consider the core packages relied upon by most of the community and thus are the packages we work really hard to remain stable.

What was particularly valuable about this compromise is that by not exposing c++11 in the headers we prevented a single packages from forcing all downstream packages from requiring a patch to their CMakeLists.txt before releasing. This is especially valuable as we know that this forced patch will no longer be needed once gcc6 is standard, and the changes to the CMakeLists.txt will no longer be needed. And as @wjwwood and @de-vri-es have pointed out if one package exports a compiler standard it may interfere with other packages choices of compiler.

When coordinating a large community it’s always true that some parts of the community will adopt new standards sooner than others. In this case we can consider making an exception to the rule and allow geometric_shapes to use c++11 in the headers. But we need to build community consensus since it will effect a large fraction of the community. The reason I suggested moving to this forum is to get a larger community discussion going and make sure that we’re understanding all the ramifications.

6

I think its relevant to point out as a maintainer of geometric_shapes that perhaps we are over-debating a package that happens to be in desktop-full. I do not think it is used by many packages beyond moveit (and the moveit maintainers want the C++11 support). In indigo it is used by:

cob_obstacle_distanc…
cob_pick_place_actio…
collada_urdf
collada_urdf_jsk_pat…
romeo_moveit_actions…

but the proposed change is just for kinetic which is only currently being used by collada_urdf - a simple tool that converts model formats. So perhaps it won’t affect much in kinetic to make the change.

7

It looks like there is no real opposition to using std::shared_ptr in the geometric_shapes API. If that is true (correct me if I’m jumping the gun here) the only question remaining is whether or not to export the flags from geometric_shapes.

As stated before, I would prefer not, since it breaks our current code base and forces us to change the order of our dependencies in the CMakeLists.txt (which are currently sorted by alphabet, so that would make me sad) or overwrite the C++ standard again in all of our packages after find_packageing the dependencies. If not aware of the issue, it is confusing to see that your own compiler flags are overwritten by one of your dependencies.

When all is said and done, it is true that we can work around it. I still prefer nobody touches the flag, but it’s not the end of the world. Just keep in mind that geometric_shapes exporting the flag doesn’t mean other packages are automatically all fine, because we have packages that would have to be changed to continue to compile. I know C++14 isn’t officially supported by ROS, but it works fine without any trouble as long as nobody touches the -std flag. It is only a minor update compared to C++11 after all.

8

Thanks @de-vri-es, @tfoote and @v4hn for thinking through this. Personally I’m not sure what the right solution is, but there are a few things I’d like to understand/bring up.

@davetcoleman I did a similar check for dependencies and came to the same conclusion you did. It looks like collada_urdf is the only released package that depends on geometric_shapes. I’m not sure how many people have non-released packages that use geometric_shapes though. I’m guessing it’s not a huge number, but probably non-zero.

@v4hn you’ve pointed out that using c++11 features (shared_ptr and weak_ptr) internally in geometric shapes while only exposing boost types in the API of geometric_shapes would be hacky. I admit I don’t have a great understanding of the internals of the standard library - could you summarize the difficulty there? I’m assuming that along the same lines, if geometric_shapes uses c++11 types then collada_urdf will also need to be converted to use c++11 types internally to avoid hackiness. So it sounds like if we commit to turning on c++11 for the geometric_shapes API, we should also commit to updating the internals of collada_urdf? In particular the boost::scoped_ptr here: https://github.com/ros/robot_model/blob/a7c9e1b071018a66bf16b3f220131b907409e358/collada_urdf/src/collada_urdf.cpp#L1285

I also have realized there are more complications than I realized in linking C++11 and non-C++11 compiled libraries. The boost variant incompatibility was a surprise to me, and it worries me that I have no idea what other incompatibilities may exist. My guess is that others have a much better understanding of this - are there other incompatibilities that are going to bite us in the future? Perhaps REP 0003 should have a line that says “and try to avoid using pre-c++11 features in APIs that have link-incompatibilities with c++11 compiled code, include boost::variant, other thing, other thing”

On a more general note, I’d like to point out that whatever decision is made here, we’ll be stuck with it for a while. Like many people (I assume) I try to only use Ubuntu LTS versions on my robots. That means that I’m going to be using ubuntu 16.04 for about the next two years. The default compiler in 16.04 does not turn on C++11 flags, so anything built with C++11 that I’m going to run on one of my robots in the next couple of years will have to play nice with lots of system dependencies that are compiled without the C++11 flag.

If it is decided that turning on C++11 for compilation for geometric_shapes and all of its dependencies is the right way to go, I would personally be in favor of forcing it to be done explicitly in each package. In fact It would be awesome to add a pre-processor directive in the geometric_shapes header that cheks for c++11 being enabled and, if not enabled, causes the compile to fail with an error like “It looks like you are trying to use geometric_shapes without C++11 enabled. See LINK for a discussion of why you need to enable it, and how to do so”. Is that possible?

9

I’m also in favor of adding a check for std >= 11 in findGeometricShapes.cmake if possible

Maybe it would also be possible to get rid of the boost::variant altogether?

10

Ok,

first of all I apologize for implying that REP003 was a decision by OSRF.
Although these processes sometimes feel like “But the plans were on display…”,
everyone can influence them. Actually, I didn’t intend to imply anything different.
Doesn’t have anything to do with this discussion though.

Concerning shared_ptr conversion:
The answer by Fozi to http://stackoverflow.com/questions/6326757/conversion-from-boostshared-ptr-to-stdshared-ptr presents a way to
piggy-back an std::shared_ptr on top of the boost-equivalent and the other way around by implementing a functor object as the custom destructor
of the new pointer and add the old shared_ptr as a member to that object. It looks ugly and obfuscates the code, but it should work without limitations.
This would not be part of geometric_shapes but part of the MoveIt code that receives geometric_shape's boost::shared_ptr<octomap> and requires std::shared_ptr to hand it to fcl.

Removing the variant would require the removal of the visitor patterns in geometric_shapes/src/shape_operations.cpp and
to spell out the different implementations of the functions. Not really beautiful, but neither is (imho) the original code.

After reading up on the comments, I discussed this yesterday with a fellow FOSS distribution maintainer.
Requiring a language standard is a fundamental issue in working with external dependencies and there is no perfect solution.
There are a number of options one can find across projects to handle such an issue:

1a. don’t do anything at all and imply that the project requires at least C++11

1b. add a preprocessor test for at least C++11 and add an error message “This header requires at least C++11”

  1. add -std=c++11 to the variable ${_CXX_FLAGS} that is usually exported by cmake configs and pkg-config descriptions.

  2. add the flag to the CMAKE_CXX_FLAGS or to the directory properties via add_compile_options

  3. add interface compile flags to the geometric_shapes library so that anything that links the geometric_shapes automagically compiles with the flag

The only options that don’t have the problem to enforce C++11 when the compiler would default to C++14 are 1* and it would be a valid stance to take for geometric_shapes.
The reason why this is somewhat problematic is because geometric_shapes was already released in kinetic without having considered the interface required by MoveIt.
As a result we already have software released that depends on the non-c++11 interface. To avoid breakage it might have been a solution to implicitly specify the flag.
(Turns out it isn’t a solution either way.)

However, as @davetcoleman pointed out above, there is only one dependent package released in kinetic up to now: collada_urdf.
By default, this does not compile with c++11 and that sparked the whole discussion.
Also, who would have guessed…, collada_urdf requires a different c++11 compatible standard: gnu++11.

I’ve just filed https://github.com/ros/robot_model/pull/142 to move collada_urdf to compile with gnu++11.
Once this is accepted and released, it should be no problem to go for 1b.
As far as I can see, this is the most polite and universal solution.
I added a pull-request for it to geometric_shapes here.

Thanks for the input.

11

1b sounds good to me :slight_smile:

Some additional information for completeness: adapting a boost::shared_ptr or std::shared_ptr to the other type with a custom deleter works for the shared_ptrs themselves, but will always break semantics (without compiler diagnostic) of at least one of the matching weak_ptr types. A more detailed discussion of that is available here: http://stackoverflow.com/a/12315035/636974 (it also has a slightly more readable solution that basically does the same but with a lambda). Be sure to read the hidden comments too for the full story regarding weak_ptrs, if interested.

12

I’ve just pinged @jacquelinekay for the robot_model PR so hopefully we can move forward with the MoveIt! kinetic release