Skip to content

Port to ROS2#122

Merged
henningkayser merged 5 commits intomoveit:dashing-develfrom
rhaschke:dashing-devel
Nov 18, 2019
Merged

Port to ROS2#122
henningkayser merged 5 commits intomoveit:dashing-develfrom
rhaschke:dashing-devel

Conversation

@rhaschke
Copy link
Copy Markdown
Contributor

Rebase of #118.

Copy link
Copy Markdown
Contributor Author

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

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)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I suggest to remove this: The cmake file shouldn't impose any (non-required) build setting.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Copy link
Copy Markdown
Member

@henningkayser henningkayser Nov 18, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rhaschke rhaschke force-pushed the dashing-devel branch 2 times, most recently from f39c4a9 to f7f6902 Compare November 17, 2019 16:42
@vmayoral
Copy link
Copy Markdown
Contributor

@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.

@rhaschke
Copy link
Copy Markdown
Contributor Author

@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.
As you might have noticed, I'm not mentioned as author of any commits (except fixups, which I expect to be squashed with their base commits).

@rhaschke rhaschke force-pushed the dashing-devel branch 7 times, most recently from 9cc5026 to 44005dc Compare November 17, 2019 20:49
@rhaschke
Copy link
Copy Markdown
Contributor Author

@henningkayser, I have asked for a release of octomap into dashing.

@henningkayser
Copy link
Copy Markdown
Member

@rhaschke thanks for following up on the random_numbers and octomap dependencies! With octomap I had some issues with the ros2 branch, namely conflicting opengl headers and issues with the top-level CMakeLists octomap-distribution which lead to problems with colcon finding octomap (pretty much the same issue you just ran into with Travis CI). octomap-distribution is detected as the top-level package by colcon. Even with octomap included as build dependency, colcon doesn't find it and might even build geometric_shapes before octomap-distribution resulting in missing dependency octomap. If we add octomap-distribution as build dependency to the package.xml, rosdep complains that this package can't be found since it doesn't provide a package.xml. Either octomap-distribution needs to be a proper meta package with separate package.xml or it should be omitted in the ROS 2 package. I wanted to address those issues next, the forked branch contains temporary fixes.

@henningkayser, I have asked for a release of octomap into dashing.

That's great, with pre-built releases the issues above shouldn't occur

Copy link
Copy Markdown
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Ok, CI is successful now. Can you squash the fixup commits? After that, lgtm

@henningkayser henningkayser merged commit 83026ed into moveit:dashing-devel Nov 18, 2019
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.

4 participants