Skip to content

replaced <ros2-distro> with $ROS_DISTRO #658

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glitchhopcore
Copy link

@glitchhopcore glitchhopcore commented Mar 17, 2025

We have specified <ros2-distro> to refer to the ROS2 version, instead of which we can simply use $ROS_DISTRO, which would make it more copy-paste friendly.

Signed-off-by: doublebrackets <absolutecasul@gmail.com>
@@ -54,9 +54,9 @@ Running the Example

.. code-block:: bash

source /opt/ros/<ros2-distro>/setup.bash
source /opt/ros/$ROS_DISTRO/setup.bash
Copy link
Member

@SteveMacenski SteveMacenski Mar 17, 2025

Choose a reason for hiding this comment

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

A note: the ROS_DISTRO env variable isn't set before you source the setup.bash file, so that would not be in your environment yet to work. This is actually true of several of the changes in this PR.

Do you think that may cause more problems than it solves if when using ROS_DISTRO when not set it will return an empty string? I feel like that might confuse users if they get errors trying to access source /opt/ros//setup.bash or apt install ros--gazebo-ros-pkgs because the variable is not set

Copy link
Author

@glitchhopcore glitchhopcore Mar 17, 2025

Choose a reason for hiding this comment

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

I guess you are right, it would definitely make it more confusing.
Furthermore for places like build_docs/index.rst, we seem to source /opt/ros/<distro>/setup.bash, do you think we should suggest users to have setup.bash sourced before running the commands wherever I have mentioned the changes, or do you think there is a better way to handle this, or if it is alright as it is?


For Iron and Older
==================

.. code:: bash

  source /opt/ros/<distro>/setup.bash
  sudo apt install \
    ros-$ROS_DISTRO-navigation2 \
    ros-$ROS_DISTRO-nav2-bringup \
    ros-$ROS_DISTRO-turtlebot3*

For Jazzy and Newer
===================

.. code:: bash

  source /opt/ros/<distro>/setup.bash
  sudo apt install \
    ros-$ROS_DISTRO-navigation2 \
    ros-$ROS_DISTRO-nav2-bringup \
    ros-$ROS_DISTRO-nav2-minimal-tb*

Build
*****

Copy link
Member

@SteveMacenski SteveMacenski Mar 17, 2025

Choose a reason for hiding this comment

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

That would help for this case, but I think for all cases where you need to source (at least), then we would need to not use ROS_DISTRO. I think that counts for more than just in the build docs -- it might be worth looking over your changes and see for what other situations we haven't yet told the user to source the setup script and add that too (or leave as-is)

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.

2 participants