Skip to content

install flake8 and pytest plugins from apt instead of pip #641

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.

Sign up for GitHub

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

Merged
mikaelarguedas merged 1 commit into from
Oct 6, 2022
Merged

install flake8 and pytest plugins from apt instead of pip #641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions ros2/nightly/nightly/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,18 @@ RUN apt-get update && apt-get install -q -y --no-install-recommends \
gnupg2 \
libssl-dev \
lsb-release \
python3-flake8 \
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 \
Copy link

@nachovizzo nachovizzo Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@mikaelarguedas , @ruffsl Sorry to jump out of nowhere here. But few doubts about the linter flake8-quotes. For the moment I will drop it here in case someone else out there is looking into this as well, soon will open a discussion in the forums

  1. Is there any good reason why we are using it!?
  2. Where can I find the original motivation for adding this?
  3. Where can I propose to completely get rid of such linter 🤓 ?

Thanks in advance and sorry for the annoyance!

Copy link
Contributor Author

@mikaelarguedas mikaelarguedas Nov 6, 2023

Choose a reason for hiding this comment

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

Hi @nachovizzo,

This is not something that is decided on the docker images maintaining side.
This is part of the ROS 2 styleguide: https://docs.ros.org/en/humble/The-ROS2-Project/Contributing/Code-Style-Language-Versions.html?highlight=style%20guide#id4 specifically this line:
"We pick single quotes over double quotes as long as no escaping is necessary."

I think the topic came up a couple times, I remember a thread on discourse a while back to compatible with other linters e.g. black https://discourse.ros.org/t/python-black-formatting/13277

Not sur if this is a satisfactory answer but that's a start :)

Copy link

@nachovizzo nachovizzo Nov 6, 2023

Choose a reason for hiding this comment

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

@mikaelarguedas 🙌 Thanks! This is what I was missing , this entry point. Thanks a lot

python3-pip \
python3-pytest-repeat \
python3-pytest-rerunfailures \
wget \
&& rm -rf /var/lib/apt/lists/*

Expand Down Expand Up @@ -46,18 +57,7 @@ ENV ROSDISTRO_INDEX_URL https://raw.githubusercontent.com/osrf/docker_images/mas

# install python packages
RUN pip3 install -U \
argcomplete \
flake8 \
flake8-blind-except \
flake8-builtins \
flake8-class-newline \
flake8-comprehensions \
flake8-deprecated \
flake8-docstrings \
flake8-import-order \
flake8-quotes \
pytest-repeat \
pytest-rerunfailures
argcomplete
Copy link
Member

@ruffsl ruffsl Oct 4, 2022

Choose a reason for hiding this comment

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

Is there still a reason that argcomplete is soloed out?

Looks like its also available upstream:
https://packages.ubuntu.com/jammy/python/python3-argcomplete

I couldn't find what the build farm does for this, given argcomplete probably isn't used by the buildfarm.

Copy link
Contributor Author

@mikaelarguedas mikaelarguedas Oct 6, 2022

Choose a reason for hiding this comment

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

For a long time the version of argcomplete was too low for several ros2cli and/or colcon features.
Maybe things have evolved since. Looks like in official documentation it switched to apt for that as well: https://github.com/ros2/ros2_documentation/pull/1275/files.

On the other hand if the list of pip packages to install is empty the template will stop installing pip or setuptools altogether that may have side effects so I'd rather not do this here and solely fix the flake8 issue in this PR

Copy link
Member

@ruffsl ruffsl Oct 6, 2022

Choose a reason for hiding this comment

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

Sounds good.

# This is a workaround for pytest not found causing builds to fail
# Following RUN statements tests for regression of https://github.com/ros2/ros2/issues/722
RUN pip3 freeze | grep pytest \
Expand Down
25 changes: 12 additions & 13 deletions ros2/source/devel/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,18 @@ RUN apt-get update && apt-get install -q -y --no-install-recommends \
dirmngr \
gnupg2 \
lsb-release \
python3-flake8 \
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 \
python3-pip \
python3-pytest-repeat \
python3-pytest-rerunfailures \
&& rm -rf /var/lib/apt/lists/*

# setup sources.list
Expand All @@ -39,19 +50,7 @@ RUN apt-get update && apt-get install --no-install-recommends -y \

# install python packages
RUN pip3 install -U \
argcomplete \
flake8 \
flake8-blind-except \
flake8-builtins \
flake8-class-newline \
flake8-comprehensions \
flake8-deprecated \
flake8-docstrings \
flake8-import-order \
flake8-quotes \
pytest-cov \
pytest-repeat \
pytest-rerunfailures
argcomplete
# This is a workaround for pytest not found causing builds to fail
# Following RUN statements tests for regression of https://github.com/ros2/ros2/issues/722
RUN pip3 freeze | grep pytest \
Expand Down
22 changes: 11 additions & 11 deletions ros2/source/images.yaml.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,19 @@ images:
- docker_templates
upstream_packages:
- bash-completion
- python3-flake8
- 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
- python3-pytest-repeat
- python3-pytest-rerunfailures
pip3_install:
- argcomplete
- flake8
- flake8-blind-except
- flake8-builtins
- flake8-class-newline
- flake8-comprehensions
- flake8-deprecated
- flake8-docstrings
- flake8-import-order
- flake8-quotes
- pytest-repeat
- pytest-rerunfailures
ws: /opt/ros2_ws
source:
maintainer_name: @(maintainer_name)
Expand Down