open-rmf / rmf

Root repository for the RMF software
Apache License 2.0
238 stars 59 forks source link

Update galactic branch #202

Closed Yadunund closed 2 years ago

Yadunund commented 2 years ago

This PR

  1. Merges in latest main into galactic
  2. Updates the rmf.repos file to checkout galactic-devel branches of the Open-RMF repos which has recently been created.

Moving forward main will be supported on humble. See https://github.com/open-rmf/rmf/pull/165 We can add instructions to that PR to inform users to use the galactic version of the rmf.repos file if the would like to build Open-RMF from source + galactic

orensbruli commented 2 years ago

With the new commits, I think we have progressed a little bit. @Yadunund can you double check that the packages removed in https://github.com/open-rmf/rmf/pull/202/commits/c0769bf1612e4523cdb4bbecef3cdbbb7f1bf99e are not needed in galactic.

https://github.com/open-rmf/rmf/runs/8254192916?check_suite_focus=true#step:7:43856

Can some of you take a look into those failing tests or ping someone who can know about it?

luca-della-vedova commented 2 years ago

With the new commits, I think we have progressed a little bit. @Yadunund can you double check that the packages removed in c0769bf are not needed in galactic.

I think the simulation plugins listed there would indeed be needed in galactic, otherwise simulation wouldn't work.

https://github.com/open-rmf/rmf/runs/8254192916?check_suite_focus=true#step:7:43856

Can some of you take a look into those failing tests or ping someone who can know about it?

I believe the rmf_utils issue is due to the exclusion for catch.hpp not working in the ament_uncrustify. That file is so long that uncrustify times out. I guess I would suggest going in that direction to investigate it. For rmf_traffic it seems a bit more sketchy, I can reproduce locally failing tests that seem due to an fcl unit test, specifically with this failing testcase:

     <testcase classname="test_rmf_traffic.global" name="Verify that FCL can handle continuous collisions" time="0.004" status="run">
       <failure message="result.is_collide" type="CHECK">
 FAILED:
   CHECK( result.is_collide )
 with expansion:
   false
 at /home/luca/rmf_ws/src/rmf/rmf_traffic/rmf_traffic/test/unit/fcl_test.cpp:98
       </failure>
     </testcase>

I'm not sure what we can do about it, since it seems to be due to a test on a third party library

orensbruli commented 2 years ago

I think the simulation plugins listed there would indeed be needed in galactic, otherwise simulation wouldn't work.

https://github.com/open-rmf/rmf/runs/8254192916?check_suite_focus=true#step:7:43856 Can some of you take a look into those failing tests or ping someone who can know about it?

You are right. My fault, thank you.

I believe the rmf_utils issue is due to the exclusion for catch.hpp not working in the ament_uncrustify. That file is so long that uncrustify times out. I guess I would suggest going in that direction to investigate it.

I'm gonna try looking into this.

For rmf_traffic it seems a bit more sketchy, I can reproduce locally failing tests that seem due to an fcl unit test, specifically with this failing testcase:

     <testcase classname="test_rmf_traffic.global" name="Verify that FCL can handle continuous collisions" time="0.004" status="run">
       <failure message="result.is_collide" type="CHECK">
 FAILED:
   CHECK( result.is_collide )
 with expansion:
   false
 at /home/luca/rmf_ws/src/rmf/rmf_traffic/rmf_traffic/test/unit/fcl_test.cpp:98
       </failure>
     </testcase>

I'm not sure what we can do about it, since it seems to be due to a test on a third party library

It's a third-party library but I think the test that is failing is ours, right? https://github.com/open-rmf/rmf_traffic/blame/main/rmf_traffic/test/unit/fcl_test.cpp I think @mxgrey created this test, may be you can take a look into this?

orensbruli commented 2 years ago

I believe the rmf_utils issue is due to the exclusion for catch.hpp not working in the ament_uncrustify. That file is so long that uncrustify times out. I guess I would suggest going in that direction to investigate it.

Investigating this. Notes for myself and anyone who want to follow this.

This is the call that @luca-della-vedova suggested to be failing https://github.com/open-rmf/rmf_utils/blob/main/rmf_utils/CMakeLists.txt#L95 and the EXCLUDE argument that is probably not working https://github.com/open-rmf/rmf_utils/blob/main/rmf_utils/CMakeLists.txt#L99

The code of ament_uncrustify command itself: https://github.com/ament/ament_lint/blob/galactic/ament_uncrustify/ament_uncrustify/main.py

OPTION 1: About the exclude, it looks like it's different between galactic version and rolling: Rolling https://github.com/ament/ament_lint/blob/rolling/ament_uncrustify/ament_uncrustify/main.py#L222 Galactic https://github.com/ament/ament_lint/blob/galactic/ament_uncrustify/ament_uncrustify/main.py#L234

As we are working on Galactic I'm debugging and I see that it only looks to exclude names of subdirectories in the current directory. We are passing include/rmf_utils/catch.hpp and this does not match the name of any of the subdirectories. It looks like this behavior is fixed in rolling with this commit: https://github.com/ament/ament_lint/commit/0a7c40f402bfddf7eb72268831c7bb5930aaa76a Made a PR to backport this fix: https://github.com/ament/ament_lint/pull/409

Any ideas on how to get through this as it is released?

OPTION 2: I have also found this pull request related to ament_add_test and TIMEOUT. I think we could add TIMEOUT argument to the ament_uncrustify call to ament_add_test (https://github.com/ament/ament_lint/blame/rolling/ament_cmake_uncrustify/cmake/ament_uncrustify.cmake#L70) Added a PR for this: https://github.com/ament/ament_lint/pull/408/commits