Skip to content

Ros2 port#53

Merged
mlautman merged 36 commits intoros2from
ros2-port-building
Mar 22, 2019
Merged

Ros2 port#53
mlautman merged 36 commits intoros2from
ros2-port-building

Conversation

@mlautman
Copy link
Copy Markdown

@mlautman mlautman commented Mar 19, 2019 •
edited
Loading

This gets moveit_ci to pass.

Still TODO: clang_tidy and misc. cleanup.

Definitely squash and merge

@mlautman mlautman changed the base branch from master to ros2 March 19, 2019 20:44
@mlautman mlautman force-pushed the ros2-port-building branch from f999c74 to e73249f Compare March 19, 2019 20:47
@mlautman mlautman force-pushed the ros2-port-building branch from daba81d to 6d3a21f Compare March 19, 2019 22:53
@mlautman mlautman force-pushed the ros2-port-building branch from d68b765 to d534c15 Compare March 20, 2019 03:16
@mlautman mlautman mentioned this pull request Mar 20, 2019
Merged
@mlautman mlautman force-pushed the ros2-port-building branch from ba33d81 to 076645e Compare March 20, 2019 22:02
@mlautman mlautman changed the title WIP: Ros2 port building Ros2 port Mar 21, 2019
mlautman
mlautman commented Mar 21, 2019
View reviewed changes
mlautman
mlautman commented Mar 21, 2019
View reviewed changes
mlautman
mlautman commented Mar 21, 2019
View reviewed changes
@mlautman
Copy link
Copy Markdown
Author

mlautman commented Mar 21, 2019

@rhaschke I believe you will be interested in this PR. This effort has already taken too long so let's focus on getting this merged quickly :)

@mlautman mlautman mentioned this pull request Mar 21, 2019
Merged
@mlautman mlautman mentioned this pull request Mar 21, 2019
Merged
@mlautman mlautman requested a review from rhaschke March 21, 2019 23:07
@mlautman mlautman mentioned this pull request Mar 21, 2019
Merged
henningkayser
henningkayser requested changes Mar 22, 2019
View reviewed changes
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.

This should do good for a first prototype. I realize that my changes are probably out of the scope of this PR and not really necessary yet but it would be great if you could add corresponding TODOs.

unit_tests.sh
run_test 1 $0:$LINENO "unknown TEST" TEST=invalid TEST_PKG=valid

run_test 1 $0:$LINENO "empty catkin workspace" TEST_PKG=valid 'BEFORE_SCRIPT="rm valid"'
# TODO(mlautman): Decide if we should keep this test as an empty ros workspace doesn't
Copy link
Copy Markdown
Member

@henningkayser henningkayser Mar 22, 2019

Choose a reason for hiding this comment

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

I would also say it can probably be deleted. But sometimes tests are there for a reason

Copy link
Copy Markdown
Author

@mlautman mlautman Mar 22, 2019

Choose a reason for hiding this comment

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

This comment was left as a question for @rhaschke. I commented it out since empty ros workspaces don't seem like a compelling reason to fail. Restored

rhaschke
rhaschke reviewed Mar 22, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

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

@mlautman, at first glance I already found some points to further improve. I will work on them over the weekend. Today I don't have time for it.

@rhaschke
Copy link
Copy Markdown
Contributor

rhaschke commented Mar 22, 2019

I support all comments of Henning, but I think that they should be addressed asap. I just installed ros-crystal on my machine, so I hope I can take my first ROS2 steps over the weekend and help then...

@mlautman mlautman force-pushed the ros2-port-building branch from 075ec55 to f796b89 Compare March 22, 2019 20:05
@mlautman mlautman requested a review from henningkayser March 22, 2019 20:09
henningkayser
henningkayser approved these changes Mar 22, 2019
View reviewed changes
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.

Looks good!

@mlautman mlautman force-pushed the ros2-port-building branch from 185d9f2 to f90f97b Compare March 22, 2019 21:20
@mlautman
Copy link
Copy Markdown
Author

mlautman commented Mar 22, 2019

I am going to merge this as is so that we have a working CI. @rhaschke I look forward to reviewing your additional improvements

Thanks everyone for taking the time to help get this first pass set up!

@mlautman mlautman merged commit a154a23 into ros2 Mar 22, 2019
@mlautman mlautman deleted the ros2-port-building branch March 22, 2019 22:01
@rhaschke rhaschke mentioned this pull request Mar 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhaschke rhaschke rhaschke left review comments

@henningkayser henningkayser henningkayser approved these changes

@davetcoleman davetcoleman Awaiting requested review from davetcoleman

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mlautman @rhaschke @henningkayser