Skip to content
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.

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

Running tests in parallel can cause compilation errors #134

Open
mattip opened this issue Oct 11, 2024 · 10 comments
Open

Running tests in parallel can cause compilation errors #134

mattip opened this issue Oct 11, 2024 · 10 comments

Comments

@mattip
Copy link
Contributor

mattip commented Oct 11, 2024

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
Copy link
Contributor

arigo commented Oct 11, 2024

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
Copy link
Contributor

arigo commented Oct 11, 2024

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
Copy link
Contributor Author

mattip commented Oct 11, 2024

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
Copy link
Contributor Author

mattip commented Oct 11, 2024

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 --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
Copy link
Contributor

arigo commented Oct 11, 2024

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
Copy link
Contributor

arigo commented Oct 11, 2024

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
Copy link
Contributor

arigo commented Oct 11, 2024

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
Copy link
Contributor Author

mattip commented Oct 11, 2024

you can pass an explicit name with the modulename keyword argument

+1

@samdoran
Copy link
Contributor

samdoran commented Oct 15, 2024

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

@mattip
Copy link
Contributor Author

mattip commented Oct 15, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants