Conversation
4dbcd8b to
56f862c
Compare
rhaschke
left a comment
There was a problem hiding this comment.
Before merging this, I suggest to release random_numbers and declare octomap as a system dependency. Thus we can drop the corresponding workarounds for Travis.
@henningkayser, see comments below for my additional changes.
CMakeLists.txt
Outdated
|
|
||
| list(APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake/") | ||
|
|
||
| if(NOT CMAKE_CONFIGURATION_TYPES AND NOT CMAKE_BUILD_TYPE) |
There was a problem hiding this comment.
I suggest to remove this: The cmake file shouldn't impose any (non-required) build setting.
There was a problem hiding this comment.
This check was added throughout MoveIt packages to avoid bad first-time experiences for new users building from source. Solving planning problems can be apparently an order of magnitude slower when building with a standard pipeline and cmake's default.
If this is still the case (I don't see why the situation would have changed) I would vote to keep it.
There was a problem hiding this comment.
The only problem I see is that colcon prints all messages as warnings (actually, printing to stderr) which are treated as errors by Travis. We have to add
-DCMAKE_BUILD_TYPE=Release to each colcon build command
f39c4a9 to
f7f6902
Compare
|
@henningkayser and @rhaschke, you're self-attributing yourselves commits in here. I don't remember you contributed to this in February. See https://github.com/ros-planning/geometric_shapes/pull/96/commits. It'd be nice if you could rebase and add original authors. |
f7f6902 to
7953374
Compare
|
@vmayoral, Henning has put a lot of effort to cleanup the commits in #96, which were changing stuff back and forth. Probably that's the reason why he put himself as author for cmake related changes. |
9cc5026 to
44005dc
Compare
|
@henningkayser, I have asked for a release of octomap into dashing. |
|
@rhaschke thanks for following up on the
That's great, with pre-built releases the issues above shouldn't occur |
ab1ce0b to
c5a7dc3
Compare
Rebase of #118.