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

Migrated setuptools configuration to pyproject.toml. #17806

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

claudep
Copy link
Member

@claudep claudep commented Feb 1, 2024

No description provided.

@ngnpope ngnpope self-requested a review February 1, 2024 09:48
@pauloxnet
Copy link
Contributor

From a quick review, it seems fine to me 👍🏻

@nessita
Copy link
Contributor

nessita commented Mar 28, 2024

I will check these new instructions during the incoming release (early April).

/remind me to test this PR on April 2nd

Copy link

@nessita set a reminder for 4/2/2024

Copy link

github-actions bot commented Apr 2, 2024

👋 @nessita, test this PR

@github-actions github-actions bot removed the reminder label Apr 2, 2024
Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

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

Generally LGTM

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@claudep
Copy link
Member Author

claudep commented Apr 11, 2024

Thanks Nick! All suggestions applied.

@claudep
Copy link
Member Author

claudep commented Apr 13, 2024

Sorry, I messed up at some point. Now it should be better.

@claudep
Copy link
Member Author

claudep commented Apr 13, 2024

Mmmh, looks like deleting the find line is disturbing the tests. I'll try reintroduce it.

@ngnpope
Copy link
Member

ngnpope commented Apr 13, 2024

Yeah. I'll have to have a look next week at the details of how auto-discovery behaves.

@nessita
Copy link
Contributor

nessita commented Apr 23, 2024

I haven't tested this yet because the previous release was the "pair with Sarah" release and I prioritized showing the current flow.

/remind me to recheck this work on May 6th

Copy link

@nessita set a reminder for 5/6/2024

Copy link

github-actions bot commented May 6, 2024

👋 @nessita, recheck this work

@github-actions github-actions bot removed the reminder label May 6, 2024
@adamchainz
Copy link
Sponsor Member

Can we retitle this “Migrated setuptools configuration to pyproject.toml.”? That’s the major change here, using build is orthogonal (and could even be a second commit).

@claudep claudep changed the title Replaced setup.py build method by python -m build. Migrated setuptools configuration to pyproject.toml. May 21, 2024
@ngnpope
Copy link
Member

ngnpope commented May 22, 2024

I was having a detailed look at this last night and there are some things that need tweaking. Will try to update with something soon.

Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

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

Other than the things below I also saw the following warning in the output both before and after:

warning: no previously-included files matching '__pycache__' found anywhere in distribution

It seems that we should be able to remove global-exclude __pycache__ from MANIFEST.in.

Also, as we now depend on setuptools>=61 and given that setup.py install has been deprecated since v58.3.0 in October 2021, I think we can consider removing setup.py in its entirety. (It contains a bunch of workarounds to avoid overlaying an existing install which is no longer really applicable.)

pyproject.toml Outdated Show resolved Hide resolved
extras/Makefile Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@ngnpope
Copy link
Member

ngnpope commented May 22, 2024

@claudep Did you have any thoughts on these comments?

@claudep
Copy link
Member Author

claudep commented May 23, 2024

Oh sorry, missed it. I just pushed commits with your suggestion.

@ngnpope
Copy link
Member

ngnpope commented May 23, 2024

Also, as we now depend on setuptools>=61 and given that setup.py install has been deprecated since v58.3.0 in October 2021, I think we can consider removing setup.py in its entirety. (It contains a bunch of workarounds to avoid overlaying an existing install which is no longer really applicable.)

Another reason in favour of this is that it is relying on distutils still which has been removed from Python 3.12+ and is now a deprecated part of setuptools.

@nessita
Copy link
Contributor

nessita commented Jun 18, 2024

@ngnpope Hey there, we have a new release in a week (5.1 beta release), would you have some time to re-review this PR before then to consider merging this work and using it in that release? Thank you.

@ngnpope
Copy link
Member

ngnpope commented Jun 18, 2024

@nessita I'm basically happy with this, but am aware that the filename casing is still an issue:

$ git clean -fdx
$ python -m build
...
Successfully built django-5.1.dev20240523062023.tar.gz and Django-5.1-py3-none-any.whl

