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

Add additional dependencies to ament_flake8. #454

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

clalancette
Copy link
Contributor

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.

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.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

In general this should be a test dependency for packages. So it should not significantly expand a deployments footprint. Having these capabilities enabled by default provides a better user experience.

Also a large part of the reason we had the optional pattern was that we needed the latest version from pip and it wasn't available from apt. Now that they are available it's good to bring them in.

This will also improve our installation docs reducing the separate paths necessary for development.

@clalancette
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor Author

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 ros2/rosidl_python#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
Copy link
Contributor Author

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).

@clalancette clalancette merged commit 25e61b4 into rolling Oct 3, 2023
@delete-merged-branch delete-merged-branch bot deleted the clalancette/flake8-deps branch October 3, 2023 18:17
wuisky pushed a commit to wuisky/ament_lint that referenced this pull request Feb 7, 2024
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.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: WU MENGHUNG <wu.menghung@jp.panasonic.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.

2 participants