Skip to content
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.

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

Fix GHA + support Python 3.11. #216

Merged
merged 4 commits into from
Dec 13, 2022
Merged

Fix GHA + support Python 3.11. #216

merged 4 commits into from
Dec 13, 2022

Conversation

icemac
Copy link
Member

@icemac icemac commented Dec 8, 2022

I had to disable flake8 checks, so isort-ing the imports is not enforced.

I had to disable flake8 checks, so isort-ing the imports is not enforced.
@icemac icemac requested a review from dataflake December 8, 2022 10:32
Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd prefer if we didn't disable flake8 unnecessarily


[tox]
use-flake8 = true
use-flake8 = false
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting. We had it enabled, so flake8 should be passing. What's the reason for disabling it now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now it requires imports to be sorted using isort. There are many open PRs which might get conflicts if we do this.

On the other hand I have no problem trying it – any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

There is at least one very large PR open from @d-maurer, please hold off with any large "cosmetic" changes until then.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. That makes sense, thank you for explaining!

@mgedmin
Copy link
Member

mgedmin commented Dec 8, 2022

Some builds are failing with

py37-zodbmaster: install_deps> python -I -m pip install -e git+https://github.com/zopefoundation/ZODB.git@master
py37-zodbmaster: exit 1 (1.21 seconds) /home/runner/work/ZEO/ZEO> python -I -m pip install -e git+https://github.com/zopefoundation/ZODB.git@master pid=1656
ERROR: Could not detect requirement name for 'git+https://github.com/zopefoundation/ZODB.git@master', please specify one with #egg=your_package_name

presumably a newer version of pip is pickier about the arguments it accepts.

Adding #egg=ZODB ought to work here, if tox doesn't strip it off as a comment.

EDIT: I see tox.ini already has #egg=ZODB there, and I see that tox is stripping it off as a comment. Maybe wrapping the URL in quotes will help? Or maybe we need to escape the # with a backslash?

@mgedmin
Copy link
Member

mgedmin commented Dec 8, 2022

The comment stripping is new in tox 4.0, so we might pin tox to <4 as a workaround for now.

@mgedmin
Copy link
Member

mgedmin commented Dec 8, 2022

This works for me with tox 4.0.2:

diff --git a/tox.ini b/tox.ini
index dfa36851..5d077daa 100644
--- a/tox.ini
+++ b/tox.ini
@@ -20,7 +20,7 @@ envlist =
 usedevelop = true
 deps =
     !zodbmaster: ZODB >= 4.2.0b1
-    zodbmaster: -e git+https://github.com/zopefoundation/ZODB.git@master#egg=ZODB
+    zodbmaster: -e "git+https://github.com/zopefoundation/ZODB.git@master\#egg=ZODB"
     uvloop: uvloop
 setenv =
     !py27-!pypy: PYTHONWARNINGS=ignore::ResourceWarning
@@ -63,7 +63,7 @@ deps =
     coverage
     coverage-python-version
     !zodbmaster: ZODB >= 4.2.0b1
-    zodbmaster: -e git+https://github.com/zopefoundation/ZODB.git@master#egg=ZODB
+    zodbmaster: -e "git+https://github.com/zopefoundation/ZODB.git@master\#egg=ZODB"
     uvloop: uvloop
 commands =
     mkdir -p {toxinidir}/parts/htmlcov

Just the quotes were insufficient. I haven't tried just the backslash. The quotes add a nice safety-check where if you forget the backslash, you get an error ("No closing quotation") instead of silent truncation.

@icemac
Copy link
Member Author

icemac commented Dec 9, 2022

Just the quotes were insufficient. I haven't tried just the backslash. The quotes add a nice safety-check where if you forget the backslash, you get an error ("No closing quotation") instead of silent truncation.

According to tox-dev/tox#2638 just the backslash should be sufficient. I'll add it and switch back to a current tox version.

@d-maurer
Copy link
Contributor

d-maurer commented Dec 9, 2022 via email

@icemac
Copy link
Member Author

icemac commented Dec 9, 2022

Is there a chance just to disable the isort check (maybe via a configuration or a parameter)? It would be a pain to resolve merge conflicts caused by different sorting rules.

Currently there is no such switch. But on the other hand there are currently no enforced sorting rules for this repository, so the possible conflicts have to be resolved only once.

@dataflake
Copy link
Member

You can always disable it manually in tox.ini.

@dataflake
Copy link
Member

Just the quotes were insufficient. I haven't tried just the backslash. The quotes add a nice safety-check where if you forget the backslash, you get an error ("No closing quotation") instead of silent truncation.

According to tox-dev/tox#2638 just the backslash should be sufficient. I'll add it and switch back to a current tox version.

Ouch. tox 4 requires at least Python 3.7 so earlier Python versions will pull tox < 4 and choke on that backslash...

@d-maurer
Copy link
Contributor

d-maurer commented Dec 9, 2022 via email

@icemac
Copy link
Member Author

icemac commented Dec 9, 2022

Ouch. tox 4 requires at least Python 3.7 so earlier Python versions will pull tox < 4 and choke on that backslash...

This hurts. Time to drop py27-zodbmaster? Or should I try to find a way to keep py27-zodbmaster?

@dataflake
Copy link
Member

Ouch. tox 4 requires at least Python 3.7 so earlier Python versions will pull tox < 4 and choke on that backslash...

This hurts. Time to drop py27-zodbmaster? Or should I try to find a way to keep py27-zodbmaster?

IMHO the test itself can be dropped for GHA. I don't recall what the motivation was, maybe just an early warning for ZODB incompatibilities before a ZODB release? The added value may not be enough to justify the effort of keeping that alive.

…k with both tox 3 (Py27) and 4.

Add some other test combinations instead.
@icemac icemac enabled auto-merge December 13, 2022 07:23
@icemac icemac merged commit 7dda07f into master Dec 13, 2022
@icemac icemac deleted the fix-gha branch December 13, 2022 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants