search

sagemath / cysignals

cysignals: interrupt and signal handling for Cython. Source repository for https://pypi.org/project/cysignals/
GNU Lesser General Public License v3.0
44 stars 23 forks source link

Badly defined PY_LONG_LONG when building with PARI enabled #107

Closed embray closed 5 years ago

embray commented 5 years ago

On my local machine, ever since 64e2586f072c6e9048162bc14550a89c5febd304 when I try to build Cysignals I get:

$ make
python setup.py build
running build
running build_py
running build_ext
building 'cysignals.signals' extension
gcc -I/usr/include/ncurses -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wno-unused -DCYTHON_CLINE_IN_TRACEBACK=0 -DFD_SETSIZE=512 -U_FORTIFY_SOURCE -Isrc/cysignals -Isrc -I/home/embray/src/sagemath/sage/local/include/python2.7 -c build/src/cysignals/signals.c -o build/temp.cygwin-2.9.0-x86_64-2.7/build/src/cysignals/signals.o
build/src/cysignals/signals.c: In function ‘__Pyx_PyInt_From_int’:
build/src/cysignals/signals.c:6747:61: error: ‘long long long’ is too long for GCC
         } else if (sizeof(int) <= sizeof(unsigned PY_LONG_LONG)) {
                                                             ^~~~

If I define out the inclusion of <windows.h> (which shouldn't be needed on Cygwin anyways) it works.

Curiously, I expected to see this on AppVeyor when working on #106, but it did not occur (despite being based on the latest master branch, including that commit).

It might be a bug in the windows headers included in Cygwin that is fixed in newer versions, but I'm not sure.

embray commented 5 years ago

To add, this happens whether I use the Python included in Cygwin, or a Python built in Sage. So I don't think it's a problem with the Python headers themselves.

jdemeyer commented 5 years ago

Which Python version is this?

embray commented 5 years ago

2.7

embray commented 5 years ago

I think the problem is originating from pari/parigen.h which contains this nonsense:

 16 #ifdef _WIN64
 17 typedef unsigned long long pari_ulong;
 18 #define long long long
 19 #define labs llabs
 20 #else
 21 typedef unsigned long pari_ulong;
 22 #endif

where _WIN64 is being defined by including <windows.h>. It's not a problem on the AppVeyor build because it's not including PARI (perhaps it should...)

jdemeyer commented 5 years ago

Indeed.

embray commented 5 years ago

That #define long long long in parigen.h is just hard-coded there. That can't be right...

jdemeyer commented 5 years ago

8913e0b66a9acea11a5971868d748cde368ae29b will probably fix this

embray commented 5 years ago

Oh, I'll give that a try. I had a more complicated solution which also works:

diff --git a/src/cysignals/implementation.c b/src/cysignals/implementation.c
index dd5c13e..fc3de38 100644
--- a/src/cysignals/implementation.c
+++ b/src/cysignals/implementation.c
@@ -58,7 +58,17 @@ Interrupt and signal handling for Cython
 #endif
 #include <Python.h>
 #if HAVE_PARI
+#ifdef _WIN64
+/* One of PARI's headers unhelpfully redefines 'long' in _WIN64 is defined;
+   for the purposes of including it here this is doubly unhelpful, so
+   temporarily undefined _WIN64 for now */
+#define WIN64_DEFINED
+#undef _WIN64
+#endif
 #include <pari/pari.h>
+#ifdef WIN64_DEFINED
+#define _WIN64
+#endif
 #else
 /* Fake PARI variables */
 static int PARI_SIGINT_block = 0;
embray commented 5 years ago

8913e0b is good, thanks 👍

embray commented 5 years ago

Yeesh. Now that this and #106 are dealt with I can finally move on with the actual issue which was to submit a workaround for https://trac.sagemath.org/ticket/27214#comment:10