ament / ament_lint

Apache License 2.0
38 stars 107 forks source link

ament_flake8 assumes, does not include, flake8-import-order #254

Open rotu opened 4 years ago

rotu commented 4 years ago

ament_flake8 specifies an import order style but does not depend on or install flake8-import-order. This makes the results dependent on runtime conditions. https://github.com/ament/ament_lint/blob/892c03c0363e356e38ce748d87046d8d3788baa1/ament_flake8/ament_flake8/configuration/ament_flake8.ini#L3

dirk-thomas commented 4 years ago

ament_flake8 specifies an import order style but does not depend on or install flake8-import-order. This makes the results dependent on runtime conditions.

Correct.

The limitation is that there is no Debian package for flake8-import-order atm and we don't want to take on the effort to create a vendor package for it. Therefore the install instructions mention to install it via pip (see https://index.ros.org/doc/ros2/Installation/Foxy/Linux-Development-Setup/#install-development-tools-and-ros-tools).

If you have any suggestion how to improve the situation please let us know. Otherwise I am going to close this ticket as wontfix.

rotu commented 4 years ago

It does no good to close the issue; if it's important enough to document the workaround in installation instructions, it's important enough to fix. My suggestion is to do one of the following:

  1. Vendor the package.
  2. Install it via pip as part of ament_flake8's setup.py.
  3. Add it as a pip system dependency and include it via package.xml.
dirk-thomas commented 4 years ago
  1. Install it via pip as part of ament_flake8's setup.py.
  2. Add it as a pip system dependency and include it via package.xml.

A Debian package can not depend on a package provided by pip. So your options 2. and 3. are technically not possible.

  1. Vendor the package.

And I already comment on option 1 and why we don't do that. If you would like to contribute and maintain such packages please feel free to do so.

rotu commented 4 years ago

And I already comment on option 1 and why we don't do that.

we don't want to take on the effort to create a vendor package for it.

I don't know who "we" is. Maybe the maintainer of this package, @nuclearsandwich, has the bandwidth. In theory, it doesn't look too hard to do it with vendorize or manually. But if there are already vendored python packages, it just makes sense to reuse whichever process was used for those.

nuclearsandwich commented 4 years ago

Maybe the maintainer of this package, @nuclearsandwich, has the bandwidth.

Most of my time right now is focused on distribution and infrastructure maintenance. For package maintenance I have other issues which have been queued longer in need of attention and I don't think this is urgent enough to pre-empt them.

With that in mind, I also feel the pain here. The most-preferred contribution would be assistance contributing flake8-import-order and other packages that are installed via pip in ROS 2's development instructions into Debian and Ubuntu so we can rely on the packaged versions directly rather than via vendor packages.

mateus-amarante commented 4 years ago

I think it also assumes (and doesn't include) the packages (defined in setup-ros github action):

DLu commented 2 years ago

So I understand properly, the existence of the import-order-style configuration implies a dependency on flake8-import-order.

Are the other flake8 packages @mateus-amarante mentioned above explicitly depended on somewhere?

mateus-amarante commented 2 years ago

If I remember correctly, I had to download these dependencies manually to replicate lint tests defined in the setup-ros GitHub action (defined here). I don't remember how the additional plugins affect the lint result, though.

Anyways, I'd say you shouldn't need to install any flake8 plugins in the setup-ros CI if the ament_flake8 had all dependencies properly declared.

nachovizzo commented 10 months ago

Is there any plan for solving this? This already fired in https://github.com/ros-planning/navigation2/pull/3893 and other places as well...

I'm happy to contribute to this