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

Move Nav2 CI to 24.04 / Rolling #4298

Merged
merged 42 commits into from
May 28, 2024

Conversation

tonynajjar
Copy link
Contributor

@tonynajjar tonynajjar commented May 4, 2024


Basic Info

Info Please fill out this column
Ticket(s) this addresses (add tickets here #1)
Primary OS tested on (Ubuntu, MacOS, Windows)
Robotic platform tested on (Steve's Robot, gazebo simulation of Tally, hardware turtlebot)
Does this PR contain AI generated software? (No; Yes and it is marked inline in the code)

Description of contribution in a few bullet points

Various fixes for Nav2 Rolling on Noble

Description of documentation updates required from your changes


Future work that may be required in bullet points

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

tools/underlay.repos Show resolved Hide resolved
nav2_system_tests/CMakeLists.txt Outdated Show resolved Hide resolved
nav2_map_server/CMakeLists.txt Show resolved Hide resolved
nav2_common/cmake/nav2_package.cmake Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Show resolved Hide resolved
.devcontainer/devcontainer.json Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
nav2_mppi_controller/CMakeLists.txt Outdated Show resolved Hide resolved
nav2_common/cmake/nav2_package.cmake Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@SteveMacenski SteveMacenski requested a review from ruffsl May 6, 2024 17:49
Copy link
Member

@ruffsl ruffsl left a comment

Choose a reason for hiding this comment

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

I'll let @SteveMacenski comment on the CMake diffs, but only a few comments on the Docker/CI bits.

.devcontainer/devcontainer.json Show resolved Hide resolved
.devcontainer/devcontainer.json Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
nav2_map_server/CMakeLists.txt Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

mergify bot commented May 15, 2024

@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@SteveMacenski
Copy link
Member

SteveMacenski commented May 15, 2024

@tonynajjar my only beef left here is the cmake changes - I don't want to remove flags for our project due to dependency issues. Do you know how many files-ish (not exact; is it a lot or a little) have a problem due to this? If small, we can do what we did with other dependencies (example):

// turn off the specific warning. Can also use "-Wall"
#pragma GCC diagnostic ignored "-Wunused-but-set-variable"

#include <boost/uuid/uuid.hpp>
#include <boost/uuid/uuid_generators.hpp>
#include <boost/uuid/uuid_io.hpp>
#include <boost/lexical_cast.hpp>

// turn the warnings back on
#pragma GCC diagnostic pop

Though, I'm not sure why -finline-limit=10000000 was removed from MPPI?

Copy link
Contributor

mergify bot commented May 15, 2024

@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@tonynajjar
Copy link
Contributor Author

Okay, I restored the global compile flags and disabled them only where necessary. Currently we only know that the nav2_behavior_tree error will be resolved at the next sync. I'm not sure how to resolve the ones from nav2_system_tests and nav2_mppi_controller since the error originate from third-party code.

Copy link
Contributor

mergify bot commented May 15, 2024

@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@tonynajjar tonynajjar marked this pull request as ready for review May 15, 2024 21:06
@tonynajjar tonynajjar changed the title Attempt to solve docker image build Fix building on Rolling (Ubuntu noble) May 15, 2024
@SteveMacenski
Copy link
Member

At last review, this looks good to me. @ruffsl any thoughts?

Otherwise, CI is failing to build (still looks to be on Jammy?) but no more code-related concerns. I'll probably file a few tickets when we merge as reminders but nothing that I'm going to concern you with

@facontidavide
Copy link
Contributor

Behavior tree node failures RESOLVED - BT.CPP bug/regression causing crash if input configs don't contain all possible ports for the tests (CC @facontidavide: even if that was an intentional change, I expect that a crash isn't the expected behavior rather an exception or log. Though seems like maybe its a regression?)

Yes, I should fix it and have am exception instead.

@SteveMacenski
Copy link
Member

With the recent pushes, the remaining are the test bringdown failures which are just N instances of presumably the same problem. All the tests exhibit the Fsat-DDS shared memory log errors, but not blocking our CI motion to 24.04. Nor is the fact that there may be some issues (?) on bring down upstream once we find a workaround that gets us moving. But these probably should be followed up upon to those appropriate project leads for their awareness (which I've started).

Will be looking into the last thing this afternoon.

@SteveMacenski
Copy link
Member

SteveMacenski commented May 22, 2024

OK, fixed. It looks like another instance of the destruction not working like described in the WPF comment above. To my unsophisticated eye, it looks like some of the context / RMW items aren't still active during the pre-shutdown callbacks, causing exceptions / internal ROS 2 things to crash / deadlock. When we fully set things down so that our pre-shutdown callback doesn't have to do any work or is avoided altogether by destroying the object before the rclcpp shutdown, then we avoid the problem. I think this last, short one does a good job at highlighting this:

    [INFO] [1716413808.442739807] [costmap]: Running Nav2 LifecycleNode rcl preshutdown (costmap)
    [INFO] [1716413808.442783529] [costmap]: Destroying bond (costmap) to lifecycle manager.
    Failed to delete datawriter, at ./src/publisher.cpp:45 during '__function__'
    cannot publish data, at ./src/rmw_publish.cpp:66 during '__function__'
    Fail in delete datareader, at ./src/rmw_service.cpp:89 during '__function__'
    -- run_test.py: return code -11

@tonynajjar pull in my BR2 branch and this should be good to go pending any other issues that comes up from the CircleCI job versions of this

@SteveMacenski
Copy link
Member

@tonynajjar do you see the nav2_costmap_2d.DynParamTestNode test failure locally? I do not but CI is reporting it :(

@tonynajjar
Copy link
Contributor Author

@tonynajjar do you see the nav2_costmap_2d.DynParamTestNode test failure locally? I do not but CI is reporting it :(

Just tried on rolling locally 10 times and I didn't get it

@SteveMacenski
Copy link
Member

Maybe break the cache with v23? Or the docker image

- image: ghcr.io/ros-navigation/navigation2:main
?

Dockerfile Outdated Show resolved Hide resolved
@SteveMacenski SteveMacenski changed the title Fix building on Rolling (Ubuntu noble) Move Nav2 CI to 24.04 / Rolling May 28, 2024
.circleci/config.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Signed-off-by: Steve Macenski <stevenmacenski@gmail.com>
Signed-off-by: Steve Macenski <stevenmacenski@gmail.com>
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.

None yet

5 participants