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

Install binaries and overlay minor fixups #4369

Open
wants to merge 4 commits into
base: rolling
Choose a base branch
from

Conversation

mikaelarguedas
Copy link
Member

  • update before upgrading
  • fix line number of edit of turtlesim package

@mikaelarguedas mikaelarguedas force-pushed the install_binaries_fixups branch from d16e926 to ee233ef Compare May 6, 2024 15:58
@mikaelarguedas mikaelarguedas force-pushed the install_binaries_fixups branch from a6bc319 to 84ddab1 Compare May 6, 2024 16:07
Signed-off-by: Mikael Arguedas <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
Signed-off-by: Mikael Arguedas <[email protected]>
@ahcorde ahcorde requested a review from clalancette May 6, 2024 20:42
Comment on lines +95 to +114

.. tabs::

.. group-tab:: Fast DDS

.. code-block:: bash

rosdep install --from-paths ~/ros2_{DISTRO}/ros2-linux/share --ignore-src -y --skip-keys "cyclonedds fastcdr fastrtps iceoryx_binding_c rmw_connextdds rti-connext-dds-6.0.1 urdfdom_headers"

.. group-tab:: Cyclone DDS

.. code-block:: bash

rosdep install --from-paths ~/ros2_{DISTRO}/ros2-linux/share --ignore-src -y --skip-keys "cyclonedds fastcdr fastrtps iceoryx_binding_c rmw_connextdds rti-connext-dds-6.0.1 urdfdom_headers"

.. group-tab:: Connext DDS

.. code-block:: bash

rosdep install --from-paths ~/ros2_{DISTRO}/ros2-linux/share --ignore-src -y --skip-keys "cyclonedds fastcdr fastrtps iceoryx_binding_c rmw_connextdds urdfdom_headers"
Copy link
Contributor

@clalancette clalancette May 7, 2024

Choose a reason for hiding this comment

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

I'm not sure I love the group tabs here. The problem is that they are really all very similar, with only one difference.

My suggestion is different. I suggest that we restore line 92 to exactly how it was, and then just add a note that says something like:

.. note::

   If you are using Connext-DDS, you should install Connext with `sudo apt-get install rti-connext-dds-6.0.1`.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that they are really all very similar, with only one difference.

I saw little of the ROS 2 docs but most tabs I've seen so far were:
For Linux: command A
For macOS: command A
For Windows: command B

This seem very similar to the case here


I suggest that we restore line 92 to exactly how it was, and then just add a note that says something like:

Semantically it's a bit weird to me to says:
Step 1 "Run this long complex command blacklisting what you need"
Step 2 "Now if you need it: run this extra command to manually install the thing you just blacklisted".

Copy link
Member Author

Choose a reason for hiding this comment

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

@ahcorde should I apply #4369 (comment) ?
Or can this be merged 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.

4 participants