search

ament / ament_lint

Apache License 2.0
38 stars 107 forks source link

Add additional dependencies to ament_flake8. #454

Closed clalancette closed 1 year ago

clalancette commented 1 year ago

The ROS 2 style guides and CI depend on all of the following flake8 plugins:

python3-flake8-blind-except python3-flake8-builtins python3-flake8-class-newline python3-flake8-comprehensions python3-flake8-deprecated python3-flake8-docstrings python3-flake8-import-order python3-flake8-quotes

However, we have classically not directly depended on these anywhere. Due to the plugin nature of flake8, this means that the tests are just skipped if a user runs the tests without them installed. In turn, that means the errors only show up when we run CI against their code, leading to user frustration.

To alleviate that, directly depend on the flake8 plugins that we expect to exist. This will ensure that they are automatically installed when users do an installation. The tradeoff is that we'll increase the installation size for everyone by about 1MB, since ament_flake8 is one of the core packages.

One other note is that 3 of the plugins are (still) disabled because we have no packages on RHEL-9; that is being worked on and we'll enable those once those packages are available.

@nuclearsandwich @cottsay @tfoote FYI. Note that one other way we could consider doing this is to tie these packages to ros-dev-tools instead. The benefit would be keeping install size lower for non-development machines. The downside would be that we don't have ros-dev-tools on RHEL, so it wouldn't solve the issue there. Happy to hear additional thoughts about it.

clalancette commented 1 year ago

CI:

clalancette commented 1 year ago

In general this should be a test dependency for packages. So it should not significantly expand a deployments footprint.

Unfortunately, that is not quite true due to a recent change. In particular, we recently merged in https://github.com/ros2/rosidl_python/pull/199 , which marks ament_cmake_flake8 as an exec_depend. What we really wanted to do was mark it as a test_export_depend, but we don't have that feature in package.xml . So in the interim we marked it as an exec_depend. That means that this will expand our core footprint by the 1.1MB I found earlier. Do you still think this is OK?

clalancette commented 1 year ago

Despite the downside, I'm going to go ahead with this which should alleviate some of the issues. If we find that this is really a problem, we can revert it later on (though in that case, we should come up with an alternate strategy).