Skip to content

refactor: convert 'setup()' metadata to TOML - #195

Closed
tseaver wants to merge 3 commits into
masterfrom
tseaver-py313-tomlize
Closed

refactor: convert 'setup()' metadata to TOML#195
tseaver wants to merge 3 commits into
masterfrom
tseaver-py313-tomlize

Conversation

@tseaver

@tseaver tseaver commented Apr 17, 2024

Copy link
Copy Markdown
Member

Depend on GH master branch of 'cffi' for CPython 3.13*, until there is a release of 'cffi' which supports it.

@tseaver
refactor: convert 'setup()' metadata to TOML
b04b168 
Depend on GH master branch of 'cffi' for CPython 3.13*, until there is a
release of 'cffi' which supports it.
@tseaver
tseaver requested review from dataflake and icemac April 17, 2024 23:27
dataflake
dataflake reviewed Apr 18, 2024
View reviewed changes

@dataflake dataflake left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am a little reluctant to approve the PR. It's fine per se, but we don't use pyproject.toml anywhere else. @icemac what do you think?

@icemac

icemac commented Apr 18, 2024

Copy link
Copy Markdown
Member

pyproject.toml is probably one of the steps we have to go with all our packages. setup.py is not (yet) deprecated (https://packaging.python.org/en/latest/discussions/setup-py-deprecated/) but it is the past, not the future.

When trying something else than setup.py (like setup.cfg or pyproject.toml) I saw that mr.developer has problems because of the missing setup.py and buildout might not be happy if there is no wheel. – But this PR keeps setup.py, so this might be no problem.

@tseaver Interestingly GitHub Actions breaks only on Ubuntu Python 3.13a6: https://github.com/zopefoundation/persistent/actions/runs/8733259336/job/23961629868?pr=195
while running python setup.py build_ext -i. Do you have any experience how to replace this call with a more modern variant? The Python packaging document linked above says that using setup.py in this way is deprecated but it does not talk about a possible replacement.

@tseaver

tseaver commented Apr 18, 2024

Copy link
Copy Markdown
Member Author

@dataflake

I am a little reluctant to approve the PR. It's fine per se, but we don't use pyproject.toml anywhere else.

I thought I had added an explanation for that choice: it made it easier / saner to spell the weird conditional dependency on the Github master branch: it could likely be done in setup.py, as well, but I didn't bother to figure out how. Essentially, the trick would be to get this string added to both setup_requires and install_requires:

"cffi @ git+https://github.com/python-cffi/cffi.git ; platform_python_implementation == 'CPython' and python_version >= '3.13a0'"

@icemac

Interestingly GitHub Actions breaks only on Ubuntu Python 3.13a6: https://github.com/zopefoundation/persistent/actions/runs/8733259336/job/23961629868?pr=195
while running python setup.py build_ext -i.

Hmm, built the bdist_wheel for me with Python 3.13.0a5. Guess I need to build 3.10.0a6 and try with that. On my box:

$ cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=20.04
DISTRIB_CODENAME=focal
DISTRIB_DESCRIPTION="Ubuntu 20.04.6 LTS"
$ /opt/Python-3.13.0a6/bin/python3.13 setup.py build_ext -i
/opt/Python-3.13.0a6/lib/python3.13/site-packages/setuptools/_distutils/dist.py:266: UserWarning: Unknown distribution option: 'cffi_modules'
  warnings.warn(msg)
running build_ext
building 'persistent.cPersistence' extension
creating build
creating build/temp.linux-x86_64-cpython-313
creating build/temp.linux-x86_64-cpython-313/src
creating build/temp.linux-x86_64-cpython-313/src/persistent
gcc -pthread -fno-strict-overflow -Wsign-compare -DNDEBUG -g -O3 -Wall -fPIC -I/opt/Python-3.13.0a6/include/python3.13 -c src/persistent/cPersistence.c -o build/temp.linux-x86_64-cpython-313/src/persistent/cPersistence.o
gcc -pthread -fno-strict-overflow -Wsign-compare -DNDEBUG -g -O3 -Wall -fPIC -I/opt/Python-3.13.0a6/include/python3.13 -c src/persistent/ring.c -o build/temp.linux-x86_64-cpython-313/src/persistent/ring.o
creating build/lib.linux-x86_64-cpython-313
creating build/lib.linux-x86_64-cpython-313/persistent
gcc -pthread -shared build/temp.linux-x86_64-cpython-313/src/persistent/cPersistence.o build/temp.linux-x86_64-cpython-313/src/persistent/ring.o -o build/lib.linux-x86_64-cpython-313/persistent/cPersistence.cpython-313-x86_64-linux-gnu.so
building 'persistent.cPickleCache' extension
gcc -pthread -fno-strict-overflow -Wsign-compare -DNDEBUG -g -O3 -Wall -fPIC -I/opt/Python-3.13.0a6/include/python3.13 -c src/persistent/cPickleCache.c -o build/temp.linux-x86_64-cpython-313/src/persistent/cPickleCache.o
gcc -pthread -fno-strict-overflow -Wsign-compare -DNDEBUG -g -O3 -Wall -fPIC -I/opt/Python-3.13.0a6/include/python3.13 -c src/persistent/ring.c -o build/temp.linux-x86_64-cpython-313/src/persistent/ring.o
gcc -pthread -shared build/temp.linux-x86_64-cpython-313/src/persistent/cPickleCache.o build/temp.linux-x86_64-cpython-313/src/persistent/ring.o -o build/lib.linux-x86_64-cpython-313/persistent/cPickleCache.cpython-313-x86_64-linux-gnu.so
building 'persistent._timestamp' extension
gcc -pthread -fno-strict-overflow -Wsign-compare -DNDEBUG -g -O3 -Wall -fPIC -I/opt/Python-3.13.0a6/include/python3.13 -c src/persistent/_timestamp.c -o build/temp.linux-x86_64-cpython-313/src/persistent/_timestamp.o
gcc -pthread -shared build/temp.linux-x86_64-cpython-313/src/persistent/_timestamp.o -o build/lib.linux-x86_64-cpython-313/persistent/_timestamp.cpython-313-x86_64-linux-gnu.so
copying build/lib.linux-x86_64-cpython-313/persistent/cPersistence.cpython-313-x86_64-linux-gnu.so -> src/persistent
copying build/lib.linux-x86_64-cpython-313/persistent/cPickleCache.cpython-313-x86_64-linux-gnu.so -> src/persistent
copying build/lib.linux-x86_64-cpython-313/persistent/_timestamp.cpython-313-x86_64-linux-gnu.so -> src/persistent

Do you have any experience how to replace this call with a more modern variant?

Looking at that failure, the GH action seems to be using a cached version of cffi which predates the merge of this PR. Since we specify that we want to depend on the github master branch, I'm not sure how that is happening. Do we need to sprinkle some majyk cache-averting fairy dust on .github/workflows/tests.yml?

The Python packaging document linked above says that using setup.py in this way is deprecated but it does not talk about a possible replacement.

AFAICT, the "official" docs for building extension modules under setuptools do not deprecate setup.py: they only recommend moving all the rest of the metadata to pyproject.toml.

tseaver
tseaver commented Apr 18, 2024
View reviewed changes
Comment thread .github/workflows/tests.yml
@tseaver

tseaver commented Apr 18, 2024

Copy link
Copy Markdown
Member Author

@dataflake I think I've managed to tweak setup.py and the tests.yaml file on PR #192 to accomplish the same goal. If the 3.13 tests pass there, we can close this PR.

We should still be thinking about moving to pyproject.toml as a replacement for metadata in setup.py, plus all the other tooling support files (setup.cfg, tox.ini, .coveragerc, etc.).

@tseaver tseaver mentioned this pull request Apr 18, 2024
Closed
@dataflake

dataflake commented Apr 18, 2024

Copy link
Copy Markdown
Member

I fully agree that we need to modernize the package build setup. Something of this nature is usually discussed and decided on the meta repository. That way we can make sure it works with the configuration file template generator. We are trying to avoid manual tweaks for these common files.

@icemac

icemac commented Apr 19, 2024

Copy link
Copy Markdown
Member

Superseded by #192.

@icemac icemac closed this Apr 19, 2024
@icemac
icemac deleted the tseaver-py313-tomlize branch April 19, 2024 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dataflake dataflake dataflake left review comments
@icemac icemac Awaiting requested review from icemac

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tseaver @icemac @dataflake