Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to ros system packages #278

Merged
merged 41 commits into from
Nov 27, 2023
Merged

Conversation

adityapande-1995
Copy link
Collaborator

@adityapande-1995 adityapande-1995 commented May 17, 2023

This PR aims to migrate drake_ros, drake_ros_examples, ros2_examples_bazel_installed to use humble debs instead of the nightly cyclonedds tarball.


This change is Reviewable

Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
@adityapande-1995 adityapande-1995 changed the title Migrate to humble debs Migrate to ros system packages May 17, 2023
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
@adityapande-1995 adityapande-1995 marked this pull request as ready for review May 22, 2023 23:16
@adityapande-1995 adityapande-1995 marked this pull request as draft May 22, 2023 23:59
@adityapande-1995 adityapande-1995 marked this pull request as ready for review May 23, 2023 20:09
Copy link
Collaborator

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 11 files at r1, 8 of 9 files at r3, 1 of 1 files at r4, 2 of 4 files at r5, 2 of 3 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @adityapande-1995, @calderpg-tri, and @IanTheEngineer)


drake_ros/package.xml line 30 at r7 (raw file):

  <depend>visualization_msgs</depend>
  <depend>tf2_py</depend>
  <depend>rmw_cyclonedds_cpp</depend>

This is one of those weird cases where we probably don't want to set it in the package.xml since it's not a dependency of this package. I'd recommend installing it manually in CI for the CMake jobs, and having it in the install_prereqs.sh scripts for the bazel jobs.


drake_ros/drake_ros/BUILD.bazel line 97 at r7 (raw file):

    srcs = ["test/geometry_conversions_test.py"],
    main = "test/geometry_conversions_test.py",
    rmw_implementation = "rmw_cyclonedds_cpp",

Ah, this is repeated in a lot of places with the current design (not the fault of this PR). Which RMW implementation to use is probably a project-wide decision. Do you see a way to add rmw_implementation to the base ROS 2 repository attributes such that if specified at the workspace level then one doesn't have to pass rmw_implementation into each individual test rule?


drake_ros/setup/install_prereqs.sh line 22 at r7 (raw file):

rosdep init
rosdep update
rosdep install --from-paths . --rosdistro=humble

. is the current working directory. If the user does cd drake_ros/setup && ./install_prereqs.sh then this won't find any packages. There's a bash incantation that gives you the directory containing the script, from which you can concatenate ../ to get the right directory.


drake_ros/setup/install_prereqs.sh line 24 at r7 (raw file):

rosdep install --from-paths . --rosdistro=humble

apt install python3 python3-toposort

Recommend adding a note that this is a bazel_ros2_rules dependency.


drake_ros_examples/setup/install_prereqs.sh line 23 at r7 (raw file):

rosdep init
rosdep update
rosdep install --from-paths ../drake_ros --rosdistro=humble

same comment about this being relative to the current working directory

Copy link
Collaborator Author

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 17 files reviewed, 4 unresolved discussions (waiting on @calderpg-tri, @IanTheEngineer, and @sloretz)


drake_ros/setup/install_prereqs.sh line 22 at r7 (raw file):

Previously, sloretz (Shane Loretz) wrote…

. is the current working directory. If the user does cd drake_ros/setup && ./install_prereqs.sh then this won't find any packages. There's a bash incantation that gives you the directory containing the script, from which you can concatenate ../ to get the right directory.

Done


drake_ros/setup/install_prereqs.sh line 24 at r7 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Recommend adding a note that this is a bazel_ros2_rules dependency.

Done


drake_ros_examples/setup/install_prereqs.sh line 23 at r7 (raw file):

Previously, sloretz (Shane Loretz) wrote…

same comment about this being relative to the current working directory

Done


drake_ros/package.xml line 30 at r7 (raw file):

Previously, sloretz (Shane Loretz) wrote…

This is one of those weird cases where we probably don't want to set it in the package.xml since it's not a dependency of this package. I'd recommend installing it manually in CI for the CMake jobs, and having it in the install_prereqs.sh scripts for the bazel jobs.

Done

Copy link
Collaborator Author

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 15 of 17 files reviewed, 2 unresolved discussions (waiting on @calderpg-tri, @IanTheEngineer, and @sloretz)


drake_ros/setup/install_prereqs.sh line 22 at r7 (raw file):

Previously, sloretz (Shane Loretz) wrote…

I'm not sure this works, but it's almost there. It can return just a dot:

$ dirname -- "$0"
.

This trick uses pwd to get the full path: https://stackoverflow.com/questions/59895/how-do-i-get-the-directory-where-a-bash-script-is-located-from-within-the-script

Done


