python-cffi / cffi

A Foreign Function Interface package for calling C libraries from Python.
https://cffi.readthedocs.io/en/latest/
Other
133 stars 42 forks source link

Running tests in parallel can cause compilation errors #134

Open mattip opened 1 month ago

mattip commented 1 month ago

When running tests with pytest-xtest, different test runs will try to compile the test code in the same __pycache__ directory. If the tests contain the same C code, this will lead to a situation where two tests are trying to build the same extension simultaneously, which can lead to compilation overwriting the same files, which will not work on windows (and perhaps elsewhere).

For instance, these tests will produce the same cffi source hash so calling ffi.verify will generate exactly the same object and linker output

def test_rounding_1():
    ffi = FFI()
    ffi.cdef("double sinf(float x);")
    lib = ffi.verify('#include <math.h>', libraries=lib_m)
    res = lib.sinf(1.23)
    assert res != math.sin(1.23)     # not exact, because of double->float
    assert abs(res - math.sin(1.23)) < 1E-5

def test_rounding_2():
    ffi = FFI()
    ffi.cdef("double sin(float x);")
    lib = ffi.verify('#include <math.h>', libraries=lib_m)
    res = lib.sin(1.23)
    assert res != math.sin(1.23)     # not exact, because of double->float
    assert abs(res - math.sin(1.23)) < 1E-5

One possible mitigation could be to change the __init__ of the FFI class to add a random `ffi.cdef("/ %d /" % random.randint(0, 32_000))

arigo commented 1 month ago

Is that a test-only issue, which we should fix by hacking at the tests? Or is that a real-world issue? I would think the former only, because if I understand it correctly it's only about the long-deprecated ffi.verify(). Am I correct?

arigo commented 1 month ago

Also, wtf, these two tests are entirely identical to each other. I think they were originally not, but were refactored into that. We can just remove one of the copies. But I guess they are not the only case of the problem described here.

mattip commented 1 month ago

Yes, this is a test-only issue, and maybe only on windows. I see random test failures on macos too on the pypy buildbot after translation, but that might be something else.

mattip commented 1 month ago

This seems to work. It needs to hack both __init__ and verify since comments are discarded when parsing, and any declaration needs a definition. Two tests also needed adjustment. Is there a cleaner work around?

test hack ```diff diff --git a/extra_tests/cffi_tests/cffi0/test_verify.py b/extra_tests/cffi_tests/cffi0/test_verify.py index c0fab8af5e2..fcf01ccc2b9 100644 --- a/extra_tests/cffi_tests/cffi0/test_verify.py +++ b/extra_tests/cffi_tests/cffi0/test_verify.py @@ -1,8 +1,9 @@ # Generated by pypy/tool/import_cffi.py import re import pytest +import random import sys, os, math, weakref -from cffi import FFI, VerificationError, VerificationMissing, model, FFIError +from cffi import FFI as cffi_FFI, VerificationError, VerificationMissing, model, FFIError from extra_tests.cffi_tests.support import * from extra_tests.cffi_tests.support import extra_compile_args, is_musl @@ -19,9 +20,35 @@ if sys.platform == 'win32': if distutils.ccompiler.get_default_compiler() == 'msvc': lib_m = ['msvcrt'] pass # no obvious -Werror equivalent on MSVC + class FFI(cffi_FFI): + def __init__(self, backend=None): + # Add a random piece of text to prevent hash collisions */ + super(FFI, self).__init__(backend) + self.cdef("static int nothing;") + rand = random.randint(0,32000) + def verify(self, *args, **kwds): + rand = random.randint(0,32000) + decl = "\nint nothing = %d;" % rand + if args: + args = (args[0] + decl,) + args[1:] + else: + args = [decl] + return super(FFI, self).verify( + *args, **kwds) else: - class FFI(FFI): + class FFI(cffi_FFI): + def __init__(self, backend=None): + # Add something to prevent hash collisions */ + super(FFI, self).__init__(backend) + self.cdef("static int nothing;") + def verify(self, *args, **kwds): + rand = random.randint(0,32000) + decl = "\nint nothing = %d;" % rand + if args: + args = (args[0] + decl,) + args[1:] + else: + args = (decl,) return super(FFI, self).verify( *args, extra_compile_args=extra_compile_args, **kwds) @@ -36,6 +63,7 @@ def setup_module(): _r_comment = re.compile(r"/\*.*?\*/|//.*?$", re.DOTALL | re.MULTILINE) _r_string = re.compile(r'\".*?\"') def _write_source_and_check(self, file=None): + base_write_source(self, file) if file is None: f = open(self.sourcefilename) @@ -1947,7 +1975,7 @@ def test_dir(): #define sv2 "text" enum my_e { AA, BB }; #define FOO 42""") - assert dir(lib) == ['AA', 'BB', 'FOO', 'somearray', + assert dir(lib) == ['AA', 'BB', 'FOO', 'nothing', 'somearray', 'somefunc', 'somevar', 'sv2'] def test_typeof_func_with_struct_argument(): @@ -2246,7 +2274,7 @@ def test_implicit_unicode_on_windows(): assert ord(outbuf[0]) < 128 # should be a letter, or '\' def test_use_local_dir(): - ffi = FFI() + ffi = cffi_FFI() lib = ffi.verify("", modulename="test_use_local_dir") this_dir = os.path.dirname(__file__) pycache_files = os.listdir(os.path.join(this_dir, '__pycache__')) ```
arigo commented 1 month ago

Uh, hacking indeed. Maybe instead touch the code that comes up with the hash in the first place? Something like: if you set an undocumented attribute on the FFI class, then ffi.verify() won't generate a hash, but instead use that hash directly. Requires a small change in the non-test source code, but it's cleaner IMHO.

arigo commented 1 month ago

Or just a new keyword argument passed to ffi.verify(), with verify() overridden in test classes like you do, to pass _force_hash=some_long_random_string

arigo commented 1 month ago

According to the sources, you can either insert a comment in the first argument to ffi.verify()---which should influence the hash---or you can pass an explicit name with the modulename keyword argument, which overrides the hash computation completely.

mattip commented 1 month ago

you can pass an explicit name with the modulename keyword argument

+1

samdoran commented 1 month ago

I am working on fixing this in #83 to allow running the tests concurrently.

mattip commented 1 month ago

Sorry, I didn't see #83 before working on #135 which is passing CI with pytest-xdist enabled.