As I said before, I don't think we get a choice in the sdist filename. What's more confusing is why the wheel filename is still uppercased as I thought the whole point of PEP 625 was to align the naming to match the wheel filename convention... 🤔

We could, I guess, change the project name to be lowercased:

[project]                                                                                                               
name = "django"                                                                                                                                                

Which would then produce something consistent:

$ git clean -fdx
$ python -m build
...
Successfully built django-5.1.dev20240523062023.tar.gz and django-5.1-py3-none-any.whl

But this would change the casing of the name on PyPI itself. I don't know if that's a problem, but it sidesteps all of these normalization-related issues by being already normalized.

setup.cfg Outdated Show resolved Hide resolved
@ngnpope
Copy link
Member

ngnpope commented Jun 18, 2024

What's more confusing is why the wheel filename is still uppercased as I thought the whole point of PEP 625 was to align the naming to match the wheel filename convention... 🤔

It seems that there is a bug and the wheel filename should be lowercased according to this:

https://packaging.python.org/en/latest/specifications/binary-distribution-format/#escaping-and-unicode

More reading:

Urgh. What a mess.

TL;DR: This is good to go as the filename problem existed before switching to pyproject.toml anyway. You have two options - pin to setuptools>=61.0.0,<69.3.0 until setuptools is fixed to produce correct wheel filenames or switch to name = "django" to pre-normalize the name.

@nessita
Copy link
Contributor

nessita commented Jun 18, 2024

TL;DR: This is good to go as the filename problem existed before switching to pyproject.toml anyway. You have two options - pin to setuptools>=61.0.0,<69.3.0 until setuptools is fixed to produce correct wheel filenames or switch to name = "django" to pre-normalize the name.

Thank you so much @ngnpope for the complete summary. My preference is to pin setuptools, so we can merge this and progress this modernization without disrupting the project name and the binary casing. One question though: from my experiments, we need setuptools<67 to avoid any name case change. But you are proposing <69.3.0 so I wanted to double check with you about the proper upper threshold.

@ngnpope
Copy link
Member

ngnpope commented Jun 19, 2024

One question though: from my experiments, we need setuptools<67 to avoid any name case change. But you are proposing <69.3.0 so I wanted to double check with you about the proper upper threshold.

v63.9.0 introduced the changes that affected the filename, so I think the bounds I gave were correct? (It's what worked for me.)

@nessita
Copy link
Contributor

nessita commented Jun 19, 2024

One question though: from my experiments, we need setuptools<67 to avoid any name case change. But you are proposing <69.3.0 so I wanted to double check with you about the proper upper threshold.

v63.9.0 introduced the changes that affected the filename, so I think the bounds I gave were correct? (It's what worked for me.)

I'm assuming you meant v69.3.0 :-)
I tested again with a clean venv and you are correct, pip install "setuptools>=61.0.0,<69.3.0" allowed me to build these binaries:

-rw-r--r-- 1 nessita nessita  8200954 jun 19 10:22 Django-5.2.dev20240618152543-py3-none-any.whl
-rw-r--r-- 1 nessita nessita 10742249 jun 19 10:22 Django-5.2.dev20240618152543.tar.gz

So I think we are good to go!

Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Thank you everyone, this looks good. I have tested this in a clean environment and I have updated our "ad-hoc" release script. I will push a cleanup for what I think is a missed replace and then I'll merge:

docs/internals/contributing/writing-code/coding-style.txt:49:  ``setup.cfg`` file contains...

This branch migrates setuptools configuration from setup.py/setup.cfg to
pyproject.toml. In order to ensure that the generated binary files have
consistent casing (both the tarball and the wheel), setuptools version
is limited to ">=61.0.0,<69.3.0".

Configuration for flake8 was moved to a dedicated .flake8 file since
it cannot be configured via pyproject.toml.

Also, __pycache__ exclusion was removed from MANIFEST and the
extras/Makefile was replaced with a simpler build command.

Co-authored-by: Nick Pope <nick@nickpope.me.uk>
@nessita nessita merged commit 4686541 into django:main Jun 24, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants