search

Field-Robotics-Lab / dave

Project DAVE
Apache License 2.0
224 stars 72 forks source link

Change exec_depend to run_depend #250

Closed Yadunund closed 2 years ago

Yadunund commented 2 years ago

PR https://github.com/Field-Robotics-Lab/dave/pull/239 moved a couple exec dependencies from dave_nodes pkg into rexrov_oberon7_moveit. However, the former's package.xml is v3.1.1 which supports exec_depend while the latter is still on v0.3.0 which does not.

Fix https://github.com/Field-Robotics-Lab/dave/issues/248

Signed-off-by: Yadunund yadunund@openrobotics.org

bsb808 commented 2 years ago

This seems like a simple enough PR - do we know why the CI is failing?

j-herman commented 2 years ago

This seems like a simple enough PR - do we know why the CI is failing?

@bsb808 All CI checks appear to be failing, starting sometime yesterday. I don't see any info on why - trying to troubleshoot but this one might be above my paygrade.

mabelzhang commented 2 years ago

CI is failing on

install_upstream_dependencies

  $ ( source /opt/ros/noetic/setup.bash && rosdep install -q --from-paths /home/runner/work/dave/dave/.work/upstream_ws/src --ignore-src -y | grep -E '(executing command)|(Setting up)' ; )
  '( source /opt/ros/noetic/setup.bash && rosdep install -q --from-paths /home/runner/work/dave/dave/.work/upstream_ws/src --ignore-src -y | grep -E '(executing command)|(Setting up)' ; )' returned with 1
'install_upstream_dependencies' returned with code '1' after 0 min 0 sec

Looking at the list of closed PRs, the earliest one merged with failing CI is #239. Then #240. Then #242. So yeah I think this PR is fixing the right thing.

It's Saturday for Yadu, so I suggest we merge this and not wait for him.

mabelzhang commented 2 years ago

Unfortunately, this PR might not fix it. I see the 2 lines in this PR already in #240. #240 goes into the integrated world feature branch, but it also failed CI.

That's consistent with this PR failing CI. #240 was also failing at the same place.

install_upstream_dependencies

  $ ( source /opt/ros/noetic/setup.bash && rosdep install -q --from-paths /home/runner/work/dave/dave/.work/upstream_ws/src --ignore-src -y | grep -E '(executing command)|(Setting up)' ; )
  '( source /opt/ros/noetic/setup.bash && rosdep install -q --from-paths /home/runner/work/dave/dave/.work/upstream_ws/src --ignore-src -y | grep -E '(executing command)|(Setting up)' ; )' returned with 1
'install_upstream_dependencies' returned with code '1' after 0 min 0 sec

So #239 needs another fix. The easiest in the short term is to revert #239 and see if CI passes. Then at least we won't miss further regressions by merging further PRs with failing CI check.

239 fixes a runtime error, so CI will pass, but we need to open an issue to track the runtime error #239 was trying to fix.

j-herman commented 2 years ago

@mabelzhang I noticed the same thing but at least with this PR I'm able to build the main repo. Haven't found the specific issue yet in https://github.com/Field-Robotics-Lab/dave/pull/239 - distracted by other tasking today.

mabelzhang commented 2 years ago

2 package are being added. rospack shows I have one, but not the other:

$ rospack find moveit_commander
/opt/ros/noetic/share/moveit_commander
$ rospack find moveit_planners
[rospack] Error: package 'moveit_planners' not found

I think I have the latest dockwater, and I see both had been added to dockwater here https://github.com/Field-Robotics-Lab/dockwater/pull/21

Can someone else reproduce this?

I have moveit_planners_chomp and moveit_planners_ompl. Does it need to be a specific one?

mabelzhang commented 2 years ago

Reading #239 again, the errors mention both chomp and ompl. Maybe we just need to replace

  <exec_depend>moveit_planners</exec_depend>

with moveit_planners_chomp and moveit_planners_ompl? I'll open a PR to try CI.

j-herman commented 2 years ago

I can confirm the same results with trying to find moveit_planners, but functionally, the planners are working for me. I have ros-noetic-moveit-planners installed in dockwater, and MoveIt is working fine on the demos. On the plus side, it looks like my quick fix is not failing CI, so while I don't know why it works, there's a good chance it will do what we need for today.

mabelzhang commented 2 years ago

They could be working at runtime because there is already a

  <run_depend>moveit_planners_ompl</run_depend>

So the new

  <run_depend>moveit_planners</run_depend>

might just be not doing anything, and since it's at runtime, we don't know that it didn't find it. That's a guess. But it's throwing off CI because that package doesn't exist.

mabelzhang commented 2 years ago

251 fixes CI

mabelzhang commented 2 years ago

253 too :woman_shrugging: