-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Added num/malachite-bigint features #5131
base: main
Are you sure you want to change the base?
Conversation
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 |
Hi @youknowone, this is ready for review. |
umm, so many cfg and features only for bigint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's happened for this function? was there incompatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-bigint
andmalachite-bigint
to 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-bigint
andmalachite-bigint
are 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-bigint
enabledAbout 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