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

Update to REP-0003 for ROS Kinetic Kame #106

Merged
merged 1 commit into from
Feb 26, 2016
Merged

Update to REP-0003 for ROS Kinetic Kame #106

merged 1 commit into from
Feb 26, 2016

Conversation

esteve
Copy link
Contributor

@esteve esteve commented Sep 21, 2015

I've updated the requirements for Kinetic Kame. @scpeters has expressed interest in ROS having support for C++11 for better compatibility with newer versions of SDFormat. I don't know if the version of Gazebo should be 5 or 6, and the list of C++11 features will be expanded as we see if they can benefit our users (e.g. lambdas for subscriptions, std::shared_ptr instead of boost's)


- Ubuntu Trusty (14.04)
- Ubuntu Vivid (15.04)
- Ubuntu Wily (15.10)
Copy link
Member

Choose a reason for hiding this comment

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

k-turtle will not target trusty and vivid but 16.04 according to http://wiki.ros.org/Distributions/ReleasePolicy

@vrabaud
Copy link
Contributor

vrabaud commented Oct 13, 2015

I will send an email to the list soonish but I would suggest OpenCV 3 for Kinetic. Why ?

  • OpenCV 3 got released in June
  • I got 10 / 20 emails of ROS people already using it
  • it has a lot of optimizations
  • but mostly a lot of cool stuff for robotics: RGBD, solid structure from motion, much better calibration, deep learning for object recognition ... and ROS needs all that

Now, OpenCV3 has not even reached upstream Debian yet and that means we (I) 'll have to release it for Kinetic. I already bloomed it for Indigo and Jade.

It is a big change but not so big: it is fairly easy to write code that compile on OpenCV 2 and 3 (and for 5% of the API that is different, you can have an ifdef). I already did it with all the low level vision (vision_opencv, image_common and image_pipeline).

Except if there is a good reason for a definite yes / no, we should probably continue this conversation on the mailing lists (just give me a week or two if possible) as we'll start getting into technical things like why, or how to put it on OSX and such.

I just wanted to give a heads-up, thx.

@wjwwood
Copy link
Contributor

wjwwood commented Oct 13, 2015

@vrabaud will it not get into Ubuntu X? Wily isn't even officially released yet, so there is time to get it at least into Ubuntu X and then we can host a back port for Wily as long as it is supported.

@vrabaud
Copy link
Contributor

vrabaud commented Oct 13, 2015

@wjwwood : it is a possibility indeed that we'll talk about. Joschen already offered his help: we'll see what the easiest is.

@j-rivero
Copy link
Contributor

@vrabaud @wjwwood: the release cycle for Ubuntu X hasn't started yet so we have plenty of time to get any package into Ubuntu X. I'm happy to help with both tasks: getting the package into debian (probably Leo, Jochen and other members of debian-science can help too) and making the backport for Wily.

Required Support for:

- Ubuntu Wily (15.10)
- Ubuntu X (16.04)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ubuntu X is xenial: https://wiki.ubuntu.com/Releases

@esteve esteve mentioned this pull request Dec 3, 2015
74 tasks
Minimum Requirements:

- C++03
- C++11
Copy link
Member

Choose a reason for hiding this comment

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

From todays meeting: C++11 only.

But we should clarify that we won't change existing API to leverage it (e.g. not switching from boost::shared_ptr to std::shared_ptr).

@esteve
Copy link
Contributor Author

esteve commented Dec 8, 2015

I've updated the document according to today's notes, adding rationales for many of the decisions.

@jack-oquin
Copy link
Contributor

Where is the rationale for the C++11 compiler requirement? I didn't see that.

@tfoote
Copy link
Member

tfoote commented Dec 9, 2015

@jack-oquin

We typically pick the lowest common denominator of our planned supported platforms. Debian, Ubuntu, Fedora, and OSX all have c++11 support built in in the currently supported versions. It's our standard logic so I don't think we need to explicitly call it out.

It will allow our users to use the newer more advanced features. If we specify the minimum s c++03 then people cannot use std::chrono std::shared_ptr std::thread aliases, improved iterators, auto, lambda etc. There are lots of articles about how to leverage c++11 The Biggest Features in C++11, Ten C++11 Features Every C++ Developer Should Use We are also starting to see system dependencies which require c++11 such as sdformat.

Plus it will also allow better compatibility with ROS2.0 which also sets c++11 as a low water mark. And using C++11 standard libs gets us a long way toward cross platform support.

@jack-oquin
Copy link
Contributor

@tfoote: that is the sort of rationale I was looking for. Some of it might be worth mentioning explicitly.

Are we saying that the ROS 1 core may no longer work with earlier compilers?

@tfoote
Copy link
Member

tfoote commented Dec 9, 2015

Yes, with this as the minimum you will need a c++ compiler which implements c++11. As stated we do not plan to change or refactor the API, but we will accept feature enhancements and bug fixes which use c++ in their implementation. Something that can easily be used in bug fixes is auto iterators inside of for loops. It's much more compact and readable, and does not change anything externally.

@jack-oquin
Copy link
Contributor

Something that can easily be used in bug fixes is auto iterators inside of for loops. It's much more compact and readable, and does not change anything externally.

I wonder whether that is worth the reduced portability it introduces into ROS 1, auto only saves a few keystrokes, assuming a reasonable editor.

@tfoote
Copy link
Member

tfoote commented Dec 10, 2015

Yes auto is mostly a convenience, it's the simplest example I could come up with of using c++11 within the core. The features added to C++11 are very powerful, they are not just "saving a few keystrokes".

The reason we have distros is to allow us to make progress and provide a structured way to drop backwards compatibility. Supporting older compilers than any of our target platforms provide is commitment to support those other configurations for 5 years. It will take more work for core maintenance and unless you have a strong use case which requires supporting an older compiler we don't want to commit the whole community to that additional burden.

As I see it most people who will be running an older compiler will probably also be running an older distro, and as such will need to run an older ROS distro too.

@jack-oquin
Copy link
Contributor

OK, I just wanted to make sure there was a conscious decision to burn those bridges.

@wjwwood
Copy link
Contributor

wjwwood commented Dec 10, 2015

OK, I just wanted to make sure there was a conscious decision to burn those bridges.

Bottom line is that Debian Jessie has GCC 4.9 which supports C++11 and Ubuntu Wily has a C++11 compiler and that's the minimum (oldest) systems we're interested in supporting for 5 years starting in May 2016. Not to mention Trusty has a C++11 compiler (for the most part), Fedora is already on the GCC 5.x series, OS X has had one for 4 years, and even VS2015 has C++11 support. I think we've waited long enough to up the requirement. Indigo will continue to be an option for those without a C++11 compiler for an additional 3 years. I don't think we're burning any bridges in the sense that we're disenfranchising any group of people of which I am aware. If you can point to any single example where this concretely affects portability then I'd be very interested to learn about it. This is after all why we post the requirements so far in advance, so that people who are adversely affected can speak up, but I haven't heard a valid argument against C++11 as a minimum requirement.

@jack-oquin
Copy link
Contributor

I am not arguing against it, just requesting an explicit rationale.

As far as usage, I expect that most of ROS 1 will continue running Indigo on Trusty for several more years. So, the ability to easily back-port useful new features to that platform will probably prove quite helpful.

@wjwwood
Copy link
Contributor

wjwwood commented Dec 10, 2015

Well I apologize then, because I interpreted this statement:

I wonder whether that is worth the reduced portability it introduces into ROS 1, auto only saves a few keystrokes, assuming a reasonable editor.

As an argument against increasing the minimum requirement and/or using C++11 features like auto in Kinetic.

It seems we agree, and if any further justification is required I guess we could simply put something like: "We are requiring C++11 so that the community can begin to use C++11 in their packages and we do not foresee any issue with this minimum requirement because all target systems for Kinetic provide support for C++11."

So, the ability to easily back-port useful new features to that platform [indigo] will probably prove quite helpful.

The idea of Indigo as an LTS is that we don't back port features in order to keep it stable. I know not everyone follows that rule, but not everyone follows the C++03 rule in Indigo now (Trusty supports C++11 and people do use it in their own packages). If someone is doing a bug fix that should be back ported to Indigo then they should consider that when deciding whether or not to use C++11 features in the fix.

@jack-oquin
Copy link
Contributor

The idea of Indigo as an LTS is that we don't back port features in order to keep it stable. I know not everyone follows that rule, but not everyone follows the C++03 rule in Indigo now (Trusty supports C++11 and people do use it in their own packages). If someone is doing a bug fix that should be back ported to Indigo then they should consider that when deciding whether or not to use C++11 features in the fix.

Right. You should keep Indigo stable by avoiding back-ports.

But, over the next few years specific Indigo-based projects are likely to pick up useful new features, by back-porting some newer packages (in source). That is not destabilizing for the distro at all.

For that to be practical, the new code should not rely on exotic C++11 features not supported by the Trusty compiler.

@wjwwood
Copy link
Contributor

wjwwood commented Dec 11, 2015

For that to be practical, the new code should not rely on exotic C++11 features not supported by the Trusty compiler.

Of the C++11 features that Trusty's compiler does not support (a short list of uncommonly used features), I think most could easily be worked around by changing the way to code works or by using Boost. Evidence of the sufficiency of Trusty's compiler for C++11 is that all of our ROS 2 work, which relies heavily on C++11 features, has been done using Trusty and gcc-4.8. That being said I do not believe that strategy to be practical anyways, since it ignores any other issues that might come up due changes between indigo and kinetic that would make it impractical. I think this is a non-issue. If there is to be further discussion on this topic I'd like to hear the opinion of people who are not me or @jack-oquin.

@gavanderhoorn
Copy link
Contributor

Would it be wise to check the support for C++11 in embedded contexts? Obviously if they use a relatively recent compiler it would imply that it is supported, but I'm not sure if there are runtime implications for embedded platforms that we're not considering here.

This is rather vague, I admit, but something I thought might be overlooked.

@wjwwood
Copy link
Contributor

wjwwood commented Dec 11, 2015

@gavanderhoorn That's a good example of a group that might be at risk. Can you be more specific on what you'd consider "embedded contexts"?

AFAIK, ROS 1 still only runs on systems with OS's and of those I'm most familiar with Ubuntu. Here at OSRF we do regular testing on Qualcomm and Nvidia boards among others, and they use Trusty or newer and apparently they plan to standardize on Debian Jessie at some point (which has gcc 4.9) because Ubuntu is moving too fast. That's why we considered what Debian Jessie has when trying to decide on a minimum requirement. I'm not up to speed on other embedded development environments, but as long as they are based on gcc-4.8 or better then they should be able to compile. It would be great if someone from the embedded ROS community could speak to whether this as an issue or not.

Separately there is a concern about runtime implications, which I take to mean performance at runtime? Most of the functionality in C++11 is either compiler sugar (disappears at the assembly level) or is something that we're already using Boost to accomplish. So if doesn't work with C++11 even though the compiler supports it, then I'd be surprised that it worked before without syntactic sugar and with Boost.

@scpeters
Copy link

There's a version of gcc 4.9 for ARM in a PPA that is maintained by an ARM employee:

@vrabaud
Copy link
Contributor

vrabaud commented Dec 20, 2015

I'd like to reiterate the need for OpenCV3 : getting it into upstream can also be problematic because of the usual patent issues (that's why SIFT is not in Debian for example). Right now, people are kindof backporting that in to Indigo / Jade but OpenCV does nto provide an easy way. Having the ROS package deal with it will also simplify that effort.

