Skip to content

Use setuptools instead of distutils #100

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.

Sign up for GitHub

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

Merged
oscarbenjamin merged 13 commits into from
Oct 21, 2023
+48 −45
Merged

Use setuptools instead of distutils #100

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ac7d735
Use setuptools instead of distutils
oscarbenjamin Oct 21, 2023
9958bfe
Add Python 3.12 to the wheel build matrix
oscarbenjamin Oct 21, 2023
7b8dba9
Install setuptools in CI
oscarbenjamin Oct 21, 2023
621531e
Install setuptools in CI
oscarbenjamin Oct 21, 2023
17852e7
Don't unstall from PyPI when testing in CI
oscarbenjamin Oct 21, 2023
3b6b11e
Add 3.12 to the Cirrus build matrix
oscarbenjamin Oct 21, 2023
bc63dfd
Update cibuildwheel version
oscarbenjamin Oct 21, 2023
211aba0
Fix use of private Fraction methods for 3.12
oscarbenjamin Oct 21, 2023
3d162df
Only build wheels for 3.12
oscarbenjamin Oct 21, 2023
1e77473
Build for all Python versions in CI
oscarbenjamin Oct 21, 2023
c343f39
Use version checks for setuptools in setup.py
oscarbenjamin Oct 21, 2023
27d4b54
Install numpy and fix up CI testing for source builds
oscarbenjamin Oct 21, 2023
159426b
Change name of pip CI job
oscarbenjamin Oct 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 12 additions & 20 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,23 @@
import os
from subprocess import check_call

from distutils.core import setup
from distutils.extension import Extension
from setuptools import setup
from setuptools.extension import Extension
from Cython.Distutils import build_ext
from Cython.Build import cythonize
from numpy.distutils.system_info import default_include_dirs, default_lib_dirs

from distutils.sysconfig import get_config_vars

default_include_dirs = []
default_lib_dirs = []
Copy link
Copy Markdown
Collaborator Author

@oscarbenjamin oscarbenjamin Oct 21, 2023

Choose a reason for hiding this comment

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

@deinst presumably something better needs to go here but I'm not sure what. I think that you said you worked out some code for this...

Copy link
Copy Markdown
Contributor

@deinst deinst Oct 21, 2023

Choose a reason for hiding this comment

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

Ive spent a bit of time rummaging about in the numpy.distutils code to see what it does to fill the default_*_dirs variables. The above works for me, but that is because I keep my flint in a non standard place and need to supply the directory to setup.py.

On the unix/osx side, this is easy, just make a list of the usual suspects, and filter out the ones that don't exist. On the windows side things are more complicated and I'm working on figuring out what is needed.

I believe you still need get config_vars for the python dirs, but you can get it from sysconfig instead of distutils.sysconfig

Copy link
Copy Markdown
Collaborator Author

@oscarbenjamin oscarbenjamin Oct 21, 2023

Choose a reason for hiding this comment

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

I believe you still need get config_vars for the python dirs, but you can get it from sysconfig instead of distutils.sysconfig

In the version of setup.py on master this is only used to set the OPT environment variable:

python-flint/setup.py

Lines 38 to 39 in 2fd4fd6

(opt,) = get_config_vars('OPT')
os.environ['OPT'] = " ".join(flag for flag in opt.split() if flag != '-Wstrict-prototypes')

What uses that environment variable?

The value I have locally on Linux is:

In [4]: from distutils.sysconfig import get_config_vars

In [5]: get_config_vars('OPT')
Out[5]: ['-DNDEBUG -g -fwrapv -O3 -Wall']

Copy link
Copy Markdown
Contributor

@deinst deinst Oct 21, 2023

Choose a reason for hiding this comment

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

As I understand it OPT supplies default options to gcc. All setup.py does is removes -Wstrict-prototypes from it. Considering the number of compiler warnings already generated, it seems that filtering out -Wstrict_prototypes is probably not particularly helpful.

Copy link
Copy Markdown
Collaborator Author

@oscarbenjamin oscarbenjamin Oct 21, 2023

Choose a reason for hiding this comment

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

With Python 3.12 I have:

In [2]: from sysconfig import get_config_vars

