Added num/malachite-bigint features #5131
Conversation
h7kanna
changed the title
|
6 tests fail compared to 287 tests OK.
6 tests failed:
test_grp test_pathlib test_posixpath test_pwd test_shutil
test_socket
10 tests skipped:
test_compile test_ctypes test_devpoll test_epoll test_kqueue
test_socketserver test_univnewlines test_urllib2net test_urllibnet
test_zipfile64
num-bigint 281 tests OK.
12 tests failed:
test_grp test_gzip test_long test_pathlib test_posixpath test_pwd
test_shutil test_socket test_tarfile test_xmlrpc test_zipfile
test_zlib
10 tests skipped:
test_compile test_ctypes test_devpoll test_epoll test_kqueue
test_socketserver test_univnewlines test_urllib2net test_urllibnet
test_zipfile64 |
h7kanna
changed the title
|
Hi @youknowone, this is ready for review. |
|
umm, so many cfg and features only for bigint. |
There was a problem hiding this comment.
I can't take this much complexity for bigint. My suggestion is creating rustpython-bigint in parser repository and make every crate to use it. Then each crate need to depending on it by feature. I believe we don't need to pass the bigint features to other sub crates in this case - because enabling both rustpython-bigint/malachite and rustpython-bigint/num will raise error.
| @@ -472,6 +479,26 @@ impl PyFloat { | |||
| }) | |||
| } | |||
|
|
|||
| #[cfg(feature = "num-bigint")] | |||
| #[pymethod] | |||
| fn as_integer_ratio(&self, vm: &VirtualMachine) -> PyResult<(PyIntRef, PyIntRef)> { | |||
There was a problem hiding this comment.
what's happened for this function? was there incompatibility?
There was a problem hiding this comment.
Yes, took this code from the reverted PR.
https://github.com/RustPython/RustPython/pull/4952/files#diff-d030ca0d2309f322c97032ab7179319e19b3806317cac7194ce0d0ef6c55ff1aL461
|
Yes, I agree. It is complex. I will make changes to create 'rustpython-bigint' crate. |
As per the comment, #5130 (comment) added support for both
num-bigintandmalachite-bigintto fix #5130Changes are tested on Mac and Ubuntu.
Some tests fail, but they also fail without these changes. So I assume these are flaky.
Note:
As the
num-bigintandmalachite-bigintare mutually exclusive features, care should be taken when changing things(deps) around. Most likely, adding 'default-features = false' will suffice.This should be worthwhile as the malachite dependency is LGPL and it complicates licensing of any projects that embed RustPython.
Example usage:
Tests when
num-bigintenabledAbout CI:
Again as the features are mutually exclusive, CI workflow should be run with both enabled separately potentially doubling the run time. I added the 'malachite-bigint' in the workflow as it is the default and the default tests are successful
6 tests are failing when the 'num-bigint' is enabled, they need to be checked/fixed. That can be done in a separate PR if these changes are okay.
Also, we can refactor the code to use a separate crate 'rustpython-bigint' similar to https://github.com/RustPython/Parser/blob/main/format/src/bigint.rs