@gavanderhoorn
Copy link
Contributor

@wjwwood: sorry for taking so long to respond.

Can you be more specific on what you'd consider "embedded contexts"?

Unfortunately not. I was just thinking that this (embedded contexts) might be something that would deserve some more attention. If we really want to be thorough it would perhaps be a good idea to get some input from some more authoritative people from that domain.

@esteve
Copy link
Contributor Author

esteve commented Feb 4, 2016

@vrabaud I've updated the version of OpenCV to 3.1.x, sorry it took so long, we've been a bit backlogged because of ROS2

@dirk-thomas
Copy link
Member

Do we expect any further changes to this or should we put this out for vote soon?

@@ -205,7 +250,7 @@ While we mainly develop with gcc, no use of compiler-specific features is allowe
without proper use of macros to allow use on other platforms.

Use of C++11/C++14 features and filesystem/networking/etc... TS's (Technical Specifications) is allowed if they are checked for at configure time and equivalent functionality can be provided with the extra compiler features.
We will continue using only C++03 features to preserve backwards compatibility, but code must also compile with C++11 compilers, i.e. when ``-std=c++11`` is used.
Support for C++11 is now a compiler requirement, but the API will not use any C++11-specific feature.

Choose a reason for hiding this comment

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

Which packages are intended to be covered by this API requirement?