drake_ros_examples/setup/install_prereqs.sh line 23 at r7 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Still an issue: https://stackoverflow.com/questions/59895/how-do-i-get-the-directory-where-a-bash-script-is-located-from-within-the-script

Done

@EricCousineau-TRI EricCousineau-TRI self-assigned this Nov 27, 2023
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

+@EricCousineau-TRI :lgtm: with some nits

Reviewed 3 of 11 files at r1, 1 of 4 files at r5, 1 of 3 files at r8, 1 of 2 files at r9, 3 of 6 files at r10, 8 of 11 files at r13, 1 of 2 files at r14.
Reviewable status: 18 of 20 files reviewed, 10 unresolved discussions (waiting on @adityapande-1995, @calderpg-tri, @IanTheEngineer, and @sloretz)

a discussion (no related file):
Working: Will see if I can address my nits.



.github/workflows/bazelized_drake_ros.yml line 37 at r14 (raw file):

      # Setup.
      - name: Free up disk space

nit This is now redundant w.r.t. above step (see "Free up space").


.github/workflows/bazelized_drake_ros.yml line 77 at r14 (raw file):

        working-directory: drake_ros
      - name: Build drake_ros
        run: export ROS_DISTRO=humble; bazel build //...

nit Unclear why this is necessary here and not below.

For now, can we revert to not exporting this env var? (that is how we currently do things in Anzu)


drake_ros/setup/install_prereqs.sh line 2 at r14 (raw file):

#!/bin/bash
set -uo pipefail

This should fail on errors, -e.


drake_ros_examples/setup/install_prereqs.sh line 2 at r14 (raw file):

#!/bin/bash
set -euo pipefail

nit Same as above. This should fail fast on errors, so we should keep -e.


drake_ros_examples/setup/install_prereqs.sh line 30 at r14 (raw file):

# Required for bazel_ros2_rules
apt install python3 python3-toposort

nit Needs trailing line.


ros2_example_bazel_installed/setup/install_prereqs.sh line 67 at r14 (raw file):

  9276a1e11f03e9f7492f009803c95bddc307993c9ab3c463721c9f6cdaa2ccc1

if [[ -z "${ROS2_DISTRO_PREFIX:-}" ]]; then

nit For clarity, consider stating "Install ROS 2 humble if user does not specify a version."


ros2_example_bazel_installed/setup/install_prereqs.sh line 72 at r14 (raw file):

  update-locale LC_ALL=en_US.UTF-8 LANG=en_US.UTF-8
  export LANG=en_US.UTF-8
  

nit Trailing whitespace (here and elsewhere).

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 18 of 20 files reviewed, 11 unresolved discussions (waiting on @adityapande-1995, @calderpg-tri, @IanTheEngineer, and @sloretz)


drake_ros_examples/setup/install_prereqs.sh line 17 at r14 (raw file):

echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/ros-archive-keyring.gpg] http://packages.ros.org/ros2/ubuntu $(. /etc/os-release && echo $UBUNTU_CODENAME) main" |  tee /etc/apt/sources.list.d/ros2.list > /dev/null

apt update; apt upgrade

nit We should avoid unnececssary apt upgrade

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 18 of 20 files reviewed, 12 unresolved discussions (waiting on @adityapande-1995, @calderpg-tri, @IanTheEngineer, and @sloretz)


ros2_example_bazel_installed/WORKSPACE line 84 at r14 (raw file):

]

ros2_local_repository(

nit Having two different targets that resolve to near the same thing is a bit hard to parse.

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r15, all commit messages.
Dismissed @sloretz from 2 discussions.
Reviewable status: 19 of 20 files reviewed, all discussions resolved (waiting on @calderpg-tri and @IanTheEngineer)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Will see if I can address my nits.

Done.



.github/workflows/bazelized_drake_ros.yml line 37 at r14 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This is now redundant w.r.t. above step (see "Free up space").

Done.


.github/workflows/bazelized_drake_ros.yml line 77 at r14 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Unclear why this is necessary here and not below.

For now, can we revert to not exporting this env var? (that is how we currently do things in Anzu)

Done.


drake_ros/setup/install_prereqs.sh line 2 at r14 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This should fail on errors, -e.

Done.


drake_ros_examples/setup/install_prereqs.sh line 2 at r14 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Same as above. This should fail fast on errors, so we should keep -e.

Done.


drake_ros_examples/setup/install_prereqs.sh line 17 at r14 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit We should avoid unnececssary apt upgrade

Done.

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r14, 3 of 3 files at r16, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @calderpg-tri and @IanTheEngineer)

@EricCousineau-TRI EricCousineau-TRI merged commit d7aa28f into main Nov 27, 2023
8 checks passed
@adityapande-1995 adityapande-1995 deleted the aditya/switch_to_debs branch November 28, 2023 16:16
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