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

remove redundant greenlet dependency #51

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

graingert
Copy link

No description provided.

@graingert
Copy link
Author

I'm currently investigating kevin1024/vcrpy#848 and so running the pytest-httpbin tests on 3.13 with kevin1024/pytest-httpbin#90 but I'm blocked on greenlet supporting 3.13

pyproject.toml Outdated Show resolved Hide resolved
@graingert graingert changed the title make greenlet an optional mainapp dep remove redundant greenlet dependency Jul 31, 2024
@nateprewitt
Copy link
Member

Thanks for the PR, @graingert. I think Kevin was the one who reviewed the addition of greenlet as a direct dependency. I'm trying to find context for its addition and the only piece I can come up with is here.

This suggests the process cannot run in the docker image without it. I haven't taken a look at any of this yet, but we may want to verify/disprove that claim before making changes here.

Unrelated, I see macOS is having a hard time, likely due to the ARM image changes from a few months ago. I'll take a look at getting that fixed so it's decoupled from this PR.

@graingert
Copy link
Author

graingert commented Aug 8, 2024

@nateprewitt

This suggests the process cannot run in the docker image without it.

greenlet is pulled in as a dependency of gevent, this override was only needed to get pip to install a pre release version of greenlet on 3.12. This dependency override should only have been added to the 'mainapp' extras.

greenlet now supports 3.12 in a mainline release so this dependency is redundant

@nateprewitt
Copy link
Member

I'm referring specifically to this part of the comment I linked:

The cleanup of the dependencies I did recently removed a dependency which was needed after-all. The image built fine without it, but the process did not start.

Greenlet was previously deleted during that PR process and had to be readded because it broke the docker image. I agree it should come in with gevent, but there is likely something else incorrectly defined that's not resulting in all dependencies ending up in the image.

@graingert
Copy link
Author

Yeah it was added so that python 3.12 would install a pre release version of greenlet

Once greenlet has release a full/final 3.x release it would be good to simplify these dependencies.

This final release is now available

@graingert
Copy link
Author

graingert commented Aug 8, 2024

CI currently failing due to python 3.7 missing from GitHub actions' setup-python and failing brotlicffi. Something similar to https://github.com/kevin1024/pytest-httpbin/pull/89/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R81 will need to be applied

@graingert
Copy link
Author

Also it looks like something is wrong with the ci matrix, this pypy3.10 build is failing due to a cpython 3.9 exception https://github.com/psf/httpbin/actions/runs/10178190245/job/28513362402?pr=51

@nateprewitt
Copy link
Member

Yeah it was added so that python 3.12 would install a pre release version of greenlet

Totally, I understand why the branching end up there. What I was concerned about was this commit (4759eb1) which seems to have happened prior to the Python 3.12 issues being identified. This is how greenlet entered the declared dependency closure. If we're highly confident this was a misidentified issue, we can test out of the docker image.

For CI, yes, we need to fix the bare tox command being run because it's testing every version in every build.

@nateprewitt nateprewitt mentioned this pull request Aug 8, 2024
@nateprewitt nateprewitt reopened this Aug 8, 2024
@graingert
Copy link
Author

The magic words:

so we can resolve #51.

Closed this lol

@@ -35,8 +35,6 @@ dependencies = [
"decorator",
"flasgger",
"flask >= 2.2.4",
'greenlet < 3.0; python_version<"3.12"',
Copy link
Author

Choose a reason for hiding this comment

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

It looks like this was also incorrectly pinning to greenlet <3 on older pythons

@graingert
Copy link
Author

@nateprewitt can you take a look at this? I'd like to get 3.13 support landed in vcrpy and pytest-httpbin before the final release of 3.13

@nateprewitt
Copy link
Member

I've got this on my list for review next week. We should be able to get a release out then.

@graingert
Copy link
Author

@nateprewitt greenlet now has support for python 3.13 so this is not critical - would still be useful for when 3.14 is in beta

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.

2 participants