Pin navigation2 repos and simplify docker build (#142).

[xfiderek]

This PR addresses: #142

This PR introduces the following changes

• Pin version of navigation2.repos to ensure reproducible builds
• Create update script according to diagram from here: Space ROS Technical Meeting Agenda 2024-06-27 space-ros#166 (comment). The script is called docker_update_nav2_repos.sh
• Simplify dockerfile
• Add space_nav2_bringup package allowing to launch nav2 stack (we had to get rid of nav2_bringup due to heavy dependencies such as rviz)
• Update nav2_demo accordingly -> update readme

Out of scope (will be fixed as part of different issues):

• Simplifying space_robots demo (e.g. excluding gazebo from space robots img)
• Optimizing image size of nav2 image -> will be addressed separately
• Move the docker image to stacks repo -> will be addressed separately
• Remove rviz2 from the image

[mkhansenbot]

Hi Blazej, you've done great work, but there's too much in a single PR here. I think we should split this out into 3 PRs. The first PR simply pins the navigation2.repos file, add the scripts to generate it, and update the Dockerfile (fixing #142). The 2nd PR would then add the space_nav2_bringup and remove the rviz dependency (can you file a new issue for that?). The 3rd PR would update the nav2_demos to add in the rviz2 and update README. This will keep a cleaner git history, so that we can then move the nav2_demos folder to the space-ros/demos repo, per issue space-ros/demos#28

[xfiderek]

Hi @mkhansenbot, thanks for the review!

Indeed, you are right that there are lots of changes in this PR and I am fine with splitting it further. However, doing as you suggested, I believe the demo will stop working after merging PR#2 and before PR#3 is merged, since rviz will no longer be available in the nav2 image (unless we want to launch it in a separate container, however this would require some changes to other dockerfiles that would later be reverted). Am I right?
If that is the case, I propose to split this work into 2 PRs instead of 3:

• PR#1: pin the navigation2.repos file, add the scripts to generate it, and update the Dockerfile (closing Need to pin the Navigation2 version number for stability #142)
• PR#2: Remove rviz dependency, add space_nav2_bringup, update demo to use separate image from rviz (closing Update nav2_demo to match the new build approach demos#28)

What do you think?

[mkhansenbot]

Sounds good, thanks!

[mkhansenbot]

Let me know when those two PRs are ready for review

[Bckempa]

Hey @xfiderek, issue #142 is listed for this release milestone, but this draft is still set to draft and is not set to a milestone. Do you want this in this release or does it need more time to bake?

[xfiderek]

hi @Bckempa. I plan to finish it this weekend, but would be good to merge space-ros/space-ros#170 because b4 that we can't move it from draft stage

[Bckempa]

... would be good to merge space-ros/space-ros#170 because b4 that we can't move it from draft stage

You're clear on #170, no rush of course but feel free to ping when you're ready for a review

[asimonov]

ah, I never looked at this PR in detail, but looks like I was working on similar issues with #165 to cleanup some of dependencies management.

happy to revert PR I created, but I am curious what is the plan for this PR? split?

[asimonov]

May be more general question is in order here...

we are using rosinstall_generator (not just here, but in space-ros too), but the ros2 core has not fully switched to it yet, but uses ros2.repos file (https://docs.ros.org/en/jazzy/Installation/Alternatives/Ubuntu-Development-Setup.html#build-ros-2).

The issue was discussed here: ros2/ros2_documentation#1470, but it was de-scoped from humble.

Does anyone know the status of this? rosinstall_generator obviously works for ros2, but it is still not official way...

[xfiderek]

ah, I never looked at this PR in detail, but looks like I was working on similar issues with #165 to cleanup some of dependencies management.

happy to revert PR I created, but I am curious what is the plan for this PR? split?

Yes, I will split the PR into 2 according to what I discussed with @mkhansenbot above + gazebo/rviz will be removed from nav2 img

[mkhansenbot]

Yes, I will split the PR into 2 according to what I discussed with @mkhansenbot above + gazebo/rviz will be removed from nav2 img

@xfiderek - if you remove Gazebo from the Nav2 build, the system tests (which actually run simulations) will fail. I thought the plan was to remove rviz. Can we do that and leave Gazebo alone for now? I don't want a regression to occur. At a later date, we can add our own system tests and remove Gazebo

[asimonov]

ah, I never looked at this PR in detail, but looks like I was working on similar issues with #165 to cleanup some of dependencies management.
happy to revert PR I created, but I am curious what is the plan for this PR? split?

Yes, I will split the PR into 2 according to what I discussed with @mkhansenbot above + gazebo/rviz will be removed from nav2 img

ok, I would like to test it, but will wait till you split.

[xfiderek]

@xfiderek - if you remove Gazebo from the Nav2 build, the system tests (which actually run simulations) will fail. I thought the plan was to remove rviz. Can we do that and leave Gazebo alone for now? I don't want a regression to occur. At a later date, we can add our own system tests and remove Gazebo

@mkhansenbot my bad, I don't plan on removing gazebo here. In this PR I am removing only rviz dependency. Gazebo stays as-is for now. I am working on splitting this PR right now.

So overall:

• This PR will only pin nav2 repos and address the corresponding issue
• A separate PR will be created by me remove Rviz as dependency.. This will address Update nav2_demo to match the new build approach demos#28
• Yet another issue should be created to remove gazebo as a dependency and we can work on it separately. we may use refactor navigation2 docker - cleanup dependencies #165.

[xfiderek]

Hi @mkhansenbot , @Bckempa, @asimonov, other potential reviewers

I tried dividing it as we initially anticipated, but due to the problem with apt/from-source installation (dependency on nav2_bringup) it is really hard to make pinned repos working accurately without introducing space_nav2_bringup package. Is it ok if I introduce both pinned repos and space_nav2_bringup as part of this PR? Otherwise I would need to rework this significantly, only to rollback to what we have here. I will remove rviz2 changes from PR to keep reworks to the demo minimal.

I tested it locally and it is ready for review, but I think we need to wait for #162 before merging it, as we can't test it with the latest version of space robots image from main now due to using $ROSDISTRO instead of $ROS_DISTRO there.

So to sum it up:

• this PR is ready for review - it introduces space_nav2_bringup and pins repos, closing Need to pin the Navigation2 version number for stability #142
• my next PR (which I will create after this one gets merged) will remove dep on rviz2 and close Update nav2_demo to match the new build approach demos#28
• one more PR and corresponding issue will be created to remove gazebo from space_robots

Remember that you need to build the latest spaceros image locally for it to work

[xfiderek]

May be more general question is in order here...

we are using rosinstall_generator (not just here, but in space-ros too), but the ros2 core has not fully switched to it yet, but uses ros2.repos file (https://docs.ros.org/en/jazzy/Installation/Alternatives/Ubuntu-Development-Setup.html#build-ros-2).

The issue was discussed here: ros2/ros2_documentation#1470, but it was de-scoped from humble.

Does anyone know the status of this? rosinstall_generator obviously works for ros2, but it is still not official way...

We are using rosinstall_generator "offline", to generate a ros2.repos (and navigation2.repos) file, so in a sense we are using .repos file as well to pull dependencies. rosinstall_generator is only used as a convenient tool to generate .repos file. Before the release, we locally trigger a script that uses rosinstall generator. We then push generated .repos files to github and use them for building images.

[asimonov]

I tried dividing it as we initially anticipated, but due to the problem with apt/from-source installation (dependency on nav2_bringup) it is really hard to make pinned repos working accurately without introducing space_nav2_bringup package. Is it ok if I introduce both pinned repos and space_nav2_bringup as part of this PR? Otherwise I would need to rework this significantly, only to rollback to what we have here. I will remove rviz2 changes from PR to keep reworks to the demo minimal.

I agree about nav2_bringup. I am ok to keep space_nav2_bringup in this PR.

I will review later.

[asimonov]

I was about to verify this PR works by rebuilding almost all images we have from other in-flight PRs, but alas the git-lfs change interfered:

space-ros/simulation#35

[xfiderek]

I rebased, resolved conflicts, and pushed changes. testing it locally.

UPDATE: I re-tested changes and nav2 demo works fine. the only problem is that I had to setup export MESA_LOADER_DRIVER_OVERRIDE=softpipe for every GUI app in the docker container. I am not sure if its local issue, so I'd appreciate if you could run it on your pc @asimonov. Anyway, this problem is not related to that PR as I am facing it since the first time I run space robots demo locally

BTW, @EzraBrooks I can see that the commit message CI checker failed. I believe its because I opened pull request from my own repo. I think this CI check should be fixed, wdyt? All the PRs that were merged after adding this check were opened from space-ros/docker repo, and I believe that is why CI was green for them.

[asimonov]

I rebased, resolved conflicts, and pushed changes. testing it locally.

UPDATE: I re-tested changes and nav2 demo works fine. the only problem is that I had to setup export MESA_LOADER_DRIVER_OVERRIDE=softpipe for every GUI app in the docker container. I am not sure if its local issue, so I'd appreciate if you could run it on your pc @asimonov. Anyway, this problem is not related to that PR as I am facing it since the first time I run space robots demo locally

BTW, @EzraBrooks I can see that the commit message CI checker failed. I believe its because I opened pull request from my own repo. I think this CI check should be fixed, wdyt? All the PRs that were merged after adding this check were opened from space-ros/docker repo, and I believe that is why CI was green for them.

I can confirm I can run the demo.

But for some reason in the last step with rviz it shows there is no transform for 'map'.
I had to clone navigation2 repo into docker and use the default rviz config from navigation2 because the rviz config did not make it into the changed docker. Not sure if it was different from default, but it is not docker as we do not bring nav2_bringup there. So may be put it into space_nav2_bringup/config?

i am talking about this step:

docker/nav2_demo/README.md

Line 44 in 5c8467d

ros2 launch nav2_bringup rviz_launch.py

[asimonov]

I rebased, resolved conflicts, and pushed changes. testing it locally.
UPDATE: I re-tested changes and nav2 demo works fine. the only problem is that I had to setup export MESA_LOADER_DRIVER_OVERRIDE=softpipe for every GUI app in the docker container. I am not sure if its local issue, so I'd appreciate if you could run it on your pc @asimonov. Anyway, this problem is not related to that PR as I am facing it since the first time I run space robots demo locally
BTW, @EzraBrooks I can see that the commit message CI checker failed. I believe its because I opened pull request from my own repo. I think this CI check should be fixed, wdyt? All the PRs that were merged after adding this check were opened from space-ros/docker repo, and I believe that is why CI was green for them.

I can confirm I can run the demo.

But for some reason in the last step with rviz it shows there is no transform for 'map'. I had to clone navigation2 repo into docker and use the default rviz config from navigation2 because the rviz config did not make it into the changed docker. Not sure if it was different from default, but it is not docker as we do not bring nav2_bringup there. So may be put it into space_nav2_bringup/config?

i am talking about this step:

docker/nav2_demo/README.md

Line 44 in 5c8467d

ros2 launch nav2_bringup rviz_launch.py

I did another attepmt to run navigation demo.
Basically I can start navigation and localization launch files.
And I use the rviz config from humble branch of nav2 (https://raw.githubusercontent.com/ros-navigation/navigation2/refs/heads/humble/nav2_bringup/rviz/nav2_default_view.rviz) because we dont have rviz config migrated in this PR.
I can see costmaps appear but when i send a goal I see the following error message in rviz2 window:

[INFO] [1728656495.856096390] [rviz_navigation_dialog_action_client]: NavigateToPose will be called using the BT Navigator's default behavior tree.
2024-10-11 14:21:35.856 [RTPS_READER_HISTORY Error] Change payload size of '32' bytes is larger than the history payload size of '19' bytes and cannot be resized. -> Function can_change_be_added_nts
[ERROR] [1728656495.956606773] [rviz_navigation_dialog_action_client]: Send goal call failed

did anyone run navigation2 demo successfully? I was never able to do it...

[asimonov]

I think it works. But would be nice to have rviz config included from nav2 into space_nav2_bringup, otherwise it is missing. We still have rviz2 from apt in the image, so the instructions are actually valid.

[asimonov]

I think it works. But would be nice to have rviz config included from nav2 into space_nav2_bringup, otherwise it is missing. We still have rviz2 from apt in the image, so the instructions are actually valid.

I tried the navigation demo again and it worked... i would say i probaby did something wrong last time trying to juggle 2 containers from different branches, as well as few terminals.

one observation: nav2_default_view.rviz at the moment comes from /opt/ros/humble - there is nav2_bringup package there. it comes there via apt install of rviz2 and nav2-bringup... I see it is still marked as TODO in nav2 Dockerfile.
may be we need to finish it as part of this PR. at least copy rviz config so it is more self-contained. even if we dont add rviz2 to nav2 container... or may be we can just change instructions to pull vanilla nav2 docker and launch vanilla nav2 bringup rviz?

[xfiderek]

I think it works. But would be nice to have rviz config included from nav2 into space_nav2_bringup, otherwise it is missing. We still have rviz2 from apt in the image, so the instructions are actually valid.

I tried the navigation demo again and it worked... i would say i probaby did something wrong last time trying to juggle 2 containers from different branches, as well as few terminals.

one observation: nav2_default_view.rviz at the moment comes from /opt/ros/humble - there is nav2_bringup package there. it comes there via apt install of rviz2 and nav2-bringup... I see it is still marked as TODO in nav2 Dockerfile. may be we need to finish it as part of this PR. at least copy rviz config so it is more self-contained. even if we dont add rviz2 to nav2 container... or may be we can just change instructions to pull vanilla nav2 docker and launch vanilla nav2 bringup rviz?

Hi @asimonov, good to know that it works!

Following the comment of @mkhansenbot I suggested to handle rviz2 in a separate PR to limit the changes, as this PR is already v. large. You are 100% right that it should be changed, but I would suggest to do it as a separate PR as demo should work with current instructions and I would love to merge it soon, because its been open for quite some time.
Also I am not sure if these configs would be part of space_nav2_bringup, as rviz would probably be launched from separate docker container. are you ok with that?

And just to confirm - you can run a demo without additional changes (e.g. cloning nav2 repos etc), right? You should be able run the demo as-is, set 2d pose estimate via rviz and then send nav2 goal (which should be followed by rover in gz).

[xfiderek]

@EzraBrooks any update on commitee message standards CI checker? It seems it it is the only blocker now.
Maybe for now you could manually review commits and disable this rule for this PR if we don't have time to fix the CI.

[asimonov]

I think it works. But would be nice to have rviz config included from nav2 into space_nav2_bringup, otherwise it is missing. We still have rviz2 from apt in the image, so the instructions are actually valid.

I tried the navigation demo again and it worked... i would say i probaby did something wrong last time trying to juggle 2 containers from different branches, as well as few terminals.
one observation: nav2_default_view.rviz at the moment comes from /opt/ros/humble - there is nav2_bringup package there. it comes there via apt install of rviz2 and nav2-bringup... I see it is still marked as TODO in nav2 Dockerfile. may be we need to finish it as part of this PR. at least copy rviz config so it is more self-contained. even if we dont add rviz2 to nav2 container... or may be we can just change instructions to pull vanilla nav2 docker and launch vanilla nav2 bringup rviz?

Hi @asimonov, good to know that it works!

Following the comment of @mkhansenbot I suggested to handle rviz2 in a separate PR to limit the changes, as this PR is already v. large. You are 100% right that it should be changed, but I would suggest to do it as a separate PR as demo should work with current instructions and I would love to merge it soon, because its been open for quite some time. Also I am not sure if these configs would be part of space_nav2_bringup, as rviz would probably be launched from separate docker container. are you ok with that?

And just to confirm - you can run a demo without additional changes (e.g. cloning nav2 repos etc), right? You should be able run the demo as-is, set 2d pose estimate via rviz and then send nav2 goal (which should be followed by rover in gz).

yes, correct, i was able to run just by following the modified README, no other changes

ok, i am happy to merge in current state and then focus on cleaning up rviz dependency in separate issue

[eholum]

@EzraBrooks any update on commitee message standards CI checker?

Fixed! Is this good to go then?

[xfiderek]

@EzraBrooks any update on commitee message standards CI checker?

Fixed! Is this good to go then?

@eholum I resolved all conversations. @asimonov tested it locally and it works for him. Lets merge it!