Member

Choose a reason for hiding this comment

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

Mostly LGTM.

One minor suggestion, lets rename Py_FFI_TARGET_HAS_COMPLEX_TYPE to something like Py_FFI_SUPPORT_C_COMPLEX.

Only non-trivial change here is in the configure.ac. I assume, you have tested this check on your system with old libffi.

Probably, this should be tested with build bots. Old workaround with FFI_TARGET_HAS_COMPLEX_TYPE was working e.g. on Sparc #120894 (comment)

Member Author

One minor suggestion, lets rename Py_FFI_TARGET_HAS_COMPLEX_TYPE to something like Py_FFI_SUPPORT_C_COMPLEX.

Ok.

Only non-trivial change here is in the configure.ac.

Definitely.

I assume, you have tested this check on your system with old libffi.

Yes. It works fine with libffi.so.6:

root@efimov:/usr/lib/x86_64-linux-gnu# rm libffi.so && ln -s libffi.so.6 libffi.so
-> % (./configure --with-pydebug && make -j && ./python -m unittest -v test.test_ctypes.test_libc.LibTest.test_csqrt) 2>&1 | grep -E 'libffi|lffi'
checking for libffi... yes
checking libffi has complex type support... no
gcc -pthread -shared      Modules/_ctypes/_ctypes.o Modules/_ctypes/callbacks.o Modules/_ctypes/callproc.o Modules/_ctypes/stgdict.o Modules/_ctypes/cfield.o -lffi -ldl  -o Modules/_ctypes.cpython-314d-x86_64-linux-gnu.so
gcc -pthread -shared      Modules/_ctypes/_ctypes_test.o -lffi -ldl -lm  -o Modules/_ctypes_test.cpython-314d-x86_64-linux-gnu.so
test_csqrt (test.test_ctypes.test_libc.LibTest.test_csqrt) ... skipped 'requires C11 complex type and libffi >= 3.3.0'

And with libffi.so.7:

root@efimov:/usr/lib/x86_64-linux-gnu# rm libffi.so && ln -s libffi.so.7 libffi.so
-> % (./configure --with-pydebug && make -j && ./python -m unittest -v test.test_ctypes.test_libc.LibTest.test_csqrt) 2>&1 | grep -E 'libffi|lffi'
checking for libffi... yes
checking libffi has complex type support... yes
gcc -pthread -shared      Modules/_ctypes/_ctypes.o Modules/_ctypes/callbacks.o Modules/_ctypes/callproc.o Modules/_ctypes/stgdict.o Modules/_ctypes/cfield.o -lffi -ldl  -o Modules/_ctypes.cpython-314d-x86_64-linux-gnu.so
gcc -pthread -shared      Modules/_ctypes/_ctypes_test.o -lffi -ldl -lm  -o Modules/_ctypes_test.cpython-314d-x86_64-linux-gnu.so

Probably, this should be tested with build bots. Old workaround with FFI_TARGET_HAS_COMPLEX_TYPE was working e.g. on Sparc #120894 (comment)

I know nothing about build bot configuration.
It would be great if somebody helps me with it.

Member

Choose a reason for hiding this comment

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

LGTM

But someone else should trigger testing with buildbots. This and/or this.

Member

@skirpichev You can request build bots now that you're a member :)

Member

SPARCv9 fails, but I see no failures, related to affected test. On another hand, configure now prints: checking libffi has complex type support... yes. Maybe this bot now uses newer libffi?

Lets wait PPC64LE, I guess it's queried.

Member Author

SPARCv9 fails, but I see no failures, related to affected test. On another hand, configure now prints: checking libffi has complex type support... yes. Maybe this bot now uses newer libffi?

Maybe it is.

I can see such line in log from this issue:

gcc -fno-strict-overflow -I/usr/lib/sparcv9/libffi-3.2.1/include -fno-strict-overflow -fstack-protector-strong -Wtrampolines -Wsign-compare -g -Og -Wall -D_REENTRANT   -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden  -I./Include/internal -I./Include/internal/mimalloc  -I. -I./Include    -fPIC -c ./Modules/_ctypes/_ctypes.c -o Modules/_ctypes/_ctypes.o

But in actual log it differs:

gcc -fno-strict-overflow -DFFI_NO_RAW_API -fno-strict-overflow -Wsign-compare -g -Og -Wall -D_REENTRANT   -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden  -I./Include/internal -I./Include/internal/mimalloc  -I. -I./Include    -fPIC -c ./Modules/_ctypes/_ctypes.c -o Modules/_ctypes/_ctypes.o

Lets wait PPC64LE, I guess it's queried.

Yes, it is.

Member

Ok, PPC64LE build was successful (and it doesn't detect a working libffi, as expected). I can't suggest some other built bot to test. LGTM.

PS: It's silly that PKG_CHECK_MODULES doesn't report a version found. (Of course, we could echoing somewhere pkg-config --modversion libffi, but...)

Contributor

Not to derail this at all, but may want to consider looking at #103438 (#103439) in a followup too?

Contributor

Yes, I know. I'm mentioning it because there's other libffi cleanups which have gone awry because an approach couldn't be agreed upon for them. The same approach used here could work, but nevermind.

Member

The same approach used here could work

It was already suggested in the referenced issue thread.

Member Author

Maybe, there is anything I could improve?

Member

Choose a reason for hiding this comment

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