indygreg / python-zstandard

Python bindings to the Zstandard (zstd) compression library
BSD 3-Clause "New" or "Revised" License
481 stars 84 forks source link

FAILED tests/test_module_attributes.py::TestModuleAttributes::test_features fails with --system-zstd #191

Open marxin opened 1 year ago

marxin commented 1 year ago
python3 setup.py test --system-zstd
...
self = <tests.test_module_attributes.TestModuleAttributes testMethod=test_features>

    def test_features(self):
        self.assertIsInstance(zstd.backend_features, set)

        expected = {
            "cext": {
                "buffer_types",
                "multi_compress_to_buffer",
                "multi_decompress_to_buffer",
            },
            "cffi": set(),
            "rust": {
                "buffer_types",
                "multi_compress_to_buffer",
                "multi_decompress_to_buffer",
            },
        }[zstd.backend]

>       self.assertEqual(zstd.backend_features, expected)
E       AssertionError: Items in the second set but not the first:
E       'multi_compress_to_buffer'
E       'multi_decompress_to_buffer'

tests/test_module_attributes.py:29: AssertionError

I guess the tested features are available only with a statically linked copy of libzstd, right? The tests should reflect that.

marxin commented 1 year ago

It can be addressed with something like this:

diff --git a/c-ext/backend_c.c b/c-ext/backend_c.c
index e31dd15..a5d9dee 100644
--- a/c-ext/backend_c.c
+++ b/c-ext/backend_c.c
@@ -210,6 +210,20 @@ void zstd_module_init(PyObject *m) {
     Py_DECREF(feature);
 #endif

+#ifdef SYSTEM_ZSTD
+    feature = PyUnicode_FromString("system_zstd");
+    if (NULL == feature) {
+        PyErr_SetString(PyExc_ImportError, "could not create feature string");
+        return;
+    }
+
+    if (PySet_Add(features, feature) == -1) {
+        return;
+    }
+
+    Py_DECREF(feature);
+#endif
+
     if (PyObject_SetAttrString(m, "backend_features", features) == -1) {
         return;
     }
diff --git a/setup_zstd.py b/setup_zstd.py
index 399b129..d940c80 100644
--- a/setup_zstd.py
+++ b/setup_zstd.py
@@ -78,6 +78,7 @@ def get_c_extension(

     if system_zstd:
         extra_args.append("-DZSTD_MULTITHREAD")
+        extra_args.append("-DSYSTEM_ZSTD")
     else:
         extra_args.append("-DZSTD_SINGLE_FILE")
         extra_args.append("-DZSTDLIB_VISIBILITY=")
diff --git a/tests/test_module_attributes.py b/tests/test_module_attributes.py
index d487aa8..15cda73 100644
--- a/tests/test_module_attributes.py
+++ b/tests/test_module_attributes.py
@@ -26,7 +26,15 @@ class TestModuleAttributes(unittest.TestCase):
             },
         }[zstd.backend]

-        self.assertEqual(zstd.backend_features, expected)
+        # The following features are available only with
+        # statically linked version of the module.
+        available_features = set(zstd.backend_features)
+        if 'system_zstd' in available_features:
+            available_features.remove('system_zstd')
+            expected.discard('multi_compress_to_buffer')
+            expected.discard('multi_decompress_to_buffer')
+
+        self.assertEqual(available_features, expected)

     def test_constants(self):
         self.assertEqual(zstd.MAX_COMPRESSION_LEVEL, 22)
marxin commented 1 year ago

May I please ping this @indygreg ?