-
Notifications
You must be signed in to change notification settings - Fork 205
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
base: master
Are you sure you want to change the base?
Conversation
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
*****
There was a problem hiding this comment.
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)
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.