Resolve "python_syntax is python2 only"

Closes #48 (closed)

assigned to @jon.azpiazu

added 1 commit

• 470bc229 - New jobs to substitute the old Python check

Compare with previous version

requested review from @inigo.moreno

I have added the same jobs that I tested in https://git.code.tecnalia.com/tecnalia_robotics/flexbotics/flexbotics_test/dummy_application/-/pipelines/75661

marked this merge request as ready

Doesn't this break on python2 only repos? I'll have a check on the manipulation repo...

Hmm, I checked with this job in manipulation_utlis.

Even though the job fails, it is not because of what I expected. In that repo there are some iterator stuff that only works in python2 (stuff like (g for g in groups if g.name == group).next()). That does not seem to fail, however, other stuff fails.

One of the things that fails that worries me is this:

./flexbotics_manipulation_utils/setup.py:1:1: E266 too many leading '#' for block comment
## ! DO NOT MANUALLY INVOKE THIS setup.py, USE CATKIN INSTEAD
^
./flexbotics_manipulation_utils/setup.py:14:1: W391 blank line at end of file
^

This is an error and a warning on the setup.py file, which is a file that is automatically generated by catkin. Maybe we should add an exception for those two rules, or to exclude setup.py files.

Why are you specially worried about it? Even if it is generated, it can be modified, it does not get overwritten or anything similar.

We could either change that line to:

## ! DO NOT MANUALLY INVOKE THIS setup.py, USE CATKIN INSTEAD # noqa: E266

or

# ! DO NOT MANUALLY INVOKE THIS setup.py, USE CATKIN INSTEAD

I reckon there is a large number of files to be modified, but it is quite trivial to be done.

Maybe we should add an exception for those two rules, or to exclude setup.py files.

An exception would apply to every file, so I do not think it is acceptable.

Excluding setup.py files also sounds like a bad idea, as other checks might pop out useful things.

I think it's a bit harsh to force everyone to change that line of code whenever a package is created.

An exception would apply to every file, so I do not think it is acceptable.

I do think that is acceptable, it's just for one error which I think is not that important (Using more than one # for comments)

Btw I was checking how Boeing tackles this, and they are actually modifying every setup.py file:

I would in general favor ignoring such stylistic errors which trigger from autogenerated files, but I'm not quite sure that's the case here. Just did a quick test and neither catkin create pkg nor catkin_create_pkg generate a setup.py for me.

On the other hand, irrespective of whether that's autogenerated or just copy-pasted, introducing this check is going to make existing projects' pipelines to start failing.

What about an in-between solution: exclude E266 and W391 from .flake8_template and leave them in .flake8_extended_template. And possibly add them back to .flake8_template further down the line if you prefer not to keep them excluded?

Also, why does this run on the .post stage instead of the build stage?

Looking at your pipeline setting the python_security job as allowed to fail (for the moment) might be a good idea.

Opening a new discussion from !80 (comment 113888) as that thread changed to a different topic.

I see you are using a python3-alpine image, does that mean the checks are on python3? Shouldn't we keep it as python2? Or have two jobs one for python2 and one for python3 that run depending on which distro is targeted...

added 1 commit

• df025d18 - Make jobs python2 or python3 depending on distro

Compare with previous version

added 1 commit

• eb815306 - Fix yaml-lint on python.yml

Compare with previous version

Hi folks I was wondering if, since last year, you implemented an alternative to check python 3 code, or whether you somehow deactivate this check for recent python developments. Thanks!

added 19 commits

• eb815306...c5cc1e39 - 18 commits from branch master
• ff8b10d6 - Merge branch 'master' into 48-python_syntax-is-python2-only

Compare with previous version

I don't understand the logic in the usage of when: on the jobs here. That is:

• In ci-templates/python.yml
  • .py3_template has a when: never but .py2_template has a when: always.
  • .security_template has a when: always.
• In ci-templates/auto-rules/no-default.yml
  • .py3_template has a when: always and .py2_template has nothing.

Is all of this intentional or is are these just leftovers from trial-and-errors?

closed

reopened

A quick test shows that including core.yml instead of any of the entry points in auto-rules results in an invalid YAML being generated. Specific error I get is:

jobs:py3-flake8 when should be one of: on_success, on_failure, always, manual, delayed

Still need to wrap my head around the rules logic, but leaving this here before I give up and later forget.

added 1 commit

• 8c0a161e - Add 'humble' into distro-based python job selection logic

Compare with previous version