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

Set C++ 11 #6

Merged
merged 2 commits into from
Aug 23, 2018
Merged

Conversation

moriarty
Copy link
Contributor

Because @lucbettaieb said he got an email about:

http://build.ros.org/job/Ldev__sparse_bundle_adjustment__ubuntu_xenial_amd64/3/

@mikeferguson I also added myself as a maintainer. I'm taking care of whatever the fetch_ros packages depend on.

@mikeferguson
Copy link
Member

2 things:

  • I don't think we want to do it this way. Melodic will pass in a build format of C++14 when building debs -- so I think we want to set C++11 ONLY if no build version is set (otherwise we might end up with weird linking errors on Melodic, where everything else is built in C++14).
  • If you're taking over for Luc, should we remove him from the list of maintainers?

@mikeferguson
Copy link
Member

Disregard my first comment -- looks like roscpp is hard coding C++11 -- so I guess we'll be OK.

Second comment remains.

@lucbettaieb
Copy link
Contributor

I don't mind getting the emails, etc, so I'm happy to stay on. Otherwise, I don't mind.

@mikeferguson mikeferguson merged commit 23518fb into ros-perception:melodic-devel Aug 23, 2018
@mikeferguson
Copy link
Member

Ok -- I'll let that run through CI -- if it passes on Melodic and Lunar, I'll cut a new release. (I think we should also switch slam_karto to melodic-devel branch for Lunar once this is released)(

@@ -1,6 +1,8 @@
cmake_minimum_required(VERSION 2.8.3)
project(sparse_bundle_adjustment)

add_compile_options(-std=c++11)

Choose a reason for hiding this comment

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

The CMake recommended way is now:

if(NOT CMAKE_CXX_STANDARD)
  set(CMAKE_CXX_STANDARD 11)
endif()

This works cross platform and allows to override it if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I thought I remembered seeing a recommended way...
I just checked tf and went with the way it was done there.

@mikeferguson
Copy link
Member

Thanks @dirk-thomas -- I'll update to that

@mikeferguson
Copy link
Member

One note for posterity: CMAKE_CXX_STANDARD requires CMAKE > 3.1, so old CMakeLists needs to be updated to have new minimum version

@dirk-thomas
Copy link

CMake >= 3.1 😉

@mikeferguson
Copy link
Member

0.4.2 is out for lunar, coming for melodic momentarily

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

4 participants