Add drake_catkin_installed example #168
Conversation
|
+@EricCousineau-TRI for review. |
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1.
Reviewable status: 6 unresolved discussions, platform LGTM from [ericcousineau-tri] (waiting on @jamiesnape)
drake_catkin_installed/.gitignore, line 1 at r1 (raw file):
/.catkin_tools/
BTW ew... that's a lotta cruft..
drake_catkin_installed/.gitignore, line 2 at r1 (raw file):
/.catkin_tools/ /.catkin_workspace
nit Can you add a comment stating that these are auto-generated by catkin_make?
drake_catkin_installed/README.md, line 3 at r1 (raw file):
# Catkin Project with an Installed Drake This example uses the [`catkin`](https://wiki.ros.org/catkin) build system with
nit Can you add that we use this because it is ROS1's primary build system?
(perhaps with a mention that ament, the tool for ROS2, does not yet have an example?)
drake_catkin_installed/src/.gitignore, line 1 at r1 (raw file):
/CMakeLists.txt
nit Can you add a comment stating where this comes from?
drake_catkin_installed/src/drake_catkin_installed/include/drake_catkin_installed/particle.h, line 54 at r1 (raw file):
/// - linear velocity (state/output index 1), in @f$ m/s @f$ units. /// /// @tparam_double_only
nit Do we use this in Drake at all? If not, can we scrub the alias for now?
scripts/continuous_integration/common/drake_catkin_installed, line 32 at r1 (raw file):
# POSSIBILITY OF SUCH DAMAGE. set -eux
nit Include -o pipefail? (it's not necessary, but a good habit to keep)
scripts/setup/linux/ubuntu/bionic/install_prereqs, line 123 at r1 (raw file):
locale-gen en_US.UTF-8 if [[ "${1:-}" == --ros-melodic ]]; then
nit Given that this logic is specific, can you assert that $# adds only 0 or 1 args?
Can you also have it fail fast if it is not --ros-melodic?
(That, or turn this into a for ... case $1 type construct?)
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, platform LGTM from [ericcousineau-tri] (waiting on @EricCousineau-TRI and @jamiesnape)
drake_catkin_installed/src/drake_catkin_installed/include/drake_catkin_installed/particle.h, line 54 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Do we use this in Drake at all? If not, can we scrub the alias for now?
22 times in Drake, by my count (it is new).
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, platform LGTM from [ericcousineau-tri] (waiting on @jamiesnape)
drake_catkin_installed/src/drake_catkin_installed/include/drake_catkin_installed/particle.h, line 54 at r1 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
22 times in Drake, by my count (it is new).
OK Ah, missed that - thanks!
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, platform LGTM from [ericcousineau-tri] (waiting on @EricCousineau-TRI and @jamiesnape)
drake_catkin_installed/README.md, line 3 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Can you add that we use this because it is ROS1's primary build system?
(perhaps with a mention thatament, the tool for ROS2, does not yet have an example?)
The colcon/ament example is written already (module CI), so it will be there soon enough that we can skip the mention, I think.
There was a problem hiding this comment.
Reviewed 17 of 17 files at r2.
Reviewable status: 1 unresolved discussion, platform LGTM from [ericcousineau-tri] (waiting on @jamiesnape)
drake_catkin_installed/README.md, line 3 at r1 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
The colcon/ament example is written already (module CI), so it will be there soon enough that we can skip the mention, I think.
SGTM.
|
It's unclear why the Catalina action is failing: There's no output from the failing |
Toward #163. Relates RobotLocomotion/drake#12773.
This change is