Fixes for Kinetic #161
Fixes for Kinetic #161
Conversation
|
@VictorLamoine, thanks for the PR. I'm working on getting some previous PRs done in indigo and jade. Once I have those branches finalized, I will create a kinetic branch for this change (I assume you are ok with that). Thursday is my ROS-I day, so you should see more activity then. |
|
Yup, closing and waiting :) |
|
BTW...no need to close. I can re-assign the base and approve the PR without anything else from you. |
|
I didn't know you could re-assign the base! Good to know |
| @@ -2,6 +2,8 @@ cmake_minimum_required(VERSION 2.8.3) | |||
|
|
|||
| project(industrial_trajectory_filters) | |||
|
|
|||
| add_compile_options(-std=c++11) | |||
There was a problem hiding this comment.
add_compile_options requires CMake version 2.8.12, so the first line should be updated as well.
There was a problem hiding this comment.
I changed all versions to 2.8.12. Where do you find the minimum required version for a CMake option?
There was a problem hiding this comment.
Hint from MoveIt code, https://cmake.org/pipermail/cmake/2013-October/056020.html
There was a problem hiding this comment.
Based on my own understanding of things (and this random SO post to back me up), wouldn't target_compile_options(..) be a better way of doing this?
There was a problem hiding this comment.
add_compile_options is to target_compile_options as include_directories is to target_include_directories.
If I understand it correctly, mixing options between targets might lead to ABI incompabilities, so the per-directory/package directive should be less error-prone.
There was a problem hiding this comment.
add_compile_optionsis totarget_compile_optionsasinclude_directoriesis totarget_include_directories.
I'm not sure I follow this. Being more specific is always a good thing in my opinion.
If I understand it correctly, mixing options between targets might lead to ABI incompabilities, so the per-directory/package directive should be less error-prone.
We wouldn't be mixing options, as target_compile_options(..) would be used with all targets in the package. It would also seem to support the transitive declaration of flags, which seems beneficial to me. I can't find docs on add_compile_options(..) stating that it supports that.
There was a problem hiding this comment.
I'm not sure I follow this. Being more specific is always a good thing in my opinion.
So, are you really using target_include_directories? ;)
We wouldn't be mixing options, as target_compile_options(..) would be used with all targets in the package.
IMHO this is not the purpose of target_compile_options, it is meant for fine granular settings. If you want to set options for all targets anyway, add_compile_options should work as expected. It sets the options for all sources in the current directory and below.
There was a problem hiding this comment.
What's the status of this pull request? If there are many ways to skin this cat, but the difference is minute, then perhaps we could merge this and iterate on the implementation as we go.
|
Travis is foobar'ed. I'm going to approve this PR and see if the branch build fails. Thanks everyone for the contribution. I'm going to work on a kinetic release tomorrow. |
#156