Choose a reason for hiding this comment

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

(if someone is writing a non-core package, is it OK for them to have an API that uses C++11 features?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, these are the rules we should follow for the core packages (historically desktop-full). For everyone else they are just guidelines which they may follow if they agree with the rational.

Choose a reason for hiding this comment

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

@esteve maybe it would be good to add a line to that effect here, just to be explcit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonbinney I've added a note about this, let me know if it needs to be improved. Thanks!

Choose a reason for hiding this comment

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

Looks great, thanks!

@Tabjones
Copy link

👍

@stonier
Copy link
Contributor

stonier commented Feb 20, 2016

Some comments re c++11 support.

From what I understand, g++ will never have c++03 nor c++11 support by default. It will skip it and introduce c++14 support by default in g++ 6.0. Which in the ubuntu world is probably still quite some time off.

The one headache I have had and can think of is that any and all libraries built by a ros system will require people down the line to almost always add that std=c++11 flag when building their own applications (think - any library header using a std::shared_ptr will bomb if they dont. Even if their own application doesn't use, nor need any c++11 api from the underlying library. In a way, this is forcing the whole ros community to adopt c++11 as well as anyone who depends on a library built by the ros community.

Now, having said that - I would still be in favour of seeing ROS do this. The significance of the jump is such that the cost of dealing with non-default flags is worthwhile. As validation - we already set the flag in all of our internal software running on top of ROS and make extensive use of c++11 features, even on indigo/trusty.

For cross-compilation on lower end arm boards, we went through the transition to toolchains supporting c++03/c++11 about a year ago. Note that we don't compile any ROS for these boards, but we use catkin to build all of our cross-compiled code. I would hazard to say that if you have a good enough arm board to be running ROS 1.0 (which is above the level of boards we use), or programs that are equivalent complexity, then you'll have support in the toolchains for c++11. And by the time ROS 2.0 starts getting popular on arm (which hopefully can run on much lower end boards than ROS 1.0), then the issue of c++11 or not will be well and truly passed.

@vrabaud
Copy link
Contributor

vrabaud commented Feb 22, 2016

following-up in @stonier , how about we actually impose C++14 ? That would simplify the long term maintenance. Plus gcc 5.3.1 is the default in Xenial (http://packages.ubuntu.com/xenial/gcc) which seems to have had C++14 support for a while (https://gcc.gnu.org/projects/cxx1y.html).
Otherwise, catkin could add the flag by default and that would be transparent to the user no ?

@jack-oquin
Copy link
Contributor

+1

@esteve
Copy link
Contributor Author

esteve commented Feb 22, 2016

@vrabaud that sounds like a good idea, the changes between C++11 and C++14 are fairly incremental and not as disruptive as between C++03 and C++11. +1 to C++14

@dirk-thomas
Copy link
Member

Since Jessy is the lowest common denominator with gcc 4.9 (https://packages.debian.org/jessie/gcc) I don't think C++14 is an option (https://github.com/ros-infrastructure/rep/pull/106/files#diff-41ae062315c36c45afaa5c192b2ca7c5R193).

@esteve
Copy link
Contributor Author

esteve commented Feb 22, 2016

@dirk-thomas that's true, I forgot about Jessie. Here's the status of C++14 in GCC as a reference:

https://gcc.gnu.org/projects/cxx1y.html

@scpeters
Copy link

Also following up from @stonier , I see cmake logic in various places that checks if Windows is being used before appending the -std=c++11 flag (ros2/rclcpp for example). Should we add a macro for that to catkin?

@dirk-thomas
Copy link
Member

I don't think that the few CMake lines are the problem here.

The problem with a package starting to use C++11 in public API / headers is that it will require all downstream packages to also set the compile flag. The package should then export the CMake variable pkgname_DEFINITIONS but no downstream package uses that. So every downstream package needs to start using these variables to use the same flags. Basically including C++11 in the public API / headers is an API and ABI break.

@@ -205,7 +250,7 @@ While we mainly develop with gcc, no use of compiler-specific features is allowe
without proper use of macros to allow use on other platforms.

Use of C++11/C++14 features and filesystem/networking/etc... TS's (Technical Specifications) is allowed if they are checked for at configure time and equivalent functionality can be provided with the extra compiler features.
We will continue using only C++03 features to preserve backwards compatibility, but code must also compile with C++11 compilers, i.e. when ``-std=c++11`` is used.
Support for C++11 is now a compiler requirement, but the API of the packages included in desktop-full will not use any C++11-specific feature. External packages are encouraged to follow this guideline, but not required to do so.

Choose a reason for hiding this comment

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

I don't think we need to mention

but not required to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@esteve
Copy link
Contributor Author

esteve commented Feb 26, 2016

Thanks everyone for your feedback, I've squashed this branch and CI is happy. Merging now.

esteve added a commit that referenced this pull request Feb 26, 2016
Update to REP-0003 for ROS Kinetic Kame
@esteve esteve merged commit 5e47400 into master Feb 26, 2016
@esteve esteve deleted the rep3_kinetic branch February 26, 2016 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet