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

Syntax error in py310compat.py #100

Closed
pgergov opened this issue Jul 10, 2023 · 4 comments
Closed

Syntax error in py310compat.py #100

pgergov opened this issue Jul 10, 2023 · 4 comments
Labels
invalid This doesn't seem right

Comments

@pgergov
Copy link

pgergov commented Jul 10, 2023

Hello,

With the latest release https://github.com/jaraco/zipp/releases/tag/v3.16.0 there's a syntax error in py310compat.py file:

    def _text_encoding(encoding, stacklevel=2, /):
                                               ^
SyntaxError: invalid syntax

This is the commit that introduces the regression 4269d27


As a side note, dropping support for a version (in this case, Python 3.7) is considered a breaking change. When introducing breaking changes, you should increment the major version number, instead of the minor version.

maartenbreddels added a commit to vaexio/vaex that referenced this issue Jul 10, 2023
@maartenbreddels
Copy link

https://pypi.org/project/zipp/3.16.0/ seems to list the requirement (python>=3.8) correctly, but we probably got this via conda/conda-forge.

@DamianBarabonkovQC
Copy link

I think someone has to patch the conda meta file to pin zipp if an old python is in use. I am unsure how to do that myself, but have seen it done.

kmaziarz added a commit to microsoft/syntheseus that referenced this issue Jul 11, 2023
Recently, a library called `zipp` (which we don't depend on directly,
but it seems something related to our pre-commit does) released version
`3.16`, which dropped support for Python 3.7. It seems that `zipp`'s
Python requirement is not correctly picked during installation (see
jaraco/zipp#100); this makes our Python 3.7 CI fail as it installs the
new version.

In this PR, I pin `zipp<3.16` to get the CI on `main` to pass again.
This is not ideal as we don't even depend on `zipp` directly, but I hope
it's temporary, and we can remove it once the community sorts things
out.
@jaraco
Copy link
Owner

jaraco commented Jul 11, 2023

As a side note, dropping support for a version (in this case, Python 3.7) is considered a breaking change.

I used to share this opinion, but I've since decided that since the project is intentionally excluding support for the now broken version and declaring that support in the metadata, it's not actually a breaking change. That is, it's a normal course of development for versions to be added and removed. I follow this convention across the hundreds of projects I develop. If you think there's a compelling argument for always making a major bump when dropping support for EOL Pythons, you can make that case in jaraco/skeleton and present evidence/arguments that the best practice is to make a major bump. I do intend to follow best practices here and there's definitely room for dispute, but for now I've settled on a minor bump.

As you've all divined, this change was intentional, and the bug exists in conda (or other installers) not honoring the required Pythons directive.

@jaraco jaraco closed this as completed Jul 11, 2023
@jaraco jaraco added the invalid This doesn't seem right label Jul 11, 2023
@maartenbreddels
Copy link

Yeah, I agree this is not a zipp issue, but this should be fixed in the conda-recipe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

4 participants