search

open-navigation / opennav_coverage

Nav2 Compatible Complete Cover Task Server, Navigator, & BT Utils
Apache License 2.0
115 stars 30 forks source link

Fixes and Improvements #43

Open tonynajjar opened 7 months ago

tonynajjar commented 7 months ago

Fixes:

Improvements:

SteveMacenski commented 7 months ago

Linting failed an an odd error basically complaining about conflicts with every BT.CPPv4 node?

tonynajjar commented 7 months ago

Linting failed an an odd error basically complaining about conflicts with every BT.CPPv4 node?

I found a couple of build issue that I fixed. The logs were super verbose though, maybe there is something else, let's see

tonynajjar commented 7 months ago

It's still failing, looking at the logs, they contain both /opt/ros/iron/include/behaviortree_cpp_v3 and /opt/ros/iron/include/behaviortree_cpp, maybe that's a problem? Locally it builds...

On rolling there is only the newer version:

root@tony-xmg-22:/opt/overlay_ws# ls /opt/ros/rolling/include/ | grep behavior
behaviortree_cpp
SteveMacenski commented 7 months ago

That is odd - this CI shouldn't have any caching in it (@emersonknapp any caching happen in the ros action ci?) for it to know about BT.CPPv4 if its gone here. Does this cause you issues locally?

tonynajjar commented 7 months ago

Does this cause you issues locally?

Locally on rolling no, I didn't test on iron though, I'm on humble and the devcontainer is only for rolling I believe - setting up iron will be some effort

tonynajjar commented 7 months ago

That is odd - this CI shouldn't have any caching in it (@emersonknapp any caching happen in the ros action ci?) for it to know about BT.CPPv4 if its gone here

@SteveMacenski I'm guessing it knows about BT.CPPv4 because it's using iron. I tried using rolling in this commit but that fails as well because it can't find nav2_util -> are there no binaries for nav2 on rolling?

SteveMacenski commented 7 months ago

There is no Nav2 binaries for rolling, but you could add it in the https://github.com/open-navigation/opennav_coverage/blob/main/.github/deps.repos for use in CI for testing. I'm not sure if you can isolate it to only nav2_utils and its necessary dependencies, but I suppose we could change the Action's colcon build to include packages-up-to for the opennav coverage packages, which would include only the parts of Nav2 strictly required. It would though need to build the Nav2 BT libraries which is non-trivial.

SteveMacenski commented 7 months ago

-_- action ci tools don't call rosdep, I literally can't believe that

tonynajjar commented 7 months ago

-_- action ci tools don't call rosdep, I literally can't believe that

hmm I think it does image

but for some reason it can't find "standard" packages: https://github.com/open-navigation/opennav_coverage/actions/runs/8175554856/job/22352920043?pr=43

tonynajjar commented 7 months ago

I'm wondering if https://discourse.ros.org/t/preparing-ros-2-rolling-for-the-transition-to-ubuntu-24-04/35673/8 could have anything to do with it. It still says Jammy in the CI though No definition of [geometry_msgs] for OS version [jammy]

tonynajjar commented 7 months ago

I'm wondering if https://discourse.ros.org/t/preparing-ros-2-rolling-for-the-transition-to-ubuntu-24-04/35673/8 could have anything to do with it. It still says Jammy in the CI though No definition of [geometry_msgs] for OS version [jammy]

Now I have a stronger suspicion:

root@tony-xmg-22:/opt/overlay_ws# rosdep resolve geometry_msgs
#apt
ros-rolling-geometry-msgs
root@tony-xmg-22:/opt/overlay_ws# rosdep update
reading in sources list data from /etc/ros/rosdep/sources.list.d
Warning: running 'rosdep update' as root is not recommended.
  You should run 'sudo rosdep fix-permissions' and invoke 'rosdep update' again without sudo.
Hit https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/osx-homebrew.yaml
Hit https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/base.yaml
Hit https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/python.yaml
Hit https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/ruby.yaml
Hit https://raw.githubusercontent.com/ros/rosdistro/master/releases/fuerte.yaml
Query rosdistro index https://raw.githubusercontent.com/ros/rosdistro/master/index-v4.yaml
Skip end-of-life distro "ardent"
Skip end-of-life distro "bouncy"
Skip end-of-life distro "crystal"
Skip end-of-life distro "dashing"
Skip end-of-life distro "eloquent"
Skip end-of-life distro "foxy"
Skip end-of-life distro "galactic"
Skip end-of-life distro "groovy"
Add distro "humble"
Skip end-of-life distro "hydro"
Skip end-of-life distro "indigo"
Add distro "iron"
Skip end-of-life distro "jade"
Skip end-of-life distro "kinetic"
Skip end-of-life distro "lunar"
Skip end-of-life distro "melodic"
Add distro "noetic"
Add distro "rolling"
updated cache in /root/.ros/rosdep/sources.cache
root@tony-xmg-22:/opt/overlay_ws# rosdep resolve geometry_msgs

ERROR: No definition of [geometry_msgs] for OS version [jammy]

