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

add BOOST_MATH_DISABLE_FLOAT128 #505

Merged
merged 2 commits into from
May 15, 2017

Conversation

mikaelarguedas
Copy link
Contributor

Description

This fixes #504 the compilation issue on Ubuntu Zesty and Debian Stretch.
This change is only for lunar and doesn't need to be cherry-picked into other branches

Note: I settled for this solution because it seemed the least intrusive to me. There are several other fixes that could be considered:

  • By changing the -std=c++11 flag to -std=gnu++11 in the add_compile_option() call but I preferred to keep the flags consistent across all packages of the moveit repository.
  • By using (but needs to bump cmake minimum version to 3.0.2):
set(CMAKE_CXX_STANDARD 11)`
set(CMAKE_CXX_STANDARD_REQUIRED ON)

Checklist

  • Required: Code is auto formatted using clang-format: didn't change code
  • Extended the tutorials / documentation, if necessary reference: doesn't apply
  • Include a screenshot if changing a GUI: doesn't apply
  • Optional: Created tests, which fail without this PR reference: doesn't apply
  • Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic): not needed

Thank you!

@130s
Copy link
Contributor

130s commented May 14, 2017

Thanks for the work @mikaelarguedas. I've asked a few potential reviewers. I guess with minimum one approval we can move forward.

I'm tentatively testing on my fork 130s#1 with industrial_ci that allows prerelease test on Lunar and Zesty.

To Maintainers: #504 is for ROS Lunar, and this is targetted to kinetic-devel and Travis passed. So this change can go into kinetic-devel and no need to create lunar-devel (for now)?

@mikaelarguedas
Copy link
Contributor Author

Thanks @130s
Just to clarify this does fix the build of moveit_ros_visualization on Zesty and Stretch but doesn't fix the current moveit_setup_assistant failure on Yakkety (the same failures will still exist on Zesty and Stretch once this is merged). Should I open an issue to track the moveit_setup_assistant failure?

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

Should I open an issue to track the moveit_setup_assistant failure?

Yes, please. At first glance, this seems to relate to problems with the urdf package in yakkety.

So this change can go into kinetic-devel and no need to create lunar-devel (for now)?

Yes.

@mikaelarguedas: Please add a short comment above the definition to explain that a boost header included through ogre doesn't compile with -std=c++11 without this.

I didn't test this myself, but it makes sense that this should resolve the compile error.

@mikaelarguedas
Copy link
Contributor Author

mikaelarguedas commented May 14, 2017

Should I open an issue to track the moveit_setup_assistant failure?

Yes, please. At first glance, this seems to relate to problems with the urdf package in yakkety.

Ticketed #506

@mikaelarguedas: Please add a short comment above the definition to explain that a boost header included through ogre doesn't compile with -std=c++11 without this.

Done 162b83a, feel free to push directly to this branch if you want to improve the wording

I didn't test this myself, but it makes sense that this should resolve the compile error.

I ran prerelease tests for all platforms (Xenial, Yakkety, Zesty and Stretch) and it compiled without error.

@v4hn
Copy link
Contributor

v4hn commented May 15, 2017

The wording is fine.
Thanks a lot for your support @mikaelarguedas !

@v4hn v4hn merged commit 543cba0 into moveit:kinetic-devel May 15, 2017
@mikaelarguedas mikaelarguedas deleted the fix_float128_error branch August 1, 2020 15:40
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

3 participants