Install flake8 and flake8-import-order #3
Conversation
| @@ -48,7 +48,8 @@ | |||
| 'pep8', | |||
| 'pydocstyle', | |||
| 'pyflakes', | |||
| 'flake8', | |||
| 'flake8==2.6.2', | |||
There was a problem hiding this comment.
What is the reason to pin the version to 2.x instead of using the latest release - currently 3.0.4?
There was a problem hiding this comment.
version 3 was a complete re-write and doesn't have a stable API yet http://flake8.pycqa.org/en/latest/user/python-api.html
There's a shim to the 2.x API but I wasn't able to see how to overload the reporter as is needed here
I can watch the releases to see when an API gets added
There was a problem hiding this comment.
I would propose to still use the latest release. If they really change the API we will notice immediately and can update our usage.
There was a problem hiding this comment.
it's not straightforward to use the new release in ament/ament_lint#63 (there is no 3.x API, and the shim to the 2.x API doesn't expose what's needed).
I appreciate pinning to an old version isn't ideal (especially since one of the selling points of flake8 is its active development). if using 2.x is a blocker, I'll put ament/ament_lint#63 on the backburner until there is an API in 3.x since it might take me a bit to get 3.x to work
There was a problem hiding this comment.
I don't think it is that difficult: see dhood/ament_lint#1
| @@ -35,7 +35,7 @@ RUN if test ${PLATFORM} = x86; then curl --silent https://packagecloud.io/gpg.ke | |||
| RUN apt-get update && apt-get install -y build-essential ccache cmake pkg-config python3-empy python3-setuptools python3-vcstool | |||
|
|
|||
| # Install build and test dependencies of ROS 2 packages. | |||
| RUN apt-get update && apt-get install -y clang-format-3.8 cppcheck git pydocstyle pyflakes python3-coverage python3-mock python3-nose python3-pep8 uncrustify | |||
| RUN apt-get update && apt-get install -y clang-format-3.8 cppcheck git python3-coverage python3-mock python3-nose uncrustify | |||
There was a problem hiding this comment.
I don't think the existing linter dependencies should be removed. Every package should be able to choose what linters it would like to use and I don't see a reason to delete the linter packages we already have.
There was a problem hiding this comment.
they are being installed through pip in this script. installing them here is what is causing version conflicts I believe (see http://ci.ros2.org/view/All/job/ci_linux/1802/testReport/junit/(root)/projectroot/flake8/).
There was a problem hiding this comment.
Should then all entries be removed for Python packages which are being installed via PIP?
In general we try to use the Debian packages whenever possible and "sufficient".
There was a problem hiding this comment.
I think the idea was to install what ever you could from apt, but everything must be installed in the Python script so that OS X and Windows get the Python dependencies.
I'm agnostic as to whether or not we use the apt version of the packages, though I suppose that might be more like a "typical" user's setup.
There was a problem hiding this comment.
I think installing python packages from pip where possible would be better, because 1) better version requirement resolution and 2) consistent version of packages on different platforms. flake8, for example, will be installed as v3 from pypi, but it is only released as v2 in xenial
There was a problem hiding this comment.
The reason why we recommend not to use PIP packages in ROS is that they overlay the Debian packages and are not being updated by the "native" package manager run on the system (aka apt).
There was a problem hiding this comment.
I can see the motivation for using apt instead of pip. I have just had apt suggest installing flake8 despite already having flake8 installed through pip.
If we want to stick with packages installed through apt, in the case of flake8 we'll need to either pin the batch job script to use v 2.x again, or support 2.x and 3.x simultaneously. (apt will install 2.5.4 while pip will install 3.0.4)
|
Ready for review again please. to summarise the conversation: We were discussing installing flake8 through pip or apt. apt installs flake8 v2 on xenial and pip installs v3. Because of this, this PR relies on flake8 being installed only through pip. the import order flake8 plugin is not available through apt. |
dhood
added
in review
|
since ament/ament_lint#63 can work with flake8 v2 or v3 now (see ament/ament_lint#63 (comment)), I will add flake8 installation through apt back into this PR. I haven't done it yet so that it's clear what commits are included in this PR when running CI jobs for ament/ament_lint#63. |
| @@ -45,14 +45,15 @@ | |||
| sys.stderr = UnbufferedIO(sys.stderr) | |||
|
|
|||
| pip_dependencies = [ | |||
| 'EmPy', | |||
There was a problem hiding this comment.
should dependencies be listed in alphabetical order regardless of the case? (it seems that's how flake8 import order interprets alphabetical order according to the fixes in the attached PRs e.g. https://github.com/ros2/rosidl_dds/pull/40/files)
There was a problem hiding this comment.
yep you're right that that's how the 'google-style' for flake8-import-order does it. I think it might be more common to sort considering case, so I'll leave it like this unless there are objections
connects to ament/ament_lint#53
needed by ament/ament_lint#63