In [3]: get_config_vars('OPT')
Out[3]: ['-DNDEBUG -g -O3 -Wall']

Either way there does not seem to be any -Wstrict-prototypes.

Probably that can be removed but I'll leave it there for now while getting the main problems fixed.

I think I might have get the setup.py working for all Python version or OS combinations. Fingers crossed that the Windows build is about to succeed in CI...

The only thing currently missing is default_include_dirs and default_lib_dirs but I'm not sure why we should need to get those anyway. I would have thought that it was setuptools job to handle the default dirs.

Copy link
Copy Markdown
Contributor

@deinst deinst Oct 21, 2023

Choose a reason for hiding this comment

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

If setuptools can get the directories correctly that is great. If not, I think I have gotten the requisite code from numpy.distutils.system_info to generate the default paths. I have only a foggy notion of exactly what the windows code is doing as things like vcpkg are inovations that postdate my working on code for windows.

Copy link
Copy Markdown
Collaborator Author

@oscarbenjamin oscarbenjamin Oct 21, 2023

Choose a reason for hiding this comment

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

I think I have gotten the requisite code from numpy.distutils.system_info to generate the default paths.

I think let's get this PR in for now because it at least means we have CI built wheels for 3.12 and that building a development checkout for 3.12 now works.

We can follow up with another PR for generating the paths. I don't know whether there is actually a reason for doing something other than just trusting setuptools to do it for us though.

Longer term if we can move to meson and meson-python then they should take care of default paths and library discovery etc so in principle it should not be a concern for us.


libraries = ["flint"]


if sys.platform == 'win32':
#
# This is used in CI to build wheels with mingw64
#
if os.getenv('PYTHON_FLINT_MINGW64'):
libraries = ["flint", "mpfr", "gmp"]
includedir = os.path.join(os.path.dirname(__file__), '.local', 'include')
librarydir1 = os.path.join(os.path.dirname(__file__), '.local', 'bin')
librarydir2 = os.path.join(os.path.dirname(__file__), '.local', 'lib')
Expand All @@ -26,22 +28,14 @@
# Add gcc to the PATH in GitHub Actions when this setup.py is called by
# cibuildwheel.
os.environ['PATH'] += r';C:\msys64\mingw64\bin'
libraries += ["mpfr", "gmp"]
elif os.getenv('PYTHON_FLINT_MINGW64_TMP'):
# This would be used to build under Windows against these libraries if
# they have been installed somewhere other than .local
libraries = ["flint", "mpfr", "gmp"]
libraries += ["mpfr", "gmp"]
else:
# For the MSVC toolchain link with mpir instead of gmp
libraries = ["flint", "mpir", "mpfr", "pthreads"]
else:
libraries = ["flint"]
(opt,) = get_config_vars('OPT')
os.environ['OPT'] = " ".join(flag for flag in opt.split() if flag != '-Wstrict-prototypes')


default_include_dirs += [
os.path.join(d, "flint") for d in default_include_dirs
]
libraries += ["mpir", "mpfr", "pthreads"]


define_macros = []
Expand Down Expand Up @@ -69,9 +63,7 @@


ext_files = [
# ("flint._flint", ["src/flint/_flint.pxd"]), # Main Module
("flint.pyflint", ["src/flint/pyflint.pyx"]), # Main Module
# Submodules
("flint.pyflint", ["src/flint/pyflint.pyx"]),
("flint.types.fmpz", ["src/flint/types/fmpz.pyx"]),
("flint.types.fmpz_poly", ["src/flint/types/fmpz_poly.pyx"]),
("flint.types.fmpz_mat", ["src/flint/types/fmpz_mat.pyx"]),
Expand Down Expand Up @@ -119,11 +111,11 @@
for e in ext_modules:
e.cython_directives = {"embedsignature": True}


setup(
name='python-flint',
cmdclass={'build_ext': build_ext},
ext_modules=cythonize(ext_modules, compiler_directives=compiler_directives),
#ext_modules=cythonize(ext_modules, compiler_directives=compiler_directives, annotate=True),
packages=packages,
package_dir={'': 'src'},
description='Bindings for FLINT and Arb',
Expand Down