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

Support Iron and transition Rolling to Noble #10

Merged
merged 18 commits into from
May 29, 2024

Conversation

Yadunund
Copy link
Member

WIP

@Yadunund Yadunund force-pushed the yadu/update_distributions branch 3 times, most recently from 2b61ece to 798bc4a Compare March 16, 2024 16:53
Signed-off-by: Yadunund <[email protected]>
@Yadunund Yadunund mentioned this pull request Mar 17, 2024
@Yadunund Yadunund changed the title Support Iron Support Iron and transition Rolling to Noble Mar 17, 2024
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
@Yadunund Yadunund force-pushed the yadu/update_distributions branch 2 times, most recently from c3890a8 to d0cf8ea Compare March 17, 2024 16:06
Signed-off-by: Luca Della Vedova <[email protected]>
@luca-della-vedova
Copy link
Member

Jazzy / Noble docker images are up, now the failure in the CI is in fetching a rmf.repos from the rmf repo jazzy branch since no such branch exists yet.
Rolling docker images are actually not in noble yet, but I expect that to happen anytime now.

Signed-off-by: Luca Della Vedova <[email protected]>
@Yadunund
Copy link
Member Author

Jazzy / Noble docker images are up, now the failure in the CI is in fetching a rmf.repos from the rmf repo jazzy branch since no such branch exists yet. Rolling docker images are actually not in noble yet, but I expect that to happen anytime now.

My recommendation is to not add jazzy/noble combination here until the distro is released and we've updated our repos to include jazzy branches.

Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
@luca-della-vedova
Copy link
Member

luca-della-vedova commented May 24, 2024

Findings from my research and debugging:

  • Clang doesn't seem to work because of an issue on the setup-ros action Clang on Rolling / Noble fails to compile any package ros-tooling/setup-ros#685.
  • The clang and lld mixin don't play quite well with asan, so I moved those to gcc as well.
  • rmf_fleet_adapter_python also doesn't work well with asan, we would need to preload the address sanitizer library to avoid errors when running Python, so I changed the sample asan to just run on rmf_fleet_adapter.
  • Lastly, the asan action has quite a lot of errors and segfaults, I'll not hold it as a blocker to this PR to make sure that is OK since I believe we never really ran an Open-RMF wide asan action.

Edit: I also changed to the bare ros:distro image, as a temporary fix until the new desktop images are ready. I added a TODO to make sure we address it

Signed-off-by: Luca Della Vedova <[email protected]>
@Yadunund
Copy link
Member Author

Should we just drop clang, lld, etc? They should no longer be needed.

@luca-della-vedova
Copy link
Member

I am getting a significantly worse performance for compiling under gcc/ld compared to clang/lld, for example actions for 26c5428 (with clang/lld) took a normal time, while in 14e78a5 (with gcc/ld) were taking more than double the time until I cancelled them.
I'll keep experimenting

@luca-della-vedova
Copy link
Member

I feel the current status, with green build and test for all distros and green asan on Humble is the best we can hope for.
I suspect we never actually tested for asan on Iron or Rolling in the past, current issues:

Iron asan seems to be failing on fastrtps, example output:

2024-05-24T20:17:09.6649235Z     ==44942==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000007f0 (pc 0x7ff6c375bef4 bp 0x60c0006b2dc0 sp 0x7ffff95c7048 T0)
2024-05-24T20:17:09.6650494Z     ==44942==The signal is caused by a READ memory access.
2024-05-24T20:17:09.6651220Z     ==44942==Hint: address points to the zero page.
2024-05-24T20:17:09.6651916Z     ==44942==WARNING: invalid path to external symbolizer!
2024-05-24T20:17:09.6652730Z     ==44942==WARNING: Failed to use and restart external symbolizer!
2024-05-24T20:17:09.6654030Z         #0 0x7ff6c375bef4  (/lib/x86_64-linux-gnu/libc.so.6+0x97ef4) (BuildId: 962015aa9d133c6cbcfb31ec300596d7f44d3348)
2024-05-24T20:17:09.6655747Z         #1 0x7ff6be2ef20d  (/opt/ros/iron/lib/libfastrtps.so.2.10+0x34820d) (BuildId: a17d15658847c53e8609260c3308529bf5ffa3c1)
2024-05-24T20:17:09.6657530Z         #2 0x7ff6be2f1735  (/opt/ros/iron/lib/libfastrtps.so.2.10+0x34a735) (BuildId: a17d15658847c53e8609260c3308529bf5ffa3c1)
2024-05-24T20:17:09.6659410Z         #3 0x7ff6c19ca8ed  (/opt/ros/iron/lib/librmw_fastrtps_shared_cpp.so+0x4e8ed) (BuildId: 26ffed4ec6855b417c93e7ef8977e33baa96787f)
2024-05-24T20:17:09.6661211Z         #4 0x7ff6c5a3a89b  (/opt/ros/iron/lib/librcl.so+0x2b89b) (BuildId: 06fa6dca7f44b5a0082924dbc102de5ef1754bc0)
2024-05-24T20:17:09.6662804Z         #5 0x7ff6c5a3ac52  (/opt/ros/iron/lib/librcl.so+0x2bc52) (BuildId: 06fa6dca7f44b5a0082924dbc102de5ef1754bc0)
2024-05-24T20:17:09.6664317Z         #6 0x7ff6c5c921f3  (/opt/ros/iron/lib/librclcpp.so+0x14c1f3) (BuildId: 30853bd50c7eab48c22306ef21235a4f7e60180e)
2024-05-24T20:17:09.6665933Z         #7 0x7ff6c5c734e7  (/opt/ros/iron/lib/librclcpp.so+0x12d4e7) (BuildId: 30853bd50c7eab48c22306ef21235a4f7e60180e)
2024-05-24T20:17:09.6667349Z         #8 0x7ff6c5c71fe6  (/opt/ros/iron/lib/librclcpp.so+0x12bfe6) (BuildId: 30853bd50c7eab48c22306ef21235a4f7e60180e)
2024-05-24T20:17:09.6668987Z         #9 0x7ff6c3709a55  (/lib/x86_64-linux-gnu/libc.so.6+0x45a55) (BuildId: 962015aa9d133c6cbcfb31ec300596d7f44d3348)
2024-05-24T20:17:09.6670588Z         #10 0x7ff6c5c344d6  (/opt/ros/iron/lib/librclcpp.so+0xee4d6) (BuildId: 30853bd50c7eab48c22306ef21235a4f7e60180e)
2024-05-24T20:17:09.6671688Z     
2024-05-24T20:17:09.6672109Z     AddressSanitizer can not provide additional info.
2024-05-24T20:17:09.6673848Z     SUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-gnu/libc.so.6+0x97ef4) (BuildId: 962015aa9d133c6cbcfb31ec300596d7f44d3348) 
2024-05-24T20:17:09.6674929Z     ==44942==ABORTING

I'd say we either switch to cyclone in our CI or accept that it won't really work.
Rolling asan is a bit more cryptic and seems to fail on uncrustify, I haven't looked at it yet.

In any case we should hold off merge until the upstream PR in setup-ros that fixes clang is merged, then we can revert the custom branch in our action here.

@Yadunund
Copy link
Member Author

Thanks for the efforts here! I'm not very familiar with how asan works but am curious to know what happens if we add an

        env:
          RMW_IMPLEMENTATION: rmw_cyclonedds_cpp

to the script.

Signed-off-by: Luca Della Vedova <[email protected]>
@luca-della-vedova
Copy link
Member

Thanks for the efforts here! I'm not very familiar with how asan works but am curious to know what happens if we add an

        env:
          RMW_IMPLEMENTATION: rmw_cyclonedds_cpp

to the script.

I'm a bit on the fence on this type of approach since it will mask issues for users that use the default rmw, still, since we recommend cyclone by default I did it in 358a71a and it fixed the spurious issues in iron making it green, now the only missing part is the rolling asan action that fails on uncrustify (while the normal build and test action doesn't)

@Yadunund
Copy link
Member Author

We should still open a ticket in upstream fastdds/rmw_fastrtps to let the maintainers know of the issue. Hopefully it can be fixed.

I'm really puzzled by uncrustify issues surfacing only during asan

@luca-della-vedova
Copy link
Member

I ticketed:

I'll now proceed to approve and merge this PR to unblock rolling CI and Jazzy releases

@luca-della-vedova luca-della-vedova merged commit 6343040 into main May 29, 2024
6 of 7 checks passed
@luca-della-vedova luca-della-vedova deleted the yadu/update_distributions branch May 30, 2024 07:31
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