Use setuptools instead of distutils by oscarbenjamin · Pull Request #100 · flintlib/python-flint · GitHub

Partial (short term) fix for gh-52

Uses setuptools instead of distutils.

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

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

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']

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.

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.

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.

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.

Everything here seems to be working here except Windows. One of the things that I worried about with using setuptools is that its support for mingw is supposedly not as good as numpy.distutils and mingw is used for building the Windows wheels.

The build failure on Windows is:

        gcc -mdll -O -Wall -ID:\a\python-flint\python-flint\.local\include -IC:\Users\runneradmin\AppData\Local\Temp\cibw-run-5hwr_lyj\cp39-win_amd64\build\venv\include -IC:\Users\runneradmin\AppData\Local\pypa\cibuildwheel\Cache\nuget-cpython\python.3.9.13\tools\include -IC:\Users\runneradmin\AppData\Local\pypa\cibuildwheel\Cache\nuget-cpython\python.3.9.13\tools\Include -c src/flint/pyflint.c -o build\temp.win-amd64-cpython-39\Release\src\flint\pyflint.o
        src/flint/pyflint.c:428:41: warning: division by zero [-Wdiv-by-zero]
             enum { __pyx_check_sizeof_voidp = 1 / (int)(SIZEOF_VOID_P == sizeof(void*)) };
                                                 ^
        src/flint/pyflint.c:428:12: error: enumerator value for '__pyx_check_sizeof_voidp' is not an integer constant
             enum { __pyx_check_sizeof_voidp = 1 / (int)(SIZEOF_VOID_P == sizeof(void*)) };
                    ^~~~~~~~~~~~~~~~~~~~~~~~
        src/flint/pyflint.c: In function '__Pyx_ImportType_3_0_4':
        src/flint/pyflint.c:6670:13: warning: unknown conversion type character 'z' in format [-Wformat=]
                     "%s.%s size changed, may indicate binary incompatibility. "
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        src/flint/pyflint.c:6671:24: note: format string is defined here
                     "Expected %zd from C header, got %zd from PyObject",
                                ^
        src/flint/pyflint.c:6670:13: warning: unknown conversion type character 'z' in format [-Wformat=]
                     "%s.%s size changed, may indicate binary incompatibility. "
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        src/flint/pyflint.c:6671:47: note: format string is defined here
                     "Expected %zd from C header, got %zd from PyObject",
                                                       ^
        src/flint/pyflint.c:6670:13: warning: too many arguments for format [-Wformat-extra-args]
                     "%s.%s size changed, may indicate binary incompatibility. "
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        error: command 'C:\\mingw64\\bin\\gcc.exe' failed with exit code 1

Those exact errors are reported as a general Cython+mingw issue here:
cython/cython#3405

The comments here indicate that mingw support in setuptools has improved: cython/cython#4470 (comment). Apparently though it is still necessary to apply a patch to CPython's header files for Python < 3.12.

We don't see this working for 3.12 working here because it fails building the 3.9 wheel before attempting the 3.12 wheel. I'm going to push a commit that only builds 3.12 wheels to see if at least that works.

I'm going to push a commit that only builds 3.12 wheels to see if at least that works.

It does work so what works is:

Building with setuptools for Linux/OSX in all Python versions
Building with setuptools and mingw on Windows for Python 3.12
Building with numpy.distutils and mingw on Windows for Python < 3.12

What does not work is:

Building with setuptools and mingw on Windows for Python < 3.12
Building with numpy.distutils on Python 3.12

To build with setuptools we would apparently need to apply this patch to CPython:

diff --git a/PC/pyconfig.h b/PC/pyconfig.h
index 1a33d4c5a1e4f..1d8408b363a66 100644
--- a/PC/pyconfig.h
+++ b/PC/pyconfig.h
@@ -209,6 +209,16 @@ typedef int pid_t;
 
 #endif /* _MSC_VER */
 
+/* ------------------------------------------------------------------------*/
+/* mingw and mingw-w64 define __MINGW32__ */
+#ifdef __MINGW32__
+
+#ifdef _WIN64
+#define MS_WIN64
+#endif
+
+#endif /* __MINGW32__*/
+
 /* ------------------------------------------------------------------------*/
 /* egcs/gnu-win32 defines __GNUC__ and _WIN32 */
 #if defined(__GNUC__) && defined(_WIN32)

I am more tempted though just to add some Python-version logic to setup.py. We have a setup that has been working for Python < 3.12 and I think I have just found a setup that works for Python 3.12 at least for development builds and for building wheels in CI.

Not having the default paths added in setup.py is a problem but at least being able to build at all on Python 3.12 is an improvement. This check passes in CI so it at least seems to pick up Flint in /usr/local on Linux:

python-flint/bin/pip_install_ubuntu.sh

