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

Migrate to declarative Python package config #767

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

deronnax
Copy link

No description provided.

@deronnax deronnax marked this pull request as draft September 12, 2023 08:56
@deronnax
Copy link
Author

workflow approval plz 🥺

Copy link
Collaborator

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@deronnax before we go into details, I'd like to ask for the motivation behind this change when we could (and will eventually) migrate to build and pyproject.toml. Please help me see the value in this intermediary stage. Thank you!

setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@deronnax
Copy link
Author

@hartwork pyproject.toml is a bit of a bigger leap. initially I offered direct move to pyproject.toml but people were reluctant to do the leap. But if you are willing, I will do the conversion straight to pyproject.toml.

@hartwork
Copy link
Collaborator

hartwork commented Dec 11, 2023

@hartwork pyproject.toml is a bit of a bigger leap. initially I offered direct move to pyproject.toml but people were reluctant to do the leap. But if you are willing, I will do the conversion straight to pyproject.toml.

@deronnax thanks for your reply!

I'm trying to find a previous conversion about it up here, but I fail to find any. Have a link? Maybe it was even me who reluctant about it, to some extent I still am, but simplified personally I'll be good with any of setup.py, setup.cfg and pyproject.toml here if the conversion is lossless and has some motivation.

But I'm not making the release files. @kevin1024 will you be okay to start using the build command for creating VCR.py release files (rather than python3 setup.py ...`)? This diff hartwork/sandwine@c260763 is a demo of what may come here if you say "yes" to that. What do you think?

@jairhenrique do you have any stakes in the setup.py VS setup.cfg VS pyproject.toml future discussion?

@jairhenrique
Copy link
Collaborator

Following PEP 621, projects metadata should be put on pyproject.toml file.

Using the pypa tool hatch, following this steps, hatch automatically create or update the pyproject.toml file.

pip install hatch
hatch new --init

@deronnax deronnax marked this pull request as ready for review December 13, 2023 17:35
@deronnax
Copy link
Author

@hartwork sorry I meant "people in general on GitHub", not the vcrpy community. Some people don't feel to do the jump to pyproject.toml yet.
I did the pyproject.toml conversion on today's master, and restored the gone comments.

@hartwork
Copy link
Collaborator

hartwork commented Dec 13, 2023

@hartwork sorry I meant "people in general on GitHub", not the vcrpy community. Some people don't feel to do the jump to pyproject.toml yet.

@deronnax I understand, thanks for the clarification 👍

I did the pyproject.toml conversion on today's master, and restored the gone comments.

Thanks! The CI may(?) need adjustment to use build and all, but before we know if @kevin1024 is on board with this direction there is a risk for wasting your time, just to avoid potential disappointment.

@hartwork hartwork changed the title migrate to declarative package config Migrate to declarative Python package config Dec 13, 2023
@deronnax
Copy link
Author

@kevin1024 what's your stance on the subject? I can also do the move to the pyproject.toml if you prefer.

@kevin1024
Copy link
Owner

Hello! I’m afraid I haven’t been following new developments in Python packaging very closely for awhile now. My general opinion on this is: if it increases compatibility with the newer packaging ecosystem without breaking compatibility with any of our existing environments we support, then I am in favor.

There are a number of Linux distributions that package VCR.py from source. I believe if we make this change it may also affect their build scripts, so ideally we would also coordinate with those maintainers.

@hartwork
Copy link
Collaborator

hartwork commented Mar 16, 2024

Hello! I’m afraid I haven’t been following new developments in Python packaging very closely for awhile now. My general opinion on this is: if it increases compatibility with the newer packaging ecosystem without breaking compatibility with any of our existing environments we support, then I am in favor.

@kevin1024 that seems to be the case, I think you're in favor then.

Regarding need to migrate https://packaging.python.org/en/latest/discussions/setup-py-deprecated/ could be of interest, but it's not meant as an argument to not migrate.

There are a number of Linux distributions that package VCR.py from source. I believe if we make this change it may also affect their build scripts, so ideally we would also coordinate with those maintainers.

Speaking with my Gentoo downstream maintainer hat on here: Distros already need to support various ways to build Python software, use of pyproject.toml should not cause trouble there or the distro would be in big trouble. At least as long as mainstream values for build-system are used, see https://projects.gentoo.org/python/guide/distutils.html#pep-517-build-systems maybe for a list of backends supported in Gentoo. I personally don't expect problems o that front.

@deronnax
Copy link
Author

@kevin1024 can we get a go :) ?

@deronnax
Copy link
Author

deronnax commented Jul 29, 2024

Having this merged when it was time would have avoided #855 because vcrpy uses 4 years-deprecated setup.py test command and setuptools removed it today, breaking instantly most CI using vcrpy.

# https://github.com/kevin1024/vcrpy/pull/699#issuecomment-1551439663
"urllib3 <2; python_version <'3.10'",
# https://github.com/kevin1024/vcrpy/pull/775#issuecomment-1847849962
"urllib3 <2; platform_python_implementation =='PyPy'",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't speak for the package, but if this gets wind in the sails now and gets merged, there's a third case for urllib3 added to setup.py after this PR was created.

Comment on lines +61 to +64
"PyYAML",
"wrapt",
"yarl",
# Support for urllib3 >=2 needs CPython >=3.10

Choose a reason for hiding this comment

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

So what I don't like about this approach, is that it does not attempt to lock. If the non-locking is to preserve some old use-cases for python 3.8, then I suggest making those explicit.

This wild west, "go grab whatever versions of these packages!" is what caused the most breakages, and uncertainty, because it's harder for a dependency resolver to pick the right thing for all the packages.

Copy link
Collaborator

@hartwork hartwork Jul 31, 2024

Choose a reason for hiding this comment

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

@LewisCowlesMotive that's likely a misunderstanding: pyproject.toml is meant to not lock, to be compatible and play well with the neighbors in a pot of multiple Python packages: a virtualenv, a Linux distro, Homebrew etc. Else you quickly have packages that can no longer be installed side by side. The place to lock is requirements.txt or something similar for a single deployment, not a package ecosystem.

Choose a reason for hiding this comment

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

I don't know if you are misinterpreting my request to have version constraints as asking to make pyproject.toml a lock-file.

It's the lack of any version information I am complaining about to be clear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's leave this PR to migrate the config as is without changes, we can have a follow up PR to change the config

Choose a reason for hiding this comment

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

What are the benefits of the PR? It declares the same things as the imperative code. Is that the benefit?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

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.

7 participants