No definition of [geometry_msgs] for OS version [jammy]
    rosdep key : geometry_msgs
    OS name    : ubuntu
    OS version : jammy
    Data:
_is_ros: true
        debian:
          bookworm:
            apt:
              packages:
              - ros-rolling-geometry-msgs
        osx:
          homebrew:
            packages:
            - ros/rolling/common_interfaces
        rhel:
          '9':
            dnf:
              packages:
              - ros-rolling-geometry-msgs
        ubuntu:
          noble:
            apt:
              packages:
              - ros-rolling-geometry-msgs

What a timing :smile:

SteveMacenski commented 7 months ago

Oh I see that too. So I guess transient during the Rolling migration that's underway, I see the flood of build farm emails with their work to 24.04. What's the path from here? I think this was just a test to see if it would resolve on rolling since Iron was problematic. Certainly we could move the CI to rolling too, I could roll with that :wink:

SteveMacenski commented 7 months ago

By the way, this is not just a CI problem, I see this locally as well after doing an apt update + upgrade using the BTv4 branch. I actually see the 'all the BT nodes complaining' in opennav_docking as well now after the update which is interesting.

SteveMacenski commented 7 months ago

Experimenting now on the odd BT compiling issues. Did you happen to make any progress on it? I'm starting with purging v3 from my computer so its only v4. I suspect that has something to do with it.

tonynajjar commented 7 months ago

Mmm no progress as I didn't setup Iron. On my setup on rolling there wass only BTv4 so I couldn't reproduce locally. You were able to reproduce on iron right?

SteveMacenski commented 7 months ago

I don't recall specifically what my setup was when I saw it, I was in the middle of doing something else so I just removed opennav_coverage and went back to what I was trying to do at the time when I ran into it. Its possible I had nav2 main built on iron base (which could have had btv4 in it) or some other mixed policy. I was poking around alot of projects this week so I can't recall which specifically it was I found it with respect to.

What I know I did right now:

Doesn't happen for one of the projects, I adapted that here as well now too in a separate branch with just the v4 changes and that works too. I'll open a separate PR in a moment with just the v4 updates to see what CI says about that and that I have something working locally.

I'm guessing it has to do with having the v3 and v4 installed next to each other

SteveMacenski commented 7 months ago

Ah, in CI

executing command [apt-get install -y ros-iron-nav2-behavior-tree]

That's going to pull in BT.CPPv3. Then we're using v4 that would explain it (in my PR)

tonynajjar commented 7 months ago

Yes and BTv3 is expected in Iron so the simplest solution I see is to port the CI to rolling when rosdep works again... Do you see another option?

SteveMacenski commented 7 months ago

Note that you missed adding BTCPP_format="4" to the BT XML + the inline test XML string which causes a warning on execution. Please add that and I'll close my PR (only difference)

I see is to port the CI to rolling when rosdep works again

I agree. If I'm reading the docs for action-ros-ci correctly, we can use colcon-extra-args: --packages-up-to <packages in repo> to automatically only build what is needed from Nav2. Else, we can specify exactly which packages to build with package-name:, but we'll need to figure out exactly which packages we need to build manually. Which, from a quick look, seems like ~46 packages which would be a pain in the ass to list.

https://github.com/ros-tooling/action-ros-ci/blob/634e9ac5fb970763bd6207e31e1875d0081be71e/action.yml#L31-L34

I also filed https://github.com/ros-tooling/setup-ros-docker/issues/68 which is necessary to upgrade the CI to use 24.04 (in case 22.04 is totally messed up permanently, which would be really irritating). I asked for clarity in https://discourse.ros.org/t/preparing-ros-2-rolling-for-the-transition-to-ubuntu-24-04/35673/9?u=smac. I suppose either way we need to update CI to 24.04, so I suppose if that comes, we should just do that anyway.

I think that's the path forward and the contingencies, do you agree? I didn't want to make this your problem so I made sure to look at this today.

tonynajjar commented 7 months ago

Note that you missed adding BTCPP_format="4" to the BT XML + the inline test XML string which causes a warning on execution. Please add that and I'll close my PR (only difference)

Done

https://github.com/ros-tooling/action-ros-ci/blob/634e9ac5fb970763bd6207e31e1875d0081be71e/action.yml#L31-L34

ah nice find, there's nothing about it in the readme

I think that's the path forward and the contingencies, do you agree?

yep sounds like a plan. Let's see what they'll answer in the discourse

I'm looking forward to this moving in navigation2 (if that's gonna happen) to manage it more easily and avoid the double work.

SteveMacenski commented 7 months ago

Yeah the plan is to move it to Nav2 once the F2C updated features are in F2C & updated to be used here. I still haven't decided if I will rename these to use nav2_ prefix or keep opennav_, but that's pedantic

tonynajjar commented 6 months ago

I tried again since rolling sync is out but still same issue with rosdep not finding some packages. Unfortunately I don't have the time currently to look further into it, could you maybe ask for a follow up on the issue?

SteveMacenski commented 6 months ago

It still failing everywhere, I wouldn't worry about it yet. I still see missing rosdep in Nav2 and tickets on discourse, I don't think anything changed