Lines 24 to 56 in 2fd4fd6

    
           # This will default to installing in /usr/local. If you want to install in a 
        
           # non-standard location then configure flint with 
        
           #    ./configure --disable-static --prefix=$PREFIX 
        
           # If $PREFIX is no in default search paths, then at build time set 
        
           #    export C_INCLUDE_PATH=$PREFIX/include 
        
           # and at runtime set 
        
           #    export LD_LIBRARY_PATH=$PREFIX/lib 
        
           curl -O -L https://www.flintlib.org/flint-$FLINTVER.tar.gz 
        
           tar xf flint-$FLINTVER.tar.gz 
        
           cd flint-$FLINTVER 
        
             ./bootstrap.sh 
        
             ./configure --disable-static 
        
             make -j 
        
             sudo make install 
        
           cd .. 
        
           ls -l /usr/local/lib 
        
           sudo ldconfig /usr/local/lib 
        
           # Python build requirements. Ideally these would be in pyprojec.toml, but 
        
           # first need to migrate from setup.py to pyproject.toml. 
        
           pip install 'cython>=3' numpy wheel 
        
           # Install from checkout (or sdist). 
        
           echo ----------------------------------------------------------- 
        
           echo 
        
           echo     Running: 
        
           echo        $ pip install --no-build-isolation $1 
        
           echo 
        
           echo ----------------------------------------------------------- 
        
           pip install --no-build-isolation $1

So it is working for all Python versions and for Windows as well as OSX and Linux.

Note that the pip install python-flint jobs are expected to fail. They are testing building from source with pip install python-flint after building Flint 3. Until a new sdist is pushed to PyPI that will fail because the sdist currently on PyPI expects Flint 2.9 and Arb. The only reason those jobs pass on master is because they are installing the binary wheel from PyPI but I've fixed that here which then means that those CI jobs (correctly) fail.

I think this has to be merged before Cirrus CI will start generating 3.12 wheels for OSX ARM64. I think everything else is working here now so I'll merge and then see what CI does on the merge build.

Use setuptools instead of distutils

ac7d735

oscarbenjamin commented Oct 21, 2023

View reviewed changes

oscarbenjamin added 7 commits October 21, 2023 18:34

Add Python 3.12 to the wheel build matrix

9958bfe

Install setuptools in CI

7b8dba9

Install setuptools in CI

621531e

Don't unstall from PyPI when testing in CI

17852e7

Add 3.12 to the Cirrus build matrix

3b6b11e

Update cibuildwheel version

bc63dfd

Fix use of private Fraction methods for 3.12

211aba0

Only build wheels for 3.12

3d162df

oscarbenjamin added 4 commits October 21, 2023 22:37

Build for all Python versions in CI

1e77473

Use version checks for setuptools in setup.py

c343f39

Install numpy and fix up CI testing for source builds

27d4b54

Change name of pip CI job

159426b

oscarbenjamin merged commit 39a9df9 into flintlib:master Oct 21, 2023

oscarbenjamin deleted the pr_setuptools branch October 21, 2023 22:48

This was referenced Oct 21, 2023

Update cibuildwheel version in Cirrus CI #101

Merged

Python 3.12, pyproject.toml, distutils and a new build system #52

Closed

mkoeppe mentioned this pull request Jan 25, 2024

pyproject.toml, setup.py: Only use numpy.distutils on win32 #112

Merged

-              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 = []

Use setuptools instead of distutils by oscarbenjamin · Pull Request #100 · flintlib/python-flint · GitHub

Repository navigation

Use setuptools instead of distutils - #100

Use setuptools instead of distutils#100
oscarbenjamin merged 13 commits into
flintlib:masterfrom
oscarbenjamin:pr_setuptools

oscarbenjamin commented Oct 21, 2023

Uh oh!

oscarbenjamin Oct 21, 2023

Uh oh!

deinst Oct 21, 2023

Uh oh!

oscarbenjamin Oct 21, 2023

Uh oh!

deinst Oct 21, 2023

Uh oh!

oscarbenjamin Oct 21, 2023

Uh oh!

deinst Oct 21, 2023

Uh oh!

oscarbenjamin Oct 21, 2023

Uh oh!

oscarbenjamin commented Oct 21, 2023

Uh oh!

oscarbenjamin commented Oct 21, 2023

Uh oh!

oscarbenjamin commented Oct 21, 2023

Uh oh!

oscarbenjamin commented Oct 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

Use setuptools instead of distutils by oscarbenjamin · Pull Request #100 · flintlib/python-flint · GitHub

Repository navigation

Use setuptools instead of distutils - #100

Use setuptools instead of distutils#100oscarbenjamin merged 13 commits intoflintlib:masterflintlib/python-flint:masterfrom oscarbenjamin:pr_setuptoolsoscarbenjamin/python-flint:pr_setuptoolsCopy head branch name to clipboard

Conversation

oscarbenjamin commented Oct 21, 2023

Uh oh!

oscarbenjamin Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

deinst Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

deinst Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

deinst Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin commented Oct 21, 2023

Uh oh!

oscarbenjamin commented Oct 21, 2023

Uh oh!

oscarbenjamin commented Oct 21, 2023

Uh oh!

oscarbenjamin commented Oct 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Use setuptools instead of distutils#100
oscarbenjamin merged 13 commits into
flintlib:masterfrom
oscarbenjamin:pr_setuptools