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

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

Closed
wants to merge 3 commits into from

Conversation

tseaver
Copy link
Member

@tseaver tseaver commented Apr 17, 2024

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

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

@dataflake dataflake left a comment

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
Copy link
Member

icemac commented Apr 18, 2024

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
Copy link
Member Author

tseaver commented Apr 18, 2024

@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
Copy link
Member Author

tseaver commented Apr 18, 2024

@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.).

@dataflake
Copy link
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
Copy link
Member

icemac commented Apr 19, 2024

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants