[Windows][melodic-devel] Fix Windows\MSVC build breaks#357
[Windows][melodic-devel] Fix Windows\MSVC build breaks#357v4hn merged 5 commits intomoveit:melodic-develfrom ms-iot:windows_fix
Conversation
|
Thanks for helping in improving MoveIt! |
|
Similar comments about commit msgs and quote changes as in moveit/panda_moveit_config#32. |
|
@gavanderhoorn Thanks for the feedback! I pushed again with more meaningful commit messages and removed the double quotes change. I will be looking into what I can do at |
thanks. let us know whether getting something into Otherwise we'll have to start changing MoveIt launch files "everywhere" I guess (and the templates used by the MoveIt Setup Assistant). |
rhaschke
left a comment
rhaschke
left a comment
There was a problem hiding this comment.
In general, this looks good. Some remarks though.
| project(moveit_tutorials) | ||
|
|
||
| add_compile_options(-std=c++14) | ||
| set(CMAKE_CXX_STANDARD 14) |
There was a problem hiding this comment.
CMAKE_CXX_STANDARD was introduced in cmake 3.1. Using it here, cmake_minimum_required should be increased as well!
There was a problem hiding this comment.
If we change to this way of specifying the standard, I would propose to
- (1) Do it consistently throughout MoveIt (esp. for the main repository too)
- (2)
set( CMAKE_CXX_EXTENSIONS OFF )too
Please see this old thread for a discussion on the various ways to specify the standard.
There was a problem hiding this comment.
I updated the cmake_minimum_required and added set( CMAKE_CXX_EXTENSIONS OFF ) here. And I agreed that it should be done consistently throughout Moveit repositories.
There was a problem hiding this comment.
Do you have the cycles to contribute this style-change all throughout the main repository too?
It would be very much appreciated... (but independent of this request)
There was a problem hiding this comment.
@v4hn you mean this https://github.com/ros-planning/moveit repository, right? Sure I can do it.
| ${CATKIN_PACKAGE_SHARE_DESTINATION} | ||
| ARCHIVE | ||
| DESTINATION | ||
| ${CATKIN_PACKAGE_SHARE_DESTINATION} |
There was a problem hiding this comment.
This should be probably LIB resp. BIN, shouldn't it?
There was a problem hiding this comment.
This library is not installed properly and its headers are also not exported.
I would propose to install the library to CATKIN_PACKAGE_LIB_DESTINATION (I guess that's an oversight by @mlautman ), but not install the headers at all.
After all we don't want people to depend on a library from the tutorials package.
There was a problem hiding this comment.
I'm cleaning this up as part of #355 right now.
There was a problem hiding this comment.
I rebased to get the latest cleanup.
| project(moveit_tutorials) | ||
|
|
||
| add_compile_options(-std=c++14) | ||
| set(CMAKE_CXX_STANDARD 14) |
There was a problem hiding this comment.
Do you have the cycles to contribute this style-change all throughout the main repository too?
It would be very much appreciated... (but independent of this request)
as required by the MoveIt maintainers, enforce strict standard through `CMAKE_CXX_EXTENSIONS OFF`
|
Congrats on getting your first MoveIt! pull request merged and improving open source robotics! |
|
Thanks @seanyen, One minor issue: Instead of pushing multiple fixup commits over time to address requested changes, please feel free to force-push the branch and clean up the original commits. |
5 participants
This changes is to fix build breaks on Windows/MSVC and issues found at runtime:
set(CMAKE_CXX_STANDARD 14)to regulate the compiler to use C+14 standard in a portable way.ARCHIVE DESTINATIONandRUNTIME DESTINATIONfor shared library install since they are required for Windows build.ros::Duration(1.0).sleep();oversleep()to make the code more portable.$(find xacro)/xacro.pyto$(find xacro)/xacrobecausexacroseems no longer to installxacro.pyto the package bin folder: https://github.com/ros/xacro/blob/melodic-devel/CMakeLists.txt#L15