Fix nav2_bringup flake8 tests proposal

[nachovizzo]

Hello, I'm opening a PR instead of a ticket so the pipeline is also triggered and we can discuss further

Problem: new flake8 plugins in ros:rolling make the CI fail

This sounds pretty strange, but the reason is quite simple:

The CI is using ghcr.io/ros-planning/navigation2:main as the base image, which is based on rolling.

By doing a simple docker run in such a container one can immediately see all the plugins that are installed there:

python3-flake8/now 4.0.1-2 all [installed,local]
python3-flake8-builtins/now 1.5.3-3 all [installed,local]
python3-flake8-comprehensions/now 3.8.0-1 all [installed,local]
python3-flake8-docstrings/now 1.6.0-1 all [installed,local]
python3-flake8-import-order/now 0.18.1-2 all [installed,local]
python3-flake8-quotes/now 3.3.1-2 all [installed,local]
ros-rolling-ament-cmake-flake8/now 0.16.0-1jammy.20231004.142753 amd64 [installed,local]
ros-rolling-ament-flake8/now 0.16.0-1jammy.20231004.141251 amd64 [installed,local]

In contrast, if one searches for the flake8 apt packages installed in the ros:iron image (as an example) the output is only:

python3-flake8/now 4.0.1-2 all [installed,local]
ros-iron-ament-cmake-flake8/now 0.14.2-1jammy.20230908.160003 amd64 [installed,local]
ros-iron-ament-flake8/now 0.14.2-1jammy.20230908.154814 amd64 [installed,local]

Why this happens

This is the thing I don't like much about flake8, the plugin systems. As is the case now, I can sudo apt install python3-flake8-quotes and the output of the flake8 linter is going to be different from the "vanilla" flake8.

How to reproduce the CI failure locally?

Easy:

sudo apt install \
  python3-flake8-builtins \
  python3-flake8-comprehensions \
  python3-flake8-docstrings \
  python3-flake8-import-order \
  python3-flake8-quotes

And after that just run ament_flake8 . on the navigation2 root, and you will see exactly the same errors that are on the CI.

CI Test

The commit I've pushed only fix the issues from the new flake8 plugins in the nav2_bringup package, as seen in the CI run now those tests pass: , compared to the current master branch failure:

  <<< failure message
    -- run_test.py: invoking following command in '/opt/overlay_ws/src/navigation2/nav2_bringup':
     - /opt/ros/rolling/bin/ament_flake8 --xunit-file /opt/overlay_ws/test_results/nav2_bringup/flake8.xunit.xml
    
    ./launch/bringup_launch.py:28:1: I101 Imported names are in the wrong order. Should be ReplaceString, RewrittenYaml
    from nav2_common.launch import RewrittenYaml, ReplaceString
    ^
    
    ./launch/cloned_multi_tb3_simulation_launch.py:17:1: I201 Missing newline between import groups. 'from ament_index_python.packages import get_package_share_directory' is identified as Third Party and 'import os' is identified as Stdlib.
    from ament_index_python.packages import get_package_share_directory
    ^
    
    1     I101 Imported names are in the wrong order. Should be ReplaceString, RewrittenYaml
    1     I201 Missing newline between import groups. 'from ament_index_python.packages import get_package_share_directory' is identified as Third Party and 'import os' is identified as Stdlib.
    
    8 files checked
    2 errors

Possible solutions

1. Address this in the ament_lint repo, disabling all the new stuff (especially the "quotes" linter is quite annoying)
2. Backtrace in rosdistro where those dependencies got added, and attempt to get rid of those (unlikely to happen)
3. Create a local ament_flake8.ini in nav2 and use that configuration in the ci
4. Fix all the new tests with the current state of the rolling image. I'm happy to do this, but it will take me some time

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

[SteveMacenski]

@nachovizzo thanks for starting, but CI still reports another 133 issues https://app.circleci.com/pipelines/github/ros-planning/navigation2/10248/workflows/00b8a38a-17fb-42ef-ab38-acab39d66001/jobs/32510/tests

We're not looking to disable anything - we want to be in compliance with the newest ROS 2 best practices as they evolve

[nachovizzo]

@nachovizzo thanks for starting, but CI still reports another 133 issues https://app.circleci.com/pipelines/github/ros-planning/navigation2/10248/workflows/00b8a38a-17fb-42ef-ab38-acab39d66001/jobs/32510/tests

We're not looking to disable anything - we want to be in compliance with the newest ROS 2 best practices as they evolve

@SteveMacenski I know. That's why I was asking how to proceed before spending hours changing double quotes for single quotes ;)

Then you would say to go for option 4 from the list in the PR?

[SteveMacenski]

Yes, and I totally understand!

[SteveMacenski]

Still fixes a few; I'll take incremental :-)

[SteveMacenski]

@nachovizzo is this something you're committing to doing? If not, I need to take it on. We can't leave CI red for so long.

[nachovizzo]

@SteveMacenski I was planning on doing it this weekend, probably tomorrow ;) the PR will come soon