| Message ID | 20231113133337.767803-1-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] py-requirements: Remove hacking dependency and use recent flake8. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 11/13/23 14:33, Dumitru Ceara wrote: > Without this, when using Python 3.12 and flake8 5.0.4, the following > errors are flagged: > tests/check_acl_log.py:97:25: E231 missing whitespace after ':' > tests/check_acl_log.py:102:71: E231 missing whitespace after ':' > > This was reported and discussed in a couple of contexts: > https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409325.html > https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409277.html > > And it's fixed in recent flake8/pycodestyle versions: > https://github.com/PyCQA/flake8/issues/1845#issuecomment-1766073353 > > Unfortunately we have to remove the 'hacking' requirement because that > introduces a dependency on 'flake8<4.0.0 and >=3.6.0'. That should be > OK though because 'hacking' is an OpenStack specific package and OVN > doesn't expose any Python code. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > NOTE: this patch should be backported to all supported branches. > --- > utilities/containers/py-requirements.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/utilities/containers/py-requirements.txt b/utilities/containers/py-requirements.txt > index 0d90765c97..a8e8f17da3 100644 > --- a/utilities/containers/py-requirements.txt > +++ b/utilities/containers/py-requirements.txt > @@ -1,7 +1,7 @@ > -flake8 > -hacking>=3.0 > +flake8>=6.1.0 > scapy > sphinx > setuptools > pyelftools > pyOpenSSL > +pycodestyle>=2.11.0 Alternative might be to still install hacking for python <3.12, but do not otherwise. See https://peps.python.org/pep-0508/#environment-markers Best regards, Ilya Maximets.
On 11/13/23 14:41, Ilya Maximets wrote: > On 11/13/23 14:33, Dumitru Ceara wrote: >> Without this, when using Python 3.12 and flake8 5.0.4, the following >> errors are flagged: >> tests/check_acl_log.py:97:25: E231 missing whitespace after ':' >> tests/check_acl_log.py:102:71: E231 missing whitespace after ':' >> >> This was reported and discussed in a couple of contexts: >> https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409325.html >> https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409277.html >> >> And it's fixed in recent flake8/pycodestyle versions: >> https://github.com/PyCQA/flake8/issues/1845#issuecomment-1766073353 >> >> Unfortunately we have to remove the 'hacking' requirement because that >> introduces a dependency on 'flake8<4.0.0 and >=3.6.0'. That should be >> OK though because 'hacking' is an OpenStack specific package and OVN >> doesn't expose any Python code. >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> NOTE: this patch should be backported to all supported branches. >> --- >> utilities/containers/py-requirements.txt | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/utilities/containers/py-requirements.txt b/utilities/containers/py-requirements.txt >> index 0d90765c97..a8e8f17da3 100644 >> --- a/utilities/containers/py-requirements.txt >> +++ b/utilities/containers/py-requirements.txt >> @@ -1,7 +1,7 @@ >> -flake8 >> -hacking>=3.0 >> +flake8>=6.1.0 >> scapy >> sphinx >> setuptools >> pyelftools >> pyOpenSSL >> +pycodestyle>=2.11.0 > > > Alternative might be to still install hacking for python <3.12, > but do not otherwise. > > See https://peps.python.org/pep-0508/#environment-markers > I'm not sure what benefit we get from 'hacking' to be honest. OVN uses python only in ovn-detrace (the rest of the python code is test code) and that is not exported as a package to anyone else to use. Personally, I'd just remove it (hence the patch :D) but I'll wait for more input on this. In the meantime our CI is broken for 23.06. Regards, Dumitru > Best regards, Ilya Maximets. >
On 11/13/23 14:47, Dumitru Ceara wrote: > On 11/13/23 14:41, Ilya Maximets wrote: >> On 11/13/23 14:33, Dumitru Ceara wrote: >>> Without this, when using Python 3.12 and flake8 5.0.4, the following >>> errors are flagged: >>> tests/check_acl_log.py:97:25: E231 missing whitespace after ':' >>> tests/check_acl_log.py:102:71: E231 missing whitespace after ':' >>> >>> This was reported and discussed in a couple of contexts: >>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409325.html >>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409277.html >>> >>> And it's fixed in recent flake8/pycodestyle versions: >>> https://github.com/PyCQA/flake8/issues/1845#issuecomment-1766073353 >>> >>> Unfortunately we have to remove the 'hacking' requirement because that >>> introduces a dependency on 'flake8<4.0.0 and >=3.6.0'. That should be >>> OK though because 'hacking' is an OpenStack specific package and OVN >>> doesn't expose any Python code. >>> >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> --- >>> NOTE: this patch should be backported to all supported branches. >>> --- >>> utilities/containers/py-requirements.txt | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/utilities/containers/py-requirements.txt b/utilities/containers/py-requirements.txt >>> index 0d90765c97..a8e8f17da3 100644 >>> --- a/utilities/containers/py-requirements.txt >>> +++ b/utilities/containers/py-requirements.txt >>> @@ -1,7 +1,7 @@ >>> -flake8 >>> -hacking>=3.0 >>> +flake8>=6.1.0 >>> scapy >>> sphinx >>> setuptools >>> pyelftools >>> pyOpenSSL >>> +pycodestyle>=2.11.0 >> >> >> Alternative might be to still install hacking for python <3.12, >> but do not otherwise. >> >> See https://peps.python.org/pep-0508/#environment-markers >> > > I'm not sure what benefit we get from 'hacking' to be honest. OVN uses > python only in ovn-detrace (the rest of the python code is test code) > and that is not exported as a package to anyone else to use. > > Personally, I'd just remove it (hence the patch :D) but I'll wait for > more input on this. In the meantime our CI is broken for 23.06. The problem with that is that you're only removing hacking from CI, but not from the documentation or a build process. So, CI will no longer cover it, but users may have build failures locally. Best regards, Ilya Maximets.
On 11/13/23 15:15, Ilya Maximets wrote: > On 11/13/23 14:47, Dumitru Ceara wrote: >> On 11/13/23 14:41, Ilya Maximets wrote: >>> On 11/13/23 14:33, Dumitru Ceara wrote: >>>> Without this, when using Python 3.12 and flake8 5.0.4, the following >>>> errors are flagged: >>>> tests/check_acl_log.py:97:25: E231 missing whitespace after ':' >>>> tests/check_acl_log.py:102:71: E231 missing whitespace after ':' >>>> >>>> This was reported and discussed in a couple of contexts: >>>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409325.html >>>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409277.html >>>> >>>> And it's fixed in recent flake8/pycodestyle versions: >>>> https://github.com/PyCQA/flake8/issues/1845#issuecomment-1766073353 >>>> >>>> Unfortunately we have to remove the 'hacking' requirement because that >>>> introduces a dependency on 'flake8<4.0.0 and >=3.6.0'. That should be >>>> OK though because 'hacking' is an OpenStack specific package and OVN >>>> doesn't expose any Python code. >>>> >>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>>> --- >>>> NOTE: this patch should be backported to all supported branches. >>>> --- >>>> utilities/containers/py-requirements.txt | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/utilities/containers/py-requirements.txt b/utilities/containers/py-requirements.txt >>>> index 0d90765c97..a8e8f17da3 100644 >>>> --- a/utilities/containers/py-requirements.txt >>>> +++ b/utilities/containers/py-requirements.txt >>>> @@ -1,7 +1,7 @@ >>>> -flake8 >>>> -hacking>=3.0 >>>> +flake8>=6.1.0 >>>> scapy >>>> sphinx >>>> setuptools >>>> pyelftools >>>> pyOpenSSL >>>> +pycodestyle>=2.11.0 >>> >>> >>> Alternative might be to still install hacking for python <3.12, >>> but do not otherwise. >>> >>> See https://peps.python.org/pep-0508/#environment-markers >>> >> >> I'm not sure what benefit we get from 'hacking' to be honest. OVN uses >> python only in ovn-detrace (the rest of the python code is test code) >> and that is not exported as a package to anyone else to use. >> >> Personally, I'd just remove it (hence the patch :D) but I'll wait for >> more input on this. In the meantime our CI is broken for 23.06. > > The problem with that is that you're only removing hacking from CI, > but not from the documentation or a build process. So, CI will no > longer cover it, but users may have build failures locally. > Ack, you're right, I should be updating the documentation and build too. For the latter, all the checks we currently enable are: # H231 Python 3.x incompatible 'except x,y:' construct # H232 Python 3.x incompatible octal 077 should be written as 0o77 # H233 Python 3.x incompatible use of print operator # H238 old style class declaration, use new style (inherit from `object`) FLAKE8_SELECT = H231,H232,H233,H238 These are skipped if the interpreter is Python 2. All other hacking checks are ignored (it's part of FLAKE8_IGNORE). We explicitly require Python 3 support and we don't support Python 2 anymore: https://github.com/ovn-org/ovn/commit/0c042c2d28d87b90300ca86fe67427dab908134c It's safe to remove all hacking checks from the build. I'll do that in v2. While we're at it I guess we should do the same thing for OVS. What do you think? Thanks, Dumitru
On 11/13/23 17:23, Dumitru Ceara wrote: > On 11/13/23 15:15, Ilya Maximets wrote: >> On 11/13/23 14:47, Dumitru Ceara wrote: >>> On 11/13/23 14:41, Ilya Maximets wrote: >>>> On 11/13/23 14:33, Dumitru Ceara wrote: >>>>> Without this, when using Python 3.12 and flake8 5.0.4, the following >>>>> errors are flagged: >>>>> tests/check_acl_log.py:97:25: E231 missing whitespace after ':' >>>>> tests/check_acl_log.py:102:71: E231 missing whitespace after ':' >>>>> >>>>> This was reported and discussed in a couple of contexts: >>>>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409325.html >>>>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409277.html >>>>> >>>>> And it's fixed in recent flake8/pycodestyle versions: >>>>> https://github.com/PyCQA/flake8/issues/1845#issuecomment-1766073353 >>>>> >>>>> Unfortunately we have to remove the 'hacking' requirement because that >>>>> introduces a dependency on 'flake8<4.0.0 and >=3.6.0'. That should be >>>>> OK though because 'hacking' is an OpenStack specific package and OVN >>>>> doesn't expose any Python code. >>>>> >>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>>>> --- >>>>> NOTE: this patch should be backported to all supported branches. >>>>> --- >>>>> utilities/containers/py-requirements.txt | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/utilities/containers/py-requirements.txt b/utilities/containers/py-requirements.txt >>>>> index 0d90765c97..a8e8f17da3 100644 >>>>> --- a/utilities/containers/py-requirements.txt >>>>> +++ b/utilities/containers/py-requirements.txt >>>>> @@ -1,7 +1,7 @@ >>>>> -flake8 >>>>> -hacking>=3.0 >>>>> +flake8>=6.1.0 >>>>> scapy >>>>> sphinx >>>>> setuptools >>>>> pyelftools >>>>> pyOpenSSL >>>>> +pycodestyle>=2.11.0 >>>> >>>> >>>> Alternative might be to still install hacking for python <3.12, >>>> but do not otherwise. >>>> >>>> See https://peps.python.org/pep-0508/#environment-markers >>>> >>> >>> I'm not sure what benefit we get from 'hacking' to be honest. OVN uses >>> python only in ovn-detrace (the rest of the python code is test code) >>> and that is not exported as a package to anyone else to use. >>> >>> Personally, I'd just remove it (hence the patch :D) but I'll wait for >>> more input on this. In the meantime our CI is broken for 23.06. >> >> The problem with that is that you're only removing hacking from CI, >> but not from the documentation or a build process. So, CI will no >> longer cover it, but users may have build failures locally. >> > > Ack, you're right, I should be updating the documentation and build too. > > For the latter, all the checks we currently enable are: > # H231 Python 3.x incompatible 'except x,y:' construct > # H232 Python 3.x incompatible octal 077 should be written as 0o77 > # H233 Python 3.x incompatible use of print operator > # H238 old style class declaration, use new style (inherit from `object`) > FLAKE8_SELECT = H231,H232,H233,H238 > > These are skipped if the interpreter is Python 2. All other hacking > checks are ignored (it's part of FLAKE8_IGNORE). > > We explicitly require Python 3 support and we don't support Python 2 > anymore: > > https://github.com/ovn-org/ovn/commit/0c042c2d28d87b90300ca86fe67427dab908134c > > It's safe to remove all hacking checks from the build. I'll do that in > v2. While we're at it I guess we should do the same thing for OVS. > What do you think? Some of the checks are also not selected without --enable-extentions in the latest versions of flake8, and we do not supply it. Yeah, we should probably just remove the hacking from the OVS build as well, since all these are just python 2-3 compatibility checks. Best regards, Ilya Maximets.
diff --git a/utilities/containers/py-requirements.txt b/utilities/containers/py-requirements.txt index 0d90765c97..a8e8f17da3 100644 --- a/utilities/containers/py-requirements.txt +++ b/utilities/containers/py-requirements.txt @@ -1,7 +1,7 @@ -flake8 -hacking>=3.0 +flake8>=6.1.0 scapy sphinx setuptools pyelftools pyOpenSSL +pycodestyle>=2.11.0
Without this, when using Python 3.12 and flake8 5.0.4, the following errors are flagged: tests/check_acl_log.py:97:25: E231 missing whitespace after ':' tests/check_acl_log.py:102:71: E231 missing whitespace after ':' This was reported and discussed in a couple of contexts: https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409325.html https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409277.html And it's fixed in recent flake8/pycodestyle versions: https://github.com/PyCQA/flake8/issues/1845#issuecomment-1766073353 Unfortunately we have to remove the 'hacking' requirement because that introduces a dependency on 'flake8<4.0.0 and >=3.6.0'. That should be OK though because 'hacking' is an OpenStack specific package and OVN doesn't expose any Python code. Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- NOTE: this patch should be backported to all supported branches. --- utilities/containers/py-requirements.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)