Skip to content

Change exec_depend to run_depend#250

Merged
mabelzhang merged 1 commit into
masterfrom
fix_dependencies
Mar 25, 2022
Merged

Change exec_depend to run_depend#250
mabelzhang merged 1 commit into
masterfrom
fix_dependencies

Conversation

@Yadunund
Copy link
Copy Markdown
Member

PR #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 #248

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

Signed-off-by: Yadunund <yadunund@openrobotics.org>
@Yadunund Yadunund requested a review from woensug-choi March 25, 2022 02:28
@bsb808 bsb808 added this to the 4.3.1 milestone Mar 25, 2022
@bsb808
Copy link
Copy Markdown
Contributor

bsb808 commented Mar 25, 2022

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

@j-herman
Copy link
Copy Markdown
Contributor

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.

@j-herman j-herman self-requested a review March 25, 2022 17:25
Copy link
Copy Markdown
Contributor

@j-herman j-herman left a comment

Choose a reason for hiding this comment

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

Need to merge these changes despite failing checks - master branch currently can't be built and is failing CI as well. If this does not resolve the issue we may need to investigate further.

@mabelzhang
Copy link
Copy Markdown
Contributor

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 mabelzhang merged commit 12e7123 into master Mar 25, 2022
@mabelzhang mabelzhang deleted the fix_dependencies branch March 25, 2022 22:35
@mabelzhang
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

@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 #239 - distracted by other tasking today.

@mabelzhang mabelzhang mentioned this pull request Mar 25, 2022
12 tasks
@mabelzhang
Copy link
Copy Markdown
Contributor

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 HonuRobotics/dockwater#21

Can someone else reproduce this?

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

@mabelzhang
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

mabelzhang commented Mar 25, 2022

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
Copy link
Copy Markdown
Contributor

#251 fixes CI

@mabelzhang
Copy link
Copy Markdown
Contributor

#253 too 🤷‍♀️

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.

Movit_commander package.xml build